Whitelist side-effect-free CommandLineAPI methods

Marks methods on the Command Line API that produce no JS-observable side-effect. Bug: 829571 Change-Id: I7920b2ded3eada33fc2d000aa46a0a7eef61daca Reviewed-on: https://chromium-review.googlesource.com/1025171 Reviewed-by: Dmitry Gozman <dgozman@chromium.org> Reviewed-by: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org> Commit-Queue: Erik Luo <luoe@chromium.org> Cr-Commit-Position: refs/heads/master@{#553270}
marcosholgado · Apr 24, 2018 · 18d7dcf · 18d7dcf
1 parent da37423
commit 18d7dcf
Show file tree

Hide file tree

Showing 7 changed files with 268 additions and 32 deletions.
diff --git a/...nspector-protocol/side-effects/evaluate-embedder-side-effect-free-attributes-expected.txt b/...nspector-protocol/side-effects/evaluate-embedder-side-effect-free-attributes-expected.txt
@@ -1,2 +1,130 @@
 Tests that evaluating V8-embedder callbacks allows side-effect-free attribute getters. Should not crash.
+Expression `document.title = "foo"`
+has side effect: true, expected: true
+Expression `document.domain`
+has side effect: false, expected: false
+Expression `document.referrer`
+has side effect: false, expected: false
+Expression `document.cookie`
+has side effect: false, expected: false
+Expression `document.title`
+has side effect: false, expected: false
+Expression `document.documentElement`
+has side effect: false, expected: false
+Expression `document.scrollingElement`
+has side effect: false, expected: false
+Expression `document.body`
+has side effect: false, expected: false
+Expression `document.head`
+has side effect: false, expected: false
+Expression `document.activeElement`
+has side effect: false, expected: false
+Expression `div.tagName`
+has side effect: false, expected: false
+Expression `div.id`
+has side effect: false, expected: false
+Expression `div.className`
+has side effect: false, expected: false
+Expression `div.nodeType`
+has side effect: false, expected: false
+Expression `div.nodeName`
+has side effect: false, expected: false
+Expression `div.nodeValue`
+has side effect: false, expected: false
+Expression `div.textContent`
+has side effect: false, expected: false
+Expression `div.isConnected`
+has side effect: false, expected: false
+Expression `div.parentNode`
+has side effect: false, expected: false
+Expression `div.parentElement`
+has side effect: false, expected: false
+Expression `div.childNodes`
+has side effect: false, expected: false
+Expression `div.firstChild`
+has side effect: false, expected: false
+Expression `div.lastChild`
+has side effect: false, expected: false
+Expression `div.previousSibling`
+has side effect: false, expected: false
+Expression `div.nextSibling`
+has side effect: false, expected: false
+Expression `document.nodeType`
+has side effect: false, expected: false
+Expression `document.nodeName`
+has side effect: false, expected: false
+Expression `document.nodeValue`
+has side effect: false, expected: false
+Expression `document.textContent`
+has side effect: false, expected: false
+Expression `document.isConnected`
+has side effect: false, expected: false
+Expression `document.parentNode`
+has side effect: false, expected: false
+Expression `document.parentElement`
+has side effect: false, expected: false
+Expression `document.childNodes`
+has side effect: false, expected: false
+Expression `document.firstChild`
+has side effect: false, expected: false
+Expression `document.lastChild`
+has side effect: false, expected: false
+Expression `document.previousSibling`
+has side effect: false, expected: false
+Expression `document.nextSibling`
+has side effect: false, expected: false
+Expression `textNode.nodeType`
+has side effect: false, expected: false
+Expression `textNode.nodeName`
+has side effect: false, expected: false
+Expression `textNode.nodeValue`
+has side effect: false, expected: false
+Expression `textNode.textContent`
+has side effect: false, expected: false
+Expression `textNode.isConnected`
+has side effect: false, expected: false
+Expression `textNode.parentNode`
+has side effect: false, expected: false
+Expression `textNode.parentElement`
+has side effect: false, expected: false
+Expression `textNode.childNodes`
+has side effect: false, expected: false
+Expression `textNode.firstChild`
+has side effect: false, expected: false
+Expression `textNode.lastChild`
+has side effect: false, expected: false
+Expression `textNode.previousSibling`
+has side effect: false, expected: false
+Expression `textNode.nextSibling`
+has side effect: false, expected: false
+Expression `div.childElementCount`
+has side effect: false, expected: false
+Expression `div.children`
+has side effect: false, expected: false
+Expression `div.firstElementChild`
+has side effect: false, expected: false
+Expression `div.lastElementChild`
+has side effect: false, expected: false
+Expression `document.childElementCount`
+has side effect: false, expected: false
+Expression `document.children`
+has side effect: false, expected: false
+Expression `document.firstElementChild`
+has side effect: false, expected: false
+Expression `document.lastElementChild`
+has side effect: false, expected: false
+Expression `textNode.childElementCount`
+has side effect: false, expected: false
+Expression `textNode.children`
+has side effect: false, expected: false
+Expression `textNode.firstElementChild`
+has side effect: false, expected: false
+Expression `textNode.lastElementChild`
+has side effect: false, expected: false
+Expression `devicePixelRatio`
+has side effect: false, expected: false
+Expression `screenX`
+has side effect: false, expected: false
+Expression `screenY`
+has side effect: false, expected: false
 
