Web Inspector: Runtime.enable reports duplicates (non existent) contexts
authoryurys@chromium.org <yurys@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 18 Dec 2019 22:05:27 +0000 (22:05 +0000)
committeryurys@chromium.org <yurys@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 18 Dec 2019 22:05:27 +0000 (22:05 +0000)
https://bugs.webkit.org/show_bug.cgi?id=204859

Reviewed by Devin Rousso.

Source/WebCore:

Do not report main world context as non-main world one when Runtime.enable is called.

Test: inspector/runtime/executionContextCreated-onEnable.html

* inspector/agents/page/PageRuntimeAgent.cpp:
(WebCore::PageRuntimeAgent::enable):
(WebCore::PageRuntimeAgent::reportExecutionContextCreation):

Source/WebInspectorUI:

Assert that all contexts added to the list are unique.

* UserInterface/Models/ExecutionContextList.js:
(WI.ExecutionContextList.prototype.add):

LayoutTests:

Test that only existing contexts are reported.

* http/tests/inspector/resources/stable-id-map.js: Added.
(TestPage.registerInitializer.window.StableIdMap):
(TestPage.registerInitializer.window.StableIdMap.prototype.get size):
(TestPage.registerInitializer.window.StableIdMap.prototype.get let):
(TestPage.registerInitializer):
* http/tests/inspector/target/provisional-load-cancels-previous-load.html:
* inspector/runtime/executionContextCreated-onEnable-expected.txt: Added.
* inspector/runtime/executionContextCreated-onEnable.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@253718 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/http/tests/inspector/resources/stable-id-map.js [new file with mode: 0644]
LayoutTests/http/tests/inspector/target/provisional-load-cancels-previous-load-expected.txt
LayoutTests/http/tests/inspector/target/provisional-load-cancels-previous-load.html
LayoutTests/inspector/runtime/executionContextCreated-onEnable-expected.txt [new file with mode: 0644]
LayoutTests/inspector/runtime/executionContextCreated-onEnable.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp
Source/WebInspectorUI/ChangeLog
Source/WebInspectorUI/UserInterface/Models/ExecutionContextList.js

index 5f7e841..478ca58 100644 (file)
@@ -1,3 +1,21 @@
+2019-12-18  Yury Semikhatsky  <yurys@chromium.org>
+
+        Web Inspector: Runtime.enable reports duplicates (non existent) contexts
+        https://bugs.webkit.org/show_bug.cgi?id=204859
+
+        Reviewed by Devin Rousso.
+
+        Test that only existing contexts are reported.
+
+        * http/tests/inspector/resources/stable-id-map.js: Added.
+        (TestPage.registerInitializer.window.StableIdMap):
+        (TestPage.registerInitializer.window.StableIdMap.prototype.get size):
+        (TestPage.registerInitializer.window.StableIdMap.prototype.get let):
+        (TestPage.registerInitializer):
+        * http/tests/inspector/target/provisional-load-cancels-previous-load.html:
+        * inspector/runtime/executionContextCreated-onEnable-expected.txt: Added.
+        * inspector/runtime/executionContextCreated-onEnable.html: Added.
+
 2019-12-18  youenn fablet  <youenn@apple.com>
 
         Add support for Audio Capture in GPUProcess
