Bug 206110

<rdar://problem/16945643>

Created attachment 387400 [details] Patch

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

Created attachment 387401 [details] [Image] After Patch is applied The checkbox is shown for _both_ "Auto - <...>" and the resolved execution context when "Auto - <...>" is selected so that if there are multiple execution contexts with the same display name, it's clear which one is currently active.

Comment on attachment 387400 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387400&action=review > Source/WebKit/WebProcess/InjectedBundle/InjectedBundleScriptWorld.h:43 > + static Ref<InjectedBundleScriptWorld> create(bool isInternal = true); > + static Ref<InjectedBundleScriptWorld> create(const String& name, bool isInternal = true); An `enum` would reach so much better. `enum Type { User, Internal }`

Comment on attachment 387400 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387400&action=review > Source/WebCore/html/HTMLMediaElement.cpp:7209 > + m_isolatedWorld = DOMWrapperWorld::create(commonVM(), DOMWrapperWorld::Type::Internal, makeString("Media Controls (", localName(), ')')); Should you use ")" instead of ')'?

(In reply to Timothy Hatcher from comment #6) > Comment on attachment 387400 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=387400&action=review > > > Source/WebCore/html/HTMLMediaElement.cpp:7209 > > + m_isolatedWorld = DOMWrapperWorld::create(commonVM(), DOMWrapperWorld::Type::Internal, makeString("Media Controls (", localName(), ')')); > > Should you use ")" instead of ')'? This can be kept a character literal instead of a string. It is typically more efficient for string concatenation to deal with a `char` instead of a multi-character string literal `char*`. Though I'm not sure how much of this gets optimized by the compiler, since some of this goes through templated code and the strlen(...) of a literal like this can typically be done at compile time.

Comment on attachment 387400 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387400&action=review r=me as well, but some EWS problems remain. Great work! > Source/WebCore/bindings/js/WebCoreJSClientData.cpp:120 > + clientData->m_normalWorld = DOMWrapperWorld::create(*vm, DOMWrapperWorld::Type::Normal); Lovely! > Source/WebCore/inspector/agents/InspectorPageAgent.cpp:774 > +void InspectorPageAgent::didClearWindowObjectInWorld(Frame& frame, DOMWrapperWorld& world) Well that was an awkward method signature, but now it reads better. >> Source/WebKit/WebProcess/InjectedBundle/InjectedBundleScriptWorld.h:43 >> + static Ref<InjectedBundleScriptWorld> create(const String& name, bool isInternal = true); > > An `enum` would reach so much better. `enum Type { User, Internal }` enum class?

Created attachment 388830 [details] Patch

Comment on attachment 388830 [details] Patch Clearing flags on attachment: 388830 Committed r255191: <https://trac.webkit.org/changeset/255191>

All reviewed patches have been landed. Closing bug.

Unreviewed, speculative win build fix: <https://trac.webkit.org/r255212>

Created attachment 388959 [details] [Patch] speculative win build fix

Unreviewed, speculative win build fix: <https://trac.webkit.org/r255221>

Comment on attachment 388830 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388830&action=review > Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:178 > + auto injectedScript = injectedScriptManager().injectedScriptFor(globalObject); I believe there is a subtle issue here - when didClearWindowObjectInWorld is called for the User world before it is called for the Normal world, subsequent call to clear the normal world will erase this injected script from the InjectedScriptManager through m_injectedScriptManager->discardInjectedScripts(). This can either be patched via the following change to the dispatchDidClearWindowObjectsInAllWorlds (that is somewhat hacky), or via going back to instrumenting only normal worlds and iterating over the remaining worlds within agents explicitly. void FrameLoader::dispatchDidClearWindowObjectsInAllWorlds() { Vector<Ref<DOMWrapperWorld>> worlds; ScriptController::getAllWorlds(worlds); // It is essential that the normal world is cleared first. // Various subsystem (InjectedScriptManager) will reset state upon normal // world initialization. Vector<DOMWrapperWorld*> nonNormalWorlds; for (auto& world : worlds) { if (world->type() == DOMWrapperWorld::Type::Normal) dispatchDidClearWindowObjectInWorld(world); else nonNormalWorlds.append(&world.get()); } for (auto* world : nonNormalWorlds) dispatchDidClearWindowObjectInWorld(*world); }

Comment on attachment 388830 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388830&action=review >> Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:178 >> + auto injectedScript = injectedScriptManager().injectedScriptFor(globalObject); > > I believe there is a subtle issue here - when didClearWindowObjectInWorld is called for the User world before it is called for the Normal world, subsequent call to clear the normal world will erase this injected script from the InjectedScriptManager through m_injectedScriptManager->discardInjectedScripts(). > > This can either be patched via the following change to the dispatchDidClearWindowObjectsInAllWorlds (that is somewhat hacky), or via going back to instrumenting only normal worlds and iterating over the remaining worlds within agents explicitly. > > void FrameLoader::dispatchDidClearWindowObjectsInAllWorlds() > { > Vector<Ref<DOMWrapperWorld>> worlds; > ScriptController::getAllWorlds(worlds); > // It is essential that the normal world is cleared first. > // Various subsystem (InjectedScriptManager) will reset state upon normal > // world initialization. > Vector<DOMWrapperWorld*> nonNormalWorlds; > for (auto& world : worlds) { > if (world->type() == DOMWrapperWorld::Type::Normal) > dispatchDidClearWindowObjectInWorld(world); > else > nonNormalWorlds.append(&world.get()); > } > for (auto* world : nonNormalWorlds) > dispatchDidClearWindowObjectInWorld(*world); > } Interesting! That is super subtle :P I think we'd ideally want to dispatch for the `mainThreadNormalWorld()` first (as that's really the "signpost" for resetting all the injected scripts and debugger state), then all other normal worlds, and then all other worlds. IIUC, there should only ever be one normal world though (and the current code supports that), so I think we can just do as you suggest. @Pavel Feldman, do you want to fix this, or should I?

Comment on attachment 388830 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388830&action=review >>> Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:178 >>> + auto injectedScript = injectedScriptManager().injectedScriptFor(globalObject); >> >> I believe there is a subtle issue here - when didClearWindowObjectInWorld is called for the User world before it is called for the Normal world, subsequent call to clear the normal world will erase this injected script from the InjectedScriptManager through m_injectedScriptManager->discardInjectedScripts(). >> >> This can either be patched via the following change to the dispatchDidClearWindowObjectsInAllWorlds (that is somewhat hacky), or via going back to instrumenting only normal worlds and iterating over the remaining worlds within agents explicitly. >> >> void FrameLoader::dispatchDidClearWindowObjectsInAllWorlds() >> { >> Vector<Ref<DOMWrapperWorld>> worlds; >> ScriptController::getAllWorlds(worlds); >> // It is essential that the normal world is cleared first. >> // Various subsystem (InjectedScriptManager) will reset state upon normal >> // world initialization. >> Vector<DOMWrapperWorld*> nonNormalWorlds; >> for (auto& world : worlds) { >> if (world->type() == DOMWrapperWorld::Type::Normal) >> dispatchDidClearWindowObjectInWorld(world); >> else >> nonNormalWorlds.append(&world.get()); >> } >> for (auto* world : nonNormalWorlds) >> dispatchDidClearWindowObjectInWorld(*world); >> } > > Interesting! That is super subtle :P > > I think we'd ideally want to dispatch for the `mainThreadNormalWorld()` first (as that's really the "signpost" for resetting all the injected scripts and debugger state), then all other normal worlds, and then all other worlds. IIUC, there should only ever be one normal world though (and the current code supports that), so I think we can just do as you suggest. > > @Pavel Feldman, do you want to fix this, or should I? <https://webkit.org/b/207232> Web Inspector: ensure that `didClearWindowObjectInWorld` is dispatched for the main world first

Happy to follow up and fix it!

(In reply to Pavel Feldman from comment #18) > Happy to follow up and fix it! I think Devin just did in bug 207232. (Though no test)