diff --git a/...tp/tests/inspector-protocol/side-effects/evaluate-embedder-side-effect-free-attributes.js b/...tp/tests/inspector-protocol/side-effects/evaluate-embedder-side-effect-free-attributes.js
@@ -81,10 +81,7 @@
     if (exceptionDetails &&
         exceptionDetails.exception.description.startsWith('EvalError: Possible side-effect in debug-evaluate'))
       hasSideEffect = true;
-    if (hasSideEffect !== expectSideEffect) {
-      testRunner.log(`FAIL: "${expression}" hasSideEffect = ${hasSideEffect}, expectSideEffect = ${expectSideEffect}`);
-      testRunner.completeTest();
-      return;
-    }
+    const failed = (hasSideEffect !== expectSideEffect);
+    testRunner.log(`${failed ? 'FAIL: ' : ''}Expression \`${expression}\`\nhas side effect: ${hasSideEffect}, expected: ${expectSideEffect}`);
   }
 })
diff --git a/...s/inspector-protocol/side-effects/evaluate-embedder-side-effect-free-methods-expected.txt b/...s/inspector-protocol/side-effects/evaluate-embedder-side-effect-free-methods-expected.txt
@@ -1,2 +1,88 @@
 Tests that evaluating V8-embedder callbacks allows side-effect-free methods. Should not crash.
+Expression `document.querySelector('div').x = "foo"`
+has side effect: true, expected: true
+Expression `$('div')`
+has side effect: false, expected: false
+Expression `$$('div')`
+has side effect: false, expected: false
+Expression `$x('//div')`
+has side effect: false, expected: false
+Expression `getEventListeners(document)`
+has side effect: false, expected: false
+Expression `$.toString()`
+has side effect: false, expected: false
+Expression `$$.toString()`
+has side effect: false, expected: false
+Expression `$x.toString()`
+has side effect: false, expected: false
+Expression `getEventListeners.toString()`
+has side effect: false, expected: false
+Expression `monitorEvents()`
+has side effect: true, expected: true
+Expression `unmonitorEvents()`
+has side effect: true, expected: true
+Expression `monitorEvents.toString()`
+has side effect: false, expected: false
+Expression `unmonitorEvents.toString()`
+has side effect: false, expected: false
+Expression `document.getElementsByTagName('div')`
+has side effect: false, expected: false
+Expression `document.getElementsByTagNameNS('http://www.w3.org/1999/xhtml', 'div')`
+has side effect: false, expected: false
+Expression `document.getElementsByClassName('foo')`
+has side effect: false, expected: false
+Expression `document.getElementsByName('div-name')`
+has side effect: false, expected: false
+Expression `div.getAttributeNames()`
+has side effect: false, expected: false
+Expression `divNoAttrs.getAttributeNames()`
+has side effect: false, expected: false
+Expression `div.getAttribute()`
+has side effect: false, expected: false
+Expression `div.getAttribute('attr1')`
+has side effect: false, expected: false
+Expression `div.getAttribute({})`
+has side effect: false, expected: false
+Expression `divNoAttrs.getAttribute('attr1')`
+has side effect: false, expected: false
+Expression `div.hasAttribute()`
+has side effect: false, expected: false
+Expression `div.hasAttribute('attr1')`
+has side effect: false, expected: false
+Expression `div.hasAttribute({})`
+has side effect: false, expected: false
+Expression `divNoAttrs.hasAttribute('attr1')`
+has side effect: false, expected: false
+Expression `div.contains(textNode)`
+has side effect: false, expected: false
+Expression `div.contains()`
+has side effect: false, expected: false
+Expression `div.contains({})`
+has side effect: false, expected: false
+Expression `div.querySelector('div')`
+has side effect: false, expected: false
+Expression `div.querySelectorAll('div')`
+has side effect: false, expected: false
+Expression `document.contains(textNode)`
+has side effect: false, expected: false
+Expression `document.contains()`
+has side effect: false, expected: false
+Expression `document.contains({})`
+has side effect: false, expected: false
+Expression `document.querySelector('div')`
+has side effect: false, expected: false
+Expression `document.querySelectorAll('div')`
+has side effect: false, expected: false
+Expression `textNode.contains(textNode)`
+has side effect: false, expected: false
+Expression `textNode.contains()`
+has side effect: false, expected: false
+Expression `textNode.contains({})`
+has side effect: false, expected: false
+Expression `textNode.querySelector('div')`
+has side effect: false, expected: false
+Expression `textNode.querySelectorAll('div')`
+has side effect: false, expected: false
+Expression `global_performance.now()`
+has side effect: false, expected: false
 
