Web Inspector: Regression(r266885) Crash sometimes when rehydrating i…

…mported audit results

https://bugs.webkit.org/show_bug.cgi?id=240366

Reviewed by Devin Rousso.

Source/JavaScriptCore:

* inspector/protocol/DOM.json:

Source/WebCore:

Added test cases to inspector/model/dom-node.html

After r266885, there is no path to handle the possibility that there may not be a resulting node for calls to
`DOM.querySelector`. To correct this, mark the return value as optional (Web Inspector frontend already treats
it as optional) and return early if there was no Element matching the selector.

* inspector/agents/InspectorDOMAgent.cpp:
(WebCore::InspectorDOMAgent::querySelector):
* inspector/agents/InspectorDOMAgent.h:

LayoutTests:

* inspector/model/dom-node.html:
* inspector/model/dom-node-expected.txt:

Canonical link: https://commits.webkit.org/250592@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@294234 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog

@@ -1,3 +1,13 @@
+2022-05-16  Patrick Angle  <pangle@apple.com>
+
+        Web Inspector: Regression(r266885) Crash sometimes when rehydrating imported audit results
+        https://bugs.webkit.org/show_bug.cgi?id=240366
+
+        Reviewed by Devin Rousso.
+
+        * inspector/model/dom-node.html:
+        * inspector/model/dom-node-expected.txt:
+
 2022-05-16  Rob Buis  <rbuis@igalia.com>
 
         Remove some custom-element tests

LayoutTests/inspector/model/dom-node-expected.txt

@@ -7,3 +7,11 @@ id="test-id"
 class="test-class"
 data-item="test-data"
 
+-- Running test case: WI.DOMNode.querySelector
+Calling querySelector("#test-id") on document node.
+PASS: `querySelector("#test-id")` should return a WI.DOMNode
+Calling querySelector("#non-existent-id") on document node.
+PASS: `querySelector("#non-existent-id")` should return null.
+Calling querySelector("^\_(invalid selector)_/^") on document node.
+PASS: `querySelector` with an invalid selector should throw a SyntaxError.
+

LayoutTests/inspector/model/dom-node.html

@@ -25,6 +25,34 @@
         }
     });
 
+    suite.addTestCase({
+        name: "WI.DOMNode.querySelector",
+        description: "Test getting a child node via querySelector.",
+        async test() {
+            let documentNode = await WI.domManager.requestDocument();
+
+            function querySelector(selector) {
+                InspectorTest.log(`Calling querySelector("${selector}") on document node.`);
+                return documentNode.querySelector(selector).then((nodeId) => {
+                    if (!nodeId)
+                        return null;
+                    return WI.domManager.nodeForId(nodeId);
+                });
+            }
+
+            let nodeFromQueryingExistingId = await querySelector("#test-id");
+            InspectorTest.expectThat(nodeFromQueryingExistingId instanceof WI.DOMNode, "`querySelector(\"#test-id\")` should return a WI.DOMNode");
+
+            let nodeFromQueryingNonExistantId = await querySelector("#non-existent-id");
+            InspectorTest.expectNull(nodeFromQueryingNonExistantId, "`querySelector(\"#non-existent-id\")` should return null.");
+
+            await querySelector("^\\_(invalid selector)_/^").catch((error) => {
+                InspectorTest.expectEqual(error.message, "SyntaxError", "`querySelector` with an invalid selector should throw a SyntaxError.");
+            });
+
+        }
+    });
+
     suite.runTestCasesAndFinish();
 }
 </script>

Source/JavaScriptCore/ChangeLog

@@ -1,3 +1,12 @@
+2022-05-16  Patrick Angle  <pangle@apple.com>
+
+        Web Inspector: Regression(r266885) Crash sometimes when rehydrating imported audit results
+        https://bugs.webkit.org/show_bug.cgi?id=240366
+
+        Reviewed by Devin Rousso.
+
+        * inspector/protocol/DOM.json:
+
 2022-05-13  Mark Lam  <mark.lam@apple.com>
 
         Enhance the ARM64Disassembler to print pc indices and better branch target labels.

Source/JavaScriptCore/inspector/protocol/DOM.json

