Issue 7236 - Handle sub properties in abort-on-property snippets

lib/content/snippets.js

Index: lib/content/snippets.js
===================================================================
--- a/lib/content/snippets.js
+++ b/lib/content/snippets.js
@@ -695,22 +695,44 @@
   // 2176782336 - 60466176 = 2116316160. This ensure to always have 6
   // chars even if Math.random() returns its minimum value 0.0
   //
   return Math.floor(Math.random() * 2116316160 + 60466176).toString(36);
 }
 
 function wrapPropertyAccess(object, property, descriptor)
 {
-  let currentDescriptor = Object.getOwnPropertyDescriptor(object, property);
+  let dot = property.indexOf(".");

[Manish Jethani, 2019/03/07 14:12:22]

Nit: When finding the index of a dot in a string (domain names), we have used
the name `dotIndex` in other places. Would be nice to be consistent, but up to
you.

[hub, 2019/03/07 21:32:27]

Done.

+  if (dot == -1)
+  {
+    // simple property case.

[Manish Jethani, 2019/03/07 14:12:22]

Nit: I don't know if this comment adds much value, but it's up to you.

[hub, 2019/03/07 21:32:27]

Acknowledged.

+    let currentDescriptor = Object.getOwnPropertyDescriptor(object, property);
+    if (currentDescriptor && !currentDescriptor.configurable)
+      return;
+
+    Object.defineProperty(object, property, descriptor);
+    return;
+  }
+  let name = property.slice(0, dot);

[Manish Jethani, 2019/03/07 14:12:22]

Nit: A blank line after the `if` block would make it more readable IMO.

[hub, 2019/03/07 21:32:27]

Done.

+  property = property.slice(dot + 1);
+  let value = object[name];
+  if (value && typeof value == "object")

[Manish Jethani, 2019/03/07 14:12:22]

Suggestion: It might be better if we defined this formally as:

  let isObject = value => value && typeof value == "object";

But it's your call, I'm not too keen on this either.

[hub, 2019/03/07 21:32:27]

I'll leave it as is

[Manish Jethani, 2019/03/12 07:00:08]

Acknowledged.

+    wrapPropertyAccess(value, property, descriptor);
+
+  let currentDescriptor = Object.getOwnPropertyDescriptor(object, name);
   if (currentDescriptor && !currentDescriptor.configurable)
-    return false;
+    return;
 
-  Object.defineProperty(object, property, descriptor);
-  return true;
+  let setter = newValue =>
+  {
+    value = newValue;
+    if (newValue && typeof newValue == "object")
+      wrapPropertyAccess(newValue, property, descriptor);
+  };
+  Object.defineProperty(object, name, {get: () => value, set: setter});

[Manish Jethani, 2019/03/07 14:12:23]

Nit: Blank line before this would be nice.

[hub, 2019/03/07 21:32:27]

Done.

 }
 
 /**
  * Overrides the <code>onerror</code> handler to discard tagged error messages
  * from our property wrapping.
  *
  * @param {string} magic The magic string that tags the error message.
  */
@@ -744,18 +766,18 @@
 
   let rid = randomId();
 
   function abort()
   {
     throw new ReferenceError(rid);
   }
 
-  if (wrapPropertyAccess(window, property, {get: abort, set() {}}))
-    overrideOnError(rid);
+  wrapPropertyAccess(window, property, {get: abort, set() {}});
+  overrideOnError(rid);
 }
 
 exports["abort-on-property-read"] = makeInjector(abortOnPropertyRead,
                                                  wrapPropertyAccess,
                                                  overrideOnError,
                                                  randomId);
 
 /**
@@ -776,18 +798,18 @@
 
   let rid = randomId();
 
   function abort()
   {
     throw new ReferenceError(rid);
   }
 
-  if (wrapPropertyAccess(window, property, {set: abort}))
-    overrideOnError(rid);
+  wrapPropertyAccess(window, property, {set: abort});
+  overrideOnError(rid);
 }
 
 exports["abort-on-property-write"] = makeInjector(abortOnPropertyWrite,
                                                   wrapPropertyAccess,
                                                   overrideOnError,
                                                   randomId);
 
 /**
@@ -839,18 +861,18 @@
     },
     set(value)
     {
       abort();
       currentValue = value;
     }
   };
 
-  if (wrapPropertyAccess(object, name, descriptor))
-    overrideOnError(rid);
+  wrapPropertyAccess(object, name, descriptor);
+  overrideOnError(rid);
 }
 
 exports["abort-current-inline-script"] =
   makeInjector(abortCurrentInlineScript, wrapPropertyAccess, toRegExp,
                overrideOnError, regexEscape, randomId);
 
 /**
  * Strips a query string parameter from <code>fetch()</code> calls.