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.
Created attachment 384845 [details] Patch
Created attachment 384847 [details] Patch
Created attachment 384858 [details] Patch
Created attachment 384920 [details] Patch
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?
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.
Created attachment 385442 [details] Patch
Created attachment 385445 [details] Patch
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.
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).
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.
Created attachment 385511 [details] Patch
Created attachment 385512 [details] Patch
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.
> >> 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.
(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).
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
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.
Created attachment 386001 [details] Patch for landing
Comment on attachment 386001 [details] Patch for landing Clearing flags on attachment: 386001 Committed r253718: <https://trac.webkit.org/changeset/253718>
All reviewed patches have been landed. Closing bug.
<rdar://problem/58057008>
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
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