@@ -204,7 +204,7 @@
                 { "name": "selector", "type": "string", "description": "Selector string." }
             ],
             "returns": [
-                { "name": "nodeId", "$ref": "NodeId", "description": "Query selector result." }
+                { "name": "nodeId", "$ref": "NodeId", "optional": true, "description": "Query selector result." }
             ]
         },
         {

Source/WebCore/ChangeLog

@@ -1,3 +1,20 @@
+2022-05-16  Patrick Angle  <pangle@apple.com>
+
+        Web Inspector: Regression(r266885) Crash sometimes when rehydrating imported audit results
+        https://bugs.webkit.org/show_bug.cgi?id=240366
+
+        Reviewed by Devin Rousso.
+
+        Added test cases to inspector/model/dom-node.html
+
+        After r266885, there is no path to handle the possibility that there may not be a resulting node for calls to
+        `DOM.querySelector`. To correct this, mark the return value as optional (Web Inspector frontend already treats
+        it as optional) and return early if there was no Element matching the selector.
+
+        * inspector/agents/InspectorDOMAgent.cpp:
+        (WebCore::InspectorDOMAgent::querySelector):
+        * inspector/agents/InspectorDOMAgent.h:
+
 2022-05-16  Alan Bujtas  <zalan@apple.com>
 
         [LFC][FFC] Add "flex-direction: row-reverse" basic visual/logical conversion

Source/WebCore/inspector/agents/InspectorDOMAgent.cpp

@@ -592,7 +592,7 @@ Protocol::ErrorStringOr<void> InspectorDOMAgent::requestChildNodes(Protocol::DOM
     return { };
 }
 
-Protocol::ErrorStringOr<Protocol::DOM::NodeId> InspectorDOMAgent::querySelector(Protocol::DOM::NodeId nodeId, const String& selector)
+Protocol::ErrorStringOr<std::optional<Protocol::DOM::NodeId>> InspectorDOMAgent::querySelector(Protocol::DOM::NodeId nodeId, const String& selector)
 {
     Protocol::ErrorString errorString;
 
@@ -606,11 +606,15 @@ Protocol::ErrorStringOr<Protocol::DOM::NodeId> InspectorDOMAgent::querySelector(
     if (queryResult.hasException())
         return makeUnexpected(InspectorDOMAgent::toErrorString(queryResult.releaseException()));
 
-    auto resultNodeId = pushNodePathToFrontend(errorString, queryResult.releaseReturnValue());
+    auto* queryResultNode = queryResult.releaseReturnValue();
+    if (!queryResultNode)
+        return { };
+
+    auto resultNodeId = pushNodePathToFrontend(errorString, queryResultNode);
     if (!resultNodeId)
         return makeUnexpected(errorString);
 
-    return resultNodeId;
+    return { resultNodeId };
 }
 
 Protocol::ErrorStringOr<Ref<JSON::ArrayOf<Protocol::DOM::NodeId>>> InspectorDOMAgent::querySelectorAll(Protocol::DOM::NodeId nodeId, const String& selector)

Source/WebCore/inspector/agents/InspectorDOMAgent.h

@@ -106,7 +106,7 @@ class InspectorDOMAgent final : public InspectorAgentBase, public Inspector::DOM
     void willDestroyFrontendAndBackend(Inspector::DisconnectReason);
 
     // DOMBackendDispatcherHandler
-    Inspector::Protocol::ErrorStringOr<Inspector::Protocol::DOM::NodeId> querySelector(Inspector::Protocol::DOM::NodeId, const String& selector);
+    Inspector::Protocol::ErrorStringOr<std::optional<Inspector::Protocol::DOM::NodeId>> querySelector(Inspector::Protocol::DOM::NodeId, const String& selector);
     Inspector::Protocol::ErrorStringOr<Ref<JSON::ArrayOf<Inspector::Protocol::DOM::NodeId>>> querySelectorAll(Inspector::Protocol::DOM::NodeId, const String& selector);
     Inspector::Protocol::ErrorStringOr<Ref<Inspector::Protocol::DOM::Node>> getDocument();
     Inspector::Protocol::ErrorStringOr<void> requestChildNodes(Inspector::Protocol::DOM::NodeId, std::optional<int>&& depth);