Bug 204859 – Web Inspector: Runtime.enable reports duplicates (non existent) contexts

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

[Yury Semikhatsky]

Calling Runtime.enable results in duplicate Runtime.executionContextCreated events: for each context in the main world it will report it with isPageContext=true and isPageContext=false.

[Yury Semikhatsky]

Created attachment 384845 [details]
Patch

[Yury Semikhatsky]

Created attachment 384847 [details]
Patch

[Yury Semikhatsky]

Created attachment 384858 [details]
Patch

[Yury Semikhatsky]

Created attachment 384920 [details]
Patch

[Devin Rousso]

Comment on attachment 384920 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384920&action=review

> Source/WebCore/inspector/InspectorInstrumentation.cpp:124
> -    if (&world != &mainThreadNormalWorld())
> +    if (!world.isNormal())

Why is this change necessary?

> Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:78
> +    // Report initial contexts before enabling instrumentation as the reporting
> +    // can force creation of script state which could result in duplicate notifications.
> +    reportExecutionContextCreation();

IIUC, the problem you're describing here is that `reportExecutionContextCreation()` can cause script to be evaluated, which could then pipe back to this object via the `m_instrumentingAgents.setPageRuntimeAgent(this);`?  If so, we should still call `InspectorRuntimeAgent::enable(errorString);` before doing that, as we want the agent to be considered fully "enabled" (meaning the parent class should also be "enabled") before we dispatch any events.
```
    void PageRuntimeAgent::enable(ErrorString& errorString)
    {
        if (m_instrumentingAgents.pageRuntimeAgent() == this)
            return;

        InspectorRuntimeAgent::enable(errorString);

        // Report initial contexts before connecting instrumentation as the reporting
        // can force creation of script state which could result in duplicate notifications.
        reportExecutionContextCreation();

        m_instrumentingAgents.setPageRuntimeAgent(this);
    }
```

It would be preferable to set `m_instrumentingAgents.setPageRuntimeAgent(this);` first to avoid any potential reentrancy though.  Perhaps we could keep track of the `Frame` that we're about to call `mainWorldExecState` with and early-return inside `didClearWindowObjectInWorld` if it matches?

> Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:146
> +        for (auto& context : isolatedContexts) {

NIT: you can use a structured binding:
```
    for (auto& [globalObject, securityOrigin] : isolatedContexts)
```

> Source/WebInspectorUI/UserInterface/Models/ExecutionContextList.js:48
>          this._contexts.push(context);

I think we should still have some assertion to guard against duplicate IDs:
```
    console.assert(this._contexts.every((existingContext) => existingContext.id !== context.id), "Unexpected duplicate context id", this, context);
```

> LayoutTests/http/tests/inspector/resources/stable-id-map.js:36
> +            this._protocolToStableId = new Map;

NIT: this could theoretically be used for more than just protocol ID values (e.g. timer/rAF identifiers), so I'd just make it `this._map = new Map;`.

> LayoutTests/http/tests/inspector/resources/stable-id-map.js:38
> +

Style: please add a `// Public`

> LayoutTests/http/tests/inspector/resources/stable-id-map.js:39
> +        get lastAssignedId() { return this._lastAssignedId; }

Rather than `lastAssignedId`, should we expose `this._protocolToStableId.size`?  I think they'd fundamentally map to the same thing, but `size` is more of a generic name than `lastAssignedId`.
```
    get size()
    {
        return this._protocolToStableId.size;
    }
```

> LayoutTests/http/tests/inspector/resources/stable-id-map.js:41
> +        stableId(protocolId)

I'd just call this `get(unstableId)`, since the name of this class already has "stable" so this would be somewhat repetitive.

> LayoutTests/http/tests/inspector/resources/stable-id-map.js:44
> +            if (!id) {

So this means that this will never assign a stable ID of 0?  That's fine I guess :)

> LayoutTests/inspector/runtime/execution-context-events.html:6
> +

Style: unnecessary newline

> LayoutTests/inspector/runtime/execution-context-events.html:27
> +                InspectorProtocol.awaitCommand({method: "Runtime.enable"})

Style: missing trailing comma

> LayoutTests/inspector/runtime/execution-context-events.html:30
> +            ProtocolTest.expectEqual(contextCount, 2, "Should have 2 contexts (for main world in the main frame and the subframe)")

Why not test for `contextIdMap.size` and avoid having another variable?

[Yury Semikhatsky]

Comment on attachment 384920 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384920&action=review

>> Source/WebCore/inspector/InspectorInstrumentation.cpp:124
>> +    if (!world.isNormal())
> 
> Why is this change necessary?

It's a drive-by making this check more straightforward and likely more efficient due to memory locality.

>> Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:78
>> +    reportExecutionContextCreation();
> 
> IIUC, the problem you're describing here is that `reportExecutionContextCreation()` can cause script to be evaluated, which could then pipe back to this object via the `m_instrumentingAgents.setPageRuntimeAgent(this);`?  If so, we should still call `InspectorRuntimeAgent::enable(errorString);` before doing that, as we want the agent to be considered fully "enabled" (meaning the parent class should also be "enabled") before we dispatch any events.
> ```
>     void PageRuntimeAgent::enable(ErrorString& errorString)
>     {
>         if (m_instrumentingAgents.pageRuntimeAgent() == this)
>             return;
> 
>         InspectorRuntimeAgent::enable(errorString);
> 
>         // Report initial contexts before connecting instrumentation as the reporting
>         // can force creation of script state which could result in duplicate notifications.
>         reportExecutionContextCreation();
> 
>         m_instrumentingAgents.setPageRuntimeAgent(this);
>     }
> ```
> 
> It would be preferable to set `m_instrumentingAgents.setPageRuntimeAgent(this);` first to avoid any potential reentrancy though.  Perhaps we could keep track of the `Frame` that we're about to call `mainWorldExecState` with and early-return inside `didClearWindowObjectInWorld` if it matches?

Done. Super class is 'enabled' before reporting.

I like better not adding PageRuntimeAgent to the instrumentation until all contexts are reported. Introducing a state that we are currently in the process of initialization or even reporting contexts for a particular frame would be more convoluted and require an extra field on the agent. For now the agent is instrumenting only 2 events: frame navigated and didClearWindowObjectInWorld so it's safe to enable the instrumentation later.

>> Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:146
>> +        for (auto& context : isolatedContexts) {
> 
> NIT: you can use a structured binding:
> ```
>     for (auto& [globalObject, securityOrigin] : isolatedContexts)
> ```

Done.

>> Source/WebInspectorUI/UserInterface/Models/ExecutionContextList.js:48
>>          this._contexts.push(context);
> 
> I think we should still have some assertion to guard against duplicate IDs:
> ```
>     console.assert(this._contexts.every((existingContext) => existingContext.id !== context.id), "Unexpected duplicate context id", this, context);
> ```

Done.

[Yury Semikhatsky]

Created attachment 385442 [details]
Patch

[Yury Semikhatsky]

Created attachment 385445 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 384920 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384920&action=review

>> LayoutTests/http/tests/inspector/resources/stable-id-map.js:36
>> +            this._protocolToStableId = new Map;
> 
> NIT: this could theoretically be used for more than just protocol ID values (e.g. timer/rAF identifiers), so I'd just make it `this._map = new Map;`.

Done.

>> LayoutTests/http/tests/inspector/resources/stable-id-map.js:38
>> +
> 
> Style: please add a `// Public`

Done.

>> LayoutTests/http/tests/inspector/resources/stable-id-map.js:39
>> +        get lastAssignedId() { return this._lastAssignedId; }
> 
> Rather than `lastAssignedId`, should we expose `this._protocolToStableId.size`?  I think they'd fundamentally map to the same thing, but `size` is more of a generic name than `lastAssignedId`.
> ```
>     get size()
>     {
>         return this._protocolToStableId.size;
>     }
> ```

Done.

>> LayoutTests/http/tests/inspector/resources/stable-id-map.js:41
>> +        stableId(protocolId)
> 
> I'd just call this `get(unstableId)`, since the name of this class already has "stable" so this would be somewhat repetitive.

Done.

>> LayoutTests/http/tests/inspector/resources/stable-id-map.js:44
>> +            if (!id) {
> 
> So this means that this will never assign a stable ID of 0?  That's fine I guess :)

Yes, this was intentional as 0 sometimes has special meaning of not set.

>> LayoutTests/inspector/runtime/execution-context-events.html:6
>> +
> 
> Style: unnecessary newline

Done.

>> LayoutTests/inspector/runtime/execution-context-events.html:27
>> +                InspectorProtocol.awaitCommand({method: "Runtime.enable"})
> 
> Style: missing trailing comma

Done.

>> LayoutTests/inspector/runtime/execution-context-events.html:30
>> +            ProtocolTest.expectEqual(contextCount, 2, "Should have 2 contexts (for main world in the main frame and the subframe)")
> 
> Why not test for `contextIdMap.size` and avoid having another variable?

This is to cover the case of duplicate contexts which this patch is fixing. Actually, we need to test that all of them are unique too. Updated the test to do it explicitly.

[Devin Rousso]

Comment on attachment 385445 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385445&action=review

> Source/WebCore/inspector/InspectorInstrumentation.cpp:124
> -    if (&world != &mainThreadNormalWorld())
> +    if (!world.isNormal())

>> Why is this change necessary?
> It's a drive-by making this check more straightforward and likely more efficient due to memory locality.
This seems like a potentially very risky change that shouldn't be a drive-by.  If this is unnecessary for this patch, please remove it.  If it is necessary, please explain why.

Also, "likely more efficient" is not a good reason for making changes like these.  If it is definitely more memory efficient, then maybe it can be its own change.

> Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:78
> +    if (!errorString.isEmpty())
> +        return;

We know that `InspectorRuntimeAgent::enable` doesn't populate `errorString`, and if we passed the first early return then we can safely assume that the parent class hasn't been enabled yet either.  As such, we can remove this.

> LayoutTests/http/tests/inspector/resources/stable-id-map.js:45
> +            let id = this._idMap.get(unstableId);

>> So this means that this will never assign a stable ID of 0?  That's fine I guess :)
> Yes, this was intentional as 0 sometimes has special meaning of not set.
If that's the case, maybe we should also assert that `unstableId !== 0`?  We wouldn't want to use `console.assert`, but instead probably something like `(InspectorTest || ProtocolTest).assert(unstableId, "StableIdMap should only be given truthy ID values.")`.

> LayoutTests/inspector/runtime/execution-context-events.html:8
> +    let suite = ProtocolTest.createAsyncSuite("Runtime.executionContextCreated.OnEnable");

NIT: we normally have the test file name match (or be close) to the suite name in that test file.  As such, I'd rename this test file to 'LayoutTests/inspector/runtime/executionContextCreated-OnEnable.html'.

> LayoutTests/inspector/runtime/execution-context-events.html:20
> +                ProtocolTest.addResult(`Execution context created: id=${contextIdMap.get(id)} frameId=${frameIdMap.get(frameId)} isPageContext=${isPageContext}`)

Please use `ProtocolTest.log` instead.

> LayoutTests/inspector/runtime/execution-context-events.html:30
> +            ProtocolTest.expectEqual(contextCount, 2, "Should receive 2 executionContextCreated events (for main world in the main frame and the subframe)");
> +            ProtocolTest.expectEqual(contextIdMap.size, 2, "Should have 2 unique contexts");

Style: please make sure that any expectation strings are sentences that end with "." (we don't care as much for regular logs).

[Yury Semikhatsky]

Comment on attachment 385445 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385445&action=review

>> Source/WebCore/inspector/InspectorInstrumentation.cpp:124
>> +    if (!world.isNormal())
> 
> 

> This seems like a potentially very risky change that shouldn't be a drive-by.  If this is unnecessary for this patch, please remove it.  If it is necessary, please explain why.
This is not necessary for the change. Reverting.

I'm curious why you think this is a 'very risky' change?


>Also, "likely more efficient" is not a good reason for making changes like these.  If it is definitely more memory efficient, then maybe it can be its own change.
Well, first of all its more readable. Efficiency is secondary here as this code is not hot to try and optimize for memory cache locality.

>> Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:78
>> +        return;
> 
> We know that `InspectorRuntimeAgent::enable` doesn't populate `errorString`, and if we passed the first early return then we can safely assume that the parent class hasn't been enabled yet either.  As such, we can remove this.

This sounds like too many assumption about the base class. Its logic can change in the future to allow failures and this code will break forcing the author to think whether errors were intentionally ignored before or it was a bug. Why not make it explicit.

>> LayoutTests/http/tests/inspector/resources/stable-id-map.js:45
>> +            let id = this._idMap.get(unstableId);
> 
> 

> If that's the case, maybe we should also assert that `unstableId !== 0`?  We wouldn't want to use `console.assert`, but instead probably something like `(InspectorTest || ProtocolTest).assert(unstableId, > "StableIdMap should only be given truthy ID values.")`.
I feel like this getting more attention than necessary. Changed to use 0-based ids instead.

>> LayoutTests/inspector/runtime/execution-context-events.html:8
>> +    let suite = ProtocolTest.createAsyncSuite("Runtime.executionContextCreated.OnEnable");
> 
> NIT: we normally have the test file name match (or be close) to the suite name in that test file.  As such, I'd rename this test file to 'LayoutTests/inspector/runtime/executionContextCreated-OnEnable.html'.

Renamed.

Why do inspector tests not follow file-naming-convention of other layout tests and use camel case?

>> LayoutTests/inspector/runtime/execution-context-events.html:20
>> +                ProtocolTest.addResult(`Execution context created: id=${contextIdMap.get(id)} frameId=${frameIdMap.get(frameId)} isPageContext=${isPageContext}`)
> 
> Please use `ProtocolTest.log` instead.

Done.

>> LayoutTests/inspector/runtime/execution-context-events.html:30
>> +            ProtocolTest.expectEqual(contextIdMap.size, 2, "Should have 2 unique contexts");
> 
> Style: please make sure that any expectation strings are sentences that end with "." (we don't care as much for regular logs).

Done.

[Yury Semikhatsky]

Created attachment 385511 [details]
Patch

[Yury Semikhatsky]

Created attachment 385512 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 385512 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385512&action=review

Looks good to me. Lets see if Devin is good with the most recent revision given his earlier comments.

> Source/WebInspectorUI/UserInterface/Models/ExecutionContextList.js:-52
> -        // FIXME: The backend sends duplicate page context execution contexts with the same id. Why?
> -        if (context.isPageContext && this._pageExecutionContext) {
> -            console.assert(context.id === this._pageExecutionContext.id);
> -            return false;
> -        }

It looks like we will still want something like this code for the Remote Inspector connected to legacy backends that do continue to send duplicates.

Perhaps keep the code and add:

    // COMPATIBILITY (iOS 13.0): Older iOS releases will send duplicates. Newer releases will not.

> LayoutTests/http/tests/inspector/resources/stable-id-map.js:2
> + * Copyright (C) 2019 Microsoft Corporation. All rights reserved.

Is there any need for this file to have a license? Most WebKit test code doesn't and this is effectively 10 non-complex statements.

> LayoutTests/http/tests/inspector/resources/stable-id-map.js:35
> +            this._lastAssignedId = 0;

This is unused.

[Joseph Pecoraro]

> >> Source/WebCore/inspector/InspectorInstrumentation.cpp:124
> >> +    if (!world.isNormal())
> 
> > This seems like a potentially very risky change that shouldn't be a drive-by.  If this is unnecessary for this patch, please remove it.  If it is necessary, please explain why.
> This is not necessary for the change. Reverting.
> 
> I'm curious why you think this is a 'very risky' change?

I agree with Yury here that the `isNormal()` is simpler and probably. I think it is worth moving in that direction. But this can be done separately if you have concerns.

[Devin Rousso]

(In reply to Joseph Pecoraro from comment #15)
> > >> Source/WebCore/inspector/InspectorInstrumentation.cpp:124
> > >> +    if (!world.isNormal())
> > > This seems like a potentially very risky change that shouldn't be a drive-by.  If this is unnecessary for this patch, please remove it.  If it is necessary, please explain why.
> > This is not necessary for the change. Reverting.
> > 
> > I'm curious why you think this is a 'very risky' change?
> I agree with Yury here that the `isNormal()` is simpler and probably. I think it is worth moving in that direction. But this can be done separately if you have concerns.
I agree that this is probably better, but I'd rather have it be a separate change so that if it does end up breaking something, the rollout is much more localized and wouldn't affect the fix of this bug.  I'd also want to make sure it has a test (somehow).

[Devin Rousso]

Comment on attachment 385512 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385512&action=review

r=me

> Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:156
> +            if (scriptState != globalObject)

NIT: I'd flip the order of these, given that `globalObject` is more local than `scriptState`

>> Source/WebInspectorUI/UserInterface/Models/ExecutionContextList.js:-52
>> -        }
> 
> It looks like we will still want something like this code for the Remote Inspector connected to legacy backends that do continue to send duplicates.
> 
> Perhaps keep the code and add:
> 
>     // COMPATIBILITY (iOS 13.0): Older iOS releases will send duplicates. Newer releases will not.

Ah, yeah!  I totally forgot about that.  Good catch Joe!

>> LayoutTests/http/tests/inspector/resources/stable-id-map.js:35
>> +            this._lastAssignedId = 0;
> 
> This is unused.

NIT: this is no longer used

[Yury Semikhatsky]

Comment on attachment 385512 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385512&action=review

>> Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:156
>> +            if (scriptState != globalObject)
> 
> NIT: I'd flip the order of these, given that `globalObject` is more local than `scriptState`

Done.

>>> Source/WebInspectorUI/UserInterface/Models/ExecutionContextList.js:-52
>>> -        }
>> 
>> It looks like we will still want something like this code for the Remote Inspector connected to legacy backends that do continue to send duplicates.
>> 
>> Perhaps keep the code and add:
>> 
>>     // COMPATIBILITY (iOS 13.0): Older iOS releases will send duplicates. Newer releases will not.
> 
> Ah, yeah!  I totally forgot about that.  Good catch Joe!

Good catch! Done.

>> LayoutTests/http/tests/inspector/resources/stable-id-map.js:2
>> + * Copyright (C) 2019 Microsoft Corporation. All rights reserved.
> 
> Is there any need for this file to have a license? Most WebKit test code doesn't and this is effectively 10 non-complex statements.

I don't know what's the general rule for layout test .js files these days but all .js in LayoutTests/http/tests/inspector/resources/ have license header so I've added this for consistency. Also I don't think we ever use size of a source file as a criteria for adding license header, they usually start small and simple and grow over time.

>>> LayoutTests/http/tests/inspector/resources/stable-id-map.js:35
>>> +            this._lastAssignedId = 0;
>> 
>> This is unused.
> 
> NIT: this is no longer used

Removed.

[Yury Semikhatsky]

Created attachment 386001 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 386001 [details]
Patch for landing

Clearing flags on attachment: 386001

Committed r253718: <https://trac.webkit.org/changeset/253718>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58057008>

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/253718/webkit

broke http/tests/inspector/target/provisional-load-cancels-previous-load.html

History:
https://results.webkit.org/?suite=layout-tests&test=http%2Ftests%2Finspector%2Ftarget%2Fprovisional-load-cancels-previous-load.html


tracked in https://bugs.webkit.org/show_bug.cgi?id=205473

[Truitt Savell]

The new test inspector/runtime/executionContextCreated-onEnable.html

added in https://trac.webkit.org/changeset/253718/webkit

is flaky

tracking in https://bugs.webkit.org/show_bug.cgi?id=205956