diff --git a/LayoutTests/http/tests/inspector/resources/stable-id-map.js b/LayoutTests/http/tests/inspector/resources/stable-id-map.js
new file mode 100644 (file)
index 0000000..8007289
--- /dev/null
@@ -0,0 +1,52 @@
+/*
+ * Copyright (C) 2019 Microsoft Corporation. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above
+ * copyright notice, this list of conditions and the following disclaimer
+ * in the documentation and/or other materials provided with the
+ * distribution.
+ *     * Neither the name of Microsoft Corporation nor the names of its
+ * contributors may be used to endorse or promote products derived from
+ * this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+ TestPage.registerInitializer(() => {
+    window.StableIdMap = class StableIdMap {
+        constructor()
+        {
+            this._idMap = new Map;
+        }
+
+        // Public
+
+        get size() { return this._idMap.size; }
+
+        get(unstableId)
+        {
+            let id = this._idMap.get(unstableId);
+            if (id === undefined) {
+                id = this.size;
+                this._idMap.set(unstableId, id);
+            }
+            return id;
+        }
+    }
+});
index dd07b5a..4af07a0 100644 (file)
@@ -3,15 +3,15 @@ Test that two consequtive cross domain navigation requests will result in two pr
 
 == Running test suite: Target.PSON
 -- Running test case: ProvisionalPagePaused
-Current target is 1.
+Current target is 0.
+PASS: Should receive TargetAdded event for target 1.
+PASS: Target 1 should be provisional.
+PASS: Target 1 should be paused on start.
+PASS: Should receive TargetRemoved event for target 1
 PASS: Should receive TargetAdded event for target 2.
 PASS: Target 2 should be provisional.
 PASS: Target 2 should be paused on start.
-PASS: Should receive TargetRemoved event for target 2
-PASS: Should receive TargetAdded event for target 3.
-PASS: Target 3 should be provisional.
-PASS: Target 3 should be paused on start.
-PASS: Should receive TargetRemoved event for target 1
-PASS: Should receive DidCommitProvisionalTarget event 1 => 3.
+PASS: Should receive TargetRemoved event for target 0
+PASS: Should receive DidCommitProvisionalTarget event 0 => 2.
 PASS: Should have seen 3 different targets.
 
index c96b023..2662622 100644 (file)
@@ -3,34 +3,24 @@
 <head>
 <meta charset="utf-8">
 <script src="../resources/inspector-test.js"></script>
+<script src="../resources/stable-id-map.js"></script>
 <script>
 function test()
 {
-    let lastAssignedId = 0;
-    let targetToId = new Map;
-    function stableTargetId(targetId)
-    {
-        let id = targetToId.get(targetId);
-        if (!id) {
-            id = ++lastAssignedId;
-            targetToId.set(targetId, id);
-        }
-        return id;
-    }
-
     let suite = InspectorTest.createAsyncSuite("Target.PSON");
 
     suite.addTestCase({
         name: "ProvisionalPagePaused",
         description: "Check that new provisional page can be paused before navigation.",
         test(resolve, reject) {
-            InspectorTest.log(`Current target is ${stableTargetId(WI.mainTarget.identifier)}.`);
+            let targetIdMap = new StableIdMap;
+            InspectorTest.log(`Current target is ${targetIdMap.get(WI.mainTarget.identifier)}.`);
             const url = "http://localhost:8000/inspector/target/provisional-load-cancels-previous-load.html";
             let navigatedTwice = false;
 
             WI.targetManager.addEventListener(WI.TargetManager.Event.TargetAdded, (event) => {
                 let target = event.data.target;
-                let targetId = stableTargetId(target.identifier);
+                let targetId = targetIdMap.get(target.identifier);
                 InspectorTest.pass(`Should receive TargetAdded event for target ${targetId}.`);
                 InspectorTest.expectTrue(target.isProvisional, `Target ${targetId} should be provisional.`);
                 InspectorTest.expectTrue(target.isPaused, `Target ${targetId} should be paused on start.`);
@@ -45,18 +35,18 @@ function test()
 
             WI.targetManager.addEventListener(WI.TargetManager.Event.DidCommitProvisionalTarget, (event) => {
                 let {previousTargetId, target} = event.data;
-                InspectorTest.pass(`Should receive DidCommitProvisionalTarget event ${stableTargetId(previousTargetId)} => ${stableTargetId(target.identifier)}.`);
+                InspectorTest.pass(`Should receive DidCommitProvisionalTarget event ${targetIdMap.get(previousTargetId)} => ${targetIdMap.get(target.identifier)}.`);
             });
 
             WI.targetManager.addEventListener(WI.TargetManager.Event.TargetRemoved, (event) =>{
-                let targetId = stableTargetId(event.data.target.identifier);
+                let targetId = targetIdMap.get(event.data.target.identifier);
                 InspectorTest.pass(`Should receive TargetRemoved event for target ${targetId}`);
             });
 
             // Wait for page reload event to avoid race between test results flushing and the test completion.
             InspectorTest.awaitEvent(FrontendTestHarness.Event.TestPageDidLoad)
             .then(() => {
-                InspectorTest.expectEqual(lastAssignedId, 3, `Should have seen 3 different targets.`);
+                InspectorTest.expectEqual(targetIdMap.size, 3, `Should have seen 3 different targets.`);
             })
             .then(resolve);
 
diff --git a/LayoutTests/inspector/runtime/executionContextCreated-onEnable-expected.txt b/LayoutTests/inspector/runtime/executionContextCreated-onEnable-expected.txt
new file mode 100644 (file)
index 0000000..dfc1684
--- /dev/null
@@ -0,0 +1,11 @@
+
+Test that exactly one Runtime.executionContextCreated event is fired for each existing context when Runtime.enable is called.
+
+
+== Running test suite: Runtime.executionContextCreated.OnEnable
+-- Running test case: Runtime.executionContextCreated.OnEnable.NoDuplicates
+Execution context created: id=0 frameId=0 isPageContext=true
+Execution context created: id=1 frameId=1 isPageContext=true
+PASS: Should receive 2 executionContextCreated events (for main world in the main frame and the subframe).
+PASS: Should have 2 unique contexts.
+
diff --git a/LayoutTests/inspector/runtime/executionContextCreated-onEnable.html b/LayoutTests/inspector/runtime/executionContextCreated-onEnable.html
new file mode 100644 (file)
index 0000000..ba5c93d
--- /dev/null
@@ -0,0 +1,42 @@
+<html>
+<head>
+<script src="../../http/tests/inspector/resources/protocol-test.js"></script>
+<script src="../../http/tests/inspector/resources/stable-id-map.js"></script>
+<script>
+function test()
+{
+    let suite = ProtocolTest.createAsyncSuite("Runtime.executionContextCreated.OnEnable");
+
+    suite.addTestCase({
+        name: "Runtime.executionContextCreated.OnEnable.NoDuplicates",
+        description: "Test that Runtime.enable will send executionContextCreated events only for existng main world contexts",
+        async test() {
+            let contextIdMap = new StableIdMap;
+            let frameIdMap = new StableIdMap;
+            let contextCount = 0;
+
+            InspectorProtocol.addEventListener("Runtime.executionContextCreated", (messageObject) => {
+                let {id, isPageContext, frameId} = messageObject.params.context;
+                ProtocolTest.log(`Execution context created: id=${contextIdMap.get(id)} frameId=${frameIdMap.get(frameId)} isPageContext=${isPageContext}`)
+                ++contextCount;
+            });
+
+            await Promise.all([
+                InspectorProtocol.awaitCommand({method: "Page.enable"}),
+                InspectorProtocol.awaitCommand({method: "Runtime.enable"}),
+            ]);
+
+            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.");
+        }
+    });
+
+    suite.runTestCasesAndFinish();
+}
+</script>
+</head>
+<body>
+<iframe id="subframe" src="resources/change-execution-context-identifier-subframe.html" onload="runTest()"></iframe>
+<p>Test that exactly one Runtime.executionContextCreated event is fired for each existing context when Runtime.enable is called.</p>
+</body>
+</html>
index 0430092..adf7a2e 100644 (file)
@@ -1,3 +1,18 @@
+2019-12-18  Yury Semikhatsky  <yurys@chromium.org>
+
+        Web Inspector: Runtime.enable reports duplicates (non existent) contexts
+        https://bugs.webkit.org/show_bug.cgi?id=204859
+
+        Reviewed by Devin Rousso.
+
+        Do not report main world context as non-main world one when Runtime.enable is called.
+
+        Test: inspector/runtime/executionContextCreated-onEnable.html
+
+        * inspector/agents/page/PageRuntimeAgent.cpp:
+        (WebCore::PageRuntimeAgent::enable):
+        (WebCore::PageRuntimeAgent::reportExecutionContextCreation):
+
 2019-12-18  Antti Koivisto  <antti@apple.com>
 
         Optimize Style::Invalidator for multiple RuleSet case
index b4b9d5c..864ce3c 100644 (file)
@@ -70,14 +70,18 @@ PageRuntimeAgent::~PageRuntimeAgent() = default;
 
 void PageRuntimeAgent::enable(ErrorString& errorString)
 {
-    bool enabled = m_instrumentingAgents.pageRuntimeAgent() == this;
+    if (m_instrumentingAgents.pageRuntimeAgent() == this)
+        return;
 
     InspectorRuntimeAgent::enable(errorString);
+    if (!errorString.isEmpty())
+        return;
 
-    m_instrumentingAgents.setPageRuntimeAgent(this);
+    // Report initial contexts before enabling instrumentation as the reporting
+    // can force creation of script state which could result in duplicate notifications.
+    reportExecutionContextCreation();
 
-    if (!enabled)
-        reportExecutionContextCreation();
+    m_instrumentingAgents.setPageRuntimeAgent(this);
 }
 
 void PageRuntimeAgent::disable(ErrorString& errorString)
@@ -148,8 +152,10 @@ void PageRuntimeAgent::reportExecutionContextCreation()
         frame->script().collectIsolatedContexts(isolatedContexts);
         if (isolatedContexts.isEmpty())
             continue;
-        for (auto& context : isolatedContexts)
-            notifyContextCreated(frameId, context.first, context.second, false);
+        for (auto& [globalObject, securityOrigin] : isolatedContexts) {
+            if (globalObject != scriptState)
+                notifyContextCreated(frameId, globalObject, securityOrigin, false);
+        }
         isolatedContexts.clear();
     }
 }
index 5891feb..54c3536 100644 (file)
@@ -1,3 +1,15 @@
+2019-12-18  Yury Semikhatsky  <yurys@chromium.org>
+
+        Web Inspector: Runtime.enable reports duplicates (non existent) contexts
+        https://bugs.webkit.org/show_bug.cgi?id=204859
+
+        Reviewed by Devin Rousso.
+
+        Assert that all contexts added to the list are unique.
+
+        * UserInterface/Models/ExecutionContextList.js:
+        (WI.ExecutionContextList.prototype.add):
+
 2019-12-18  Devin Rousso  <drousso@apple.com>
 
         Web Inspector: Elements: remove the "Show/Hide Shadow DOM" navigation item
index b2b9aa0..63d9573 100644 (file)
@@ -45,7 +45,8 @@ WI.ExecutionContextList = class ExecutionContextList
 
     add(context)
     {
-        // FIXME: The backend sends duplicate page context execution contexts with the same id. Why?
+        // COMPATIBILITY (iOS 13.0): Older iOS releases will send duplicates.
+        // Newer releases will not and this check should be removed eventually.
         if (context.isPageContext && this._pageExecutionContext) {
             console.assert(context.id === this._pageExecutionContext.id);
             return false;