diff --git a/.../http/tests/inspector-protocol/side-effects/evaluate-embedder-side-effect-free-methods.js b/.../http/tests/inspector-protocol/side-effects/evaluate-embedder-side-effect-free-methods.js
@@ -27,6 +27,22 @@
   // Sanity check: test that setters are not allowed on whitelisted methods.
   await checkHasSideEffect(`document.querySelector('div').x = "foo"`);
 
+  // Command Line API
+  await checkHasNoSideEffect(`$('div')`);
+  await checkHasNoSideEffect(`$$('div')`);
+  await checkHasNoSideEffect(`$x('//div')`);
+  await checkHasNoSideEffect(`getEventListeners(document)`);
+  await checkHasNoSideEffect(`$.toString()`);
+  await checkHasNoSideEffect(`$$.toString()`);
+  await checkHasNoSideEffect(`$x.toString()`);
+  await checkHasNoSideEffect(`getEventListeners.toString()`);
+
+  // Unsafe Command Line API
+  await checkHasSideEffect(`monitorEvents()`);
+  await checkHasSideEffect(`unmonitorEvents()`);
+  await checkHasNoSideEffect(`monitorEvents.toString()`);
+  await checkHasNoSideEffect(`unmonitorEvents.toString()`);
+
   // Document
   await checkHasNoSideEffect(`document.getElementsByTagName('div')`);
   await checkHasNoSideEffect(`document.getElementsByTagNameNS('http://www.w3.org/1999/xhtml', 'div')`);
@@ -70,16 +86,13 @@
   }
 
   async function checkExpression(expression, expectSideEffect) {
-    var response = await dp.Runtime.evaluate({expression, throwOnSideEffect: true});
+    var response = await dp.Runtime.evaluate({expression, throwOnSideEffect: true, includeCommandLineAPI: true});
     var hasSideEffect = false;
     var exceptionDetails = response.result.exceptionDetails;
     if (exceptionDetails &&
         exceptionDetails.exception.description.startsWith('EvalError: Possible side-effect in debug-evaluate'))
       hasSideEffect = true;
-    if (hasSideEffect !== expectSideEffect) {
-      testRunner.log(`FAIL: ${expression} hasSideEffect = ${hasSideEffect}, expectSideEffect = ${expectSideEffect}`);
-      testRunner.completeTest();
-      return;
-    }
+    const failed = (hasSideEffect !== expectSideEffect);
+    testRunner.log(`${failed ? 'FAIL: ' : ''}Expression \`${expression}\`\nhas side effect: ${hasSideEffect}, expected: ${expectSideEffect}`);
   }
 })
