Whitelist side-effect-free, lazy property IDL callbacks

Marks common callbacks that produce no JS-observable side-effect,
including
- window.window
- window.location
- location.href
- navigator.userAgent

Bug: 829571
Change-Id: I9404104dc2cd30ffeafbfae83c53c0176a28e1b1
Reviewed-on: https://chromium-review.googlesource.com/1026991
Commit-Queue: Erik Luo <luoe@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553788}

third_party/WebKit/LayoutTests/http/tests/inspector-protocol/side-effects/evaluate-embedder-side-effect-free-attributes-expected.txt

@@ -45,6 +45,10 @@ Expression `div.tabIndex`
 has side effect: false, expected: false
 Expression `div.style`
 has side effect: false, expected: false
+Expression `location.href`
+has side effect: false, expected: false
+Expression `navigator.userAgent`
+has side effect: false, expected: false
 Expression `div.nodeType`
 has side effect: false, expected: false
 Expression `div.nodeName`
@@ -162,7 +166,7 @@ has side effect: false, expected: false
 Expression `performance`
 has side effect: false, expected: false
 Expression `window`
-has side effect: true, expected: true
-Expression `window.location`
-has side effect: true, expected: true
+has side effect: false, expected: false
+Expression `location`
+has side effect: false, expected: false
 

third_party/WebKit/LayoutTests/http/tests/inspector-protocol/side-effects/evaluate-embedder-side-effect-free-attributes.js

@@ -54,6 +54,12 @@
   await checkHasNoSideEffect(`div.tabIndex`);
   await checkHasNoSideEffect(`div.style`);
 
+  // Location
+  await checkHasNoSideEffect(`location.href`);
+
+  // Navigator
+  await checkHasNoSideEffect(`navigator.userAgent`);
+
   // Node
   var testNodes = ['div', 'document', 'textNode'];
   for (var node of testNodes) {
@@ -88,10 +94,8 @@
   await checkHasNoSideEffect(`history`);
   await checkHasNoSideEffect(`navigator`);
   await checkHasNoSideEffect(`performance`);
-
-  // TODO(luoe): add support for LazyData properties.
-  await checkHasSideEffect(`window`);
-  await checkHasSideEffect(`window.location`);
+  await checkHasNoSideEffect(`window`);
+  await checkHasNoSideEffect(`location`);
 
   testRunner.completeTest();
 

third_party/blink/renderer/bindings/core/v8/v8_dom_configuration.cc

@@ -65,22 +65,26 @@ void InstallAttributeInternal(
   v8::AccessorNameGetterCallback getter = attribute.getter;
   v8::AccessorNameSetterCallback setter = attribute.setter;
 
-  DCHECK_EQ(attribute.getter_side_effect_type,
-            V8DOMConfiguration::kHasSideEffect);
+  v8::SideEffectType getter_side_effect_type =
+      attribute.getter_side_effect_type == V8DOMConfiguration::kHasNoSideEffect
+          ? v8::SideEffectType::kHasNoSideEffect
+          : v8::SideEffectType::kHasSideEffect;
   DCHECK(attribute.property_location_configuration);
   if (attribute.property_location_configuration &
       V8DOMConfiguration::kOnInstance) {
     instance_template->SetNativeDataProperty(
         name, getter, setter, v8::Local<v8::Value>(),
         static_cast<v8::PropertyAttribute>(attribute.attribute),
-        v8::Local<v8::AccessorSignature>());
+        v8::Local<v8::AccessorSignature>(), v8::AccessControl::DEFAULT,
+        getter_side_effect_type);
   }
   if (attribute.property_location_configuration &
       V8DOMConfiguration::kOnPrototype) {
     prototype_template->SetNativeDataProperty(
         name, getter, setter, v8::Local<v8::Value>(),
         static_cast<v8::PropertyAttribute>(attribute.attribute),
-        v8::Local<v8::AccessorSignature>());
+        v8::Local<v8::AccessorSignature>(), v8::AccessControl::DEFAULT,
+        getter_side_effect_type);
   }
   if (attribute.property_location_configuration &
       V8DOMConfiguration::kOnInterface)
