Bug 206110 – Web Inspector: unable to evaluate in the isolated world of content scripts injected by safari app extensions

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

.

[Devin Rousso]

<rdar://problem/16945643>

[Devin Rousso]

Created attachment 387400 [details]
Patch

[EWS Watchlist]

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.

[Devin Rousso]

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.

[Joseph Pecoraro]

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 }`

[Timothy Hatcher]

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 ')'?

[Joseph Pecoraro]

(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.

[Blaze Burg]

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?

[Devin Rousso]

Created attachment 388830 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 388830 [details]
Patch

Clearing flags on attachment: 388830

Committed r255191: <https://trac.webkit.org/changeset/255191>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Devin Rousso]

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

[Devin Rousso]

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

[Devin Rousso]

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

[Pavel Feldman]

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);
}

[Devin Rousso]

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?

[Devin Rousso]

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

[Pavel Feldman]

Happy to follow up and fix it!

[Joseph Pecoraro]

(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)