diff --git a/third_party/blink/renderer/core/inspector/main_thread_debugger.cc b/third_party/blink/renderer/core/inspector/main_thread_debugger.cc
@@ -340,13 +340,16 @@ void MainThreadDebugger::installAdditionalCommandLineAPI(
   ThreadDebugger::installAdditionalCommandLineAPI(context, object);
   CreateFunctionProperty(
       context, object, "$", MainThreadDebugger::QuerySelectorCallback,
-      "function $(selector, [startNode]) { [Command Line API] }");
+      "function $(selector, [startNode]) { [Command Line API] }",
+      v8::SideEffectType::kHasNoSideEffect);
   CreateFunctionProperty(
       context, object, "$$", MainThreadDebugger::QuerySelectorAllCallback,
-      "function $$(selector, [startNode]) { [Command Line API] }");
+      "function $$(selector, [startNode]) { [Command Line API] }",
+      v8::SideEffectType::kHasNoSideEffect);
   CreateFunctionProperty(
       context, object, "$x", MainThreadDebugger::XpathSelectorCallback,
-      "function $x(xpath, [startNode]) { [Command Line API] }");
+      "function $x(xpath, [startNode]) { [Command Line API] }",
+      v8::SideEffectType::kHasNoSideEffect);
 }
 
 static Node* SecondArgumentAsNode(

diff --git a/third_party/blink/renderer/core/inspector/thread_debugger.cc b/third_party/blink/renderer/core/inspector/thread_debugger.cc
@@ -219,24 +219,27 @@ static v8::Maybe<bool> CreateDataProperty(v8::Local<v8::Context> context,
   return object->CreateDataProperty(context, key, value);
 }
 
-static void CreateFunctionPropertyWithData(v8::Local<v8::Context> context,
-                                           v8::Local<v8::Object> object,
-                                           const char* name,
-                                           v8::FunctionCallback callback,
-                                           v8::Local<v8::Value> data,
-                                           const char* description) {
+static void CreateFunctionPropertyWithData(
+    v8::Local<v8::Context> context,
+    v8::Local<v8::Object> object,
+    const char* name,
+    v8::FunctionCallback callback,
+    v8::Local<v8::Value> data,
+    const char* description,
+    v8::SideEffectType side_effect_type) {
   v8::Local<v8::String> func_name = V8String(context->GetIsolate(), name);
   v8::Local<v8::Function> func;
   if (!v8::Function::New(context, callback, data, 0,
-                         v8::ConstructorBehavior::kThrow)
+                         v8::ConstructorBehavior::kThrow, side_effect_type)
            .ToLocal(&func))
     return;
   func->SetName(func_name);
   v8::Local<v8::String> return_value =
       V8String(context->GetIsolate(), description);
   v8::Local<v8::Function> to_string_function;
   if (v8::Function::New(context, ReturnDataCallback, return_value, 0,
-                        v8::ConstructorBehavior::kThrow)
+                        v8::ConstructorBehavior::kThrow,
+                        v8::SideEffectType::kHasNoSideEffect)
           .ToLocal(&to_string_function))
     CreateDataProperty(context, func,
                        V8AtomicString(context->GetIsolate(), "toString"),
@@ -256,14 +259,16 @@ v8::Maybe<bool> ThreadDebugger::CreateDataPropertyInArray(
   return array->CreateDataProperty(context, index, value);
 }
 
-void ThreadDebugger::CreateFunctionProperty(v8::Local<v8::Context> context,
-                                            v8::Local<v8::Object> object,
-                                            const char* name,
-                                            v8::FunctionCallback callback,
-                                            const char* description) {
+void ThreadDebugger::CreateFunctionProperty(
+    v8::Local<v8::Context> context,
+    v8::Local<v8::Object> object,
+    const char* name,
+    v8::FunctionCallback callback,
+    const char* description,
+    v8::SideEffectType side_effect_type) {
   CreateFunctionPropertyWithData(context, object, name, callback,
                                  v8::External::New(context->GetIsolate(), this),
-                                 description);
+                                 description, side_effect_type);
 }
 
 void ThreadDebugger::installAdditionalCommandLineAPI(
@@ -272,7 +277,8 @@ void ThreadDebugger::installAdditionalCommandLineAPI(
   CreateFunctionProperty(
       context, object, "getEventListeners",
       ThreadDebugger::GetEventListenersCallback,
-      "function getEventListeners(node) { [Command Line API] }");
+      "function getEventListeners(node) { [Command Line API] }",
+      v8::SideEffectType::kHasNoSideEffect);
 
   v8::Local<v8::Value> function_value;
   bool success =
@@ -287,11 +293,13 @@ void ThreadDebugger::installAdditionalCommandLineAPI(
   CreateFunctionPropertyWithData(
       context, object, "monitorEvents", ThreadDebugger::MonitorEventsCallback,
       function_value,
-      "function monitorEvents(object, [types]) { [Command Line API] }");
+      "function monitorEvents(object, [types]) { [Command Line API] }",
+      v8::SideEffectType::kHasSideEffect);
   CreateFunctionPropertyWithData(
       context, object, "unmonitorEvents",
       ThreadDebugger::UnmonitorEventsCallback, function_value,
-      "function unmonitorEvents(object, [types]) { [Command Line API] }");
+      "function unmonitorEvents(object, [types]) { [Command Line API] }",
+      v8::SideEffectType::kHasSideEffect);
 }
 
 static Vector<String> NormalizeEventTypes(

diff --git a/third_party/blink/renderer/core/inspector/thread_debugger.h b/third_party/blink/renderer/core/inspector/thread_debugger.h
@@ -69,7 +69,8 @@ class CORE_EXPORT ThreadDebugger : public v8_inspector::V8InspectorClient,
                               v8::Local<v8::Object>,
                               const char* name,
                               v8::FunctionCallback,
-                              const char* description);
+                              const char* description,
+                              v8::SideEffectType side_effect_type);
   static v8::Maybe<bool> CreateDataPropertyInArray(v8::Local<v8::Context>,
                                                    v8::Local<v8::Array>,
                                                    int index,