@@ -105,18 +109,23 @@ void InstallAttributeInternal(
   const unsigned location = config.property_location_configuration;
   DCHECK(location);
 
-  DCHECK_EQ(config.getter_side_effect_type, V8DOMConfiguration::kHasSideEffect);
+  v8::SideEffectType getter_side_effect_type =
+      config.getter_side_effect_type == V8DOMConfiguration::kHasNoSideEffect
+          ? v8::SideEffectType::kHasNoSideEffect
+          : v8::SideEffectType::kHasSideEffect;
   v8::Local<v8::Context> context = isolate->GetCurrentContext();
   if (location & V8DOMConfiguration::kOnInstance && !instance.IsEmpty()) {
     instance
         ->SetNativeDataProperty(context, name, getter, setter,
-                                v8::Local<v8::Value>(), attribute)
+                                v8::Local<v8::Value>(), attribute,
+                                getter_side_effect_type)
         .ToChecked();
   }
   if (location & V8DOMConfiguration::kOnPrototype && !prototype.IsEmpty()) {
     prototype
         ->SetNativeDataProperty(context, name, getter, setter,
-                                v8::Local<v8::Value>(), attribute)
+                                v8::Local<v8::Value>(), attribute,
+                                getter_side_effect_type)
         .ToChecked();
   }
   if (location & V8DOMConfiguration::kOnInterface)
@@ -134,20 +143,24 @@ void InstallLazyDataAttributeInternal(
   DCHECK(!attribute.setter);
   DCHECK_EQ(attribute.world_configuration, V8DOMConfiguration::kAllWorlds);
 
-  DCHECK_EQ(attribute.getter_side_effect_type,
-            V8DOMConfiguration::kHasSideEffect);
+  v8::SideEffectType getter_side_effect_type =
+      attribute.getter_side_effect_type == V8DOMConfiguration::kHasNoSideEffect
+          ? v8::SideEffectType::kHasNoSideEffect
+          : v8::SideEffectType::kHasSideEffect;
   DCHECK(attribute.property_location_configuration);
   if (attribute.property_location_configuration &
       V8DOMConfiguration::kOnInstance) {
     instance_template->SetLazyDataProperty(
         name, getter, v8::Local<v8::Value>(),
-        static_cast<v8::PropertyAttribute>(attribute.attribute));
+        static_cast<v8::PropertyAttribute>(attribute.attribute),
+        getter_side_effect_type);
   }
   if (attribute.property_location_configuration &
       V8DOMConfiguration::kOnPrototype) {
     prototype_template->SetLazyDataProperty(
         name, getter, v8::Local<v8::Value>(),
-        static_cast<v8::PropertyAttribute>(attribute.attribute));
+        static_cast<v8::PropertyAttribute>(attribute.attribute),
+        getter_side_effect_type);
   }
   if (attribute.property_location_configuration &
       V8DOMConfiguration::kOnInterface)

third_party/blink/renderer/core/frame/location.idl

@@ -55,7 +55,7 @@
     // TODO(foolip): |ancestorOrigins| should have [Unforgeable, SameObject].
     readonly attribute DOMStringList ancestorOrigins;
 
-    [SetterCallWith=(CurrentWindow,EnteredWindow), CrossOrigin=Setter, RaisesException=Setter] attribute URLString href;
+    [Affects=Nothing, SetterCallWith=(CurrentWindow,EnteredWindow), CrossOrigin=Setter, RaisesException=Setter] attribute URLString href;
     // TODO(yukishiino): Use [Unforgeable] stringifier instead of toString.
     DOMString toString();
     [MeasureAs=LocationOrigin] readonly attribute USVString origin;

third_party/blink/renderer/core/frame/navigator_id.idl

@@ -41,5 +41,5 @@
     readonly attribute DOMString product; // constant "Gecko"
     // https://www.w3.org/Bugs/Public/show_bug.cgi?id=22555
     // boolean taintEnabled(); // constant false
-    readonly attribute DOMString userAgent;
+    [Affects=Nothing] readonly attribute DOMString userAgent;
 };

third_party/blink/renderer/core/frame/window.idl

@@ -34,12 +34,12 @@
 ] interface Window : EventTarget {
     // the current browsing context
     // FIXME: The spec uses the WindowProxy type for this and many other attributes.
-    [Unforgeable, CrossOrigin] readonly attribute Window window;
+    [Affects=Nothing, Unforgeable, CrossOrigin] readonly attribute Window window;
     [Replaceable, CrossOrigin] readonly attribute Window self;
     [Affects=Nothing, Unforgeable, CachedAccessor] readonly attribute Document document;
     [Replaceable] readonly attribute DOMString origin;
     attribute DOMString name;
-    [PutForwards=href, Unforgeable, CrossOrigin=(Getter,Setter), Custom=Getter] readonly attribute Location location;
+    [Affects=Nothing, PutForwards=href, Unforgeable, CrossOrigin=(Getter,Setter), Custom=Getter] readonly attribute Location location;
     [Affects=Nothing] readonly attribute History history;
     [Replaceable, MeasureAs=BarPropLocationbar] readonly attribute BarProp locationbar;
     [Replaceable, MeasureAs=BarPropMenubar] readonly attribute BarProp menubar;