Bug 217980

Created attachment 411903 [details] Patch

Created attachment 411905 [details] Patch

Comment on attachment 411905 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411905&action=review > Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:299 > + auto guardedObjects = m_guardedObjects; We are copying this so it does the right thing even if some objects are removed while we iterate. Three questions: 1) Can we make clearer that this is a copy, perhaps by including copy in the name of the local variable or adding a comment? 2) Is it OK to call clear if this object is removed and then perhaps reused in some way? 3) Is it OK that this does not clear objects that are added during the iteration? Looks like this code was just moved, so none of this is new. > Source/WebCore/bindings/js/JSDOMGlobalObject.h:87 > + JSC::JSProxy* proxy() const { ASSERT(m_proxy); return m_proxy.get(); } If we can assert this, it seems we should return a reference, not a pointer. > Source/WebCore/bindings/js/JSDOMWindowBase.cpp:253 > JSWindowProxy* JSDOMWindowBase::proxy() const Reference?

(In reply to Darin Adler from comment #3) > Comment on attachment 411905 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=411905&action=review > > > Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:299 > > + auto guardedObjects = m_guardedObjects; > > We are copying this so it does the right thing even if some objects are > removed while we iterate. Three questions: > > 1) Can we make clearer that this is a copy, perhaps by including copy in the > name of the local variable or adding a comment? Will add "Copy" suffix. > 2) Is it OK to call clear if this object is removed and then perhaps reused > in some way? > 3) Is it OK that this does not clear objects that are added during the > iteration? I am actually not familiar with this code. However, looking at it, it seems that calling clear() on the DOMGuardedObjects is not a complex operation and from my glancing at it, I do not believe it can cause objects to get added to the list. Pretty much all clear() does it grab a lock and remove the object from the list. > > Looks like this code was just moved, so none of this is new. Correct, I merely moved it. > > > Source/WebCore/bindings/js/JSDOMGlobalObject.h:87 > > + JSC::JSProxy* proxy() const { ASSERT(m_proxy); return m_proxy.get(); } > > If we can assert this, it seems we should return a reference, not a pointer. > > > Source/WebCore/bindings/js/JSDOMWindowBase.cpp:253 > > JSWindowProxy* JSDOMWindowBase::proxy() const > > Reference? Ok, will do.

Created attachment 411925 [details] Patch

Created attachment 411927 [details] Patch

Created attachment 411928 [details] Patch

Committed r268775: <https://trac.webkit.org/changeset/268775> All reviewed patches have been landed. Closing bug and clearing flags on attachment 411928 [details].

<rdar://problem/70508220>