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,46 @@
   // 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(".");
+  if (dot == -1)
+  {
+    // simple property case.
+    let currentDescriptor = Object.getOwnPropertyDescriptor(object, property);
+    if (currentDescriptor && !currentDescriptor.configurable)
+      return false;
+
+    Object.defineProperty(object, property, descriptor);
+    return true;
+  }
+  let result = true;
+  let name = property.slice(0, dot);
+  property = property.slice(dot + 1);
+  let value = object[name];
+  if (value && typeof value == "object")
+    result = wrapPropertyAccess(value, property, descriptor);
+
+  let currentDescriptor = Object.getOwnPropertyDescriptor(object, name);
   if (currentDescriptor && !currentDescriptor.configurable)
-    return false;
+    return result;

[Manish Jethani, 2019/03/04 13:28:49]

If I understand this correctly, we return the result of the leaf property. i.e.
in `foo.bar.lambda`, if `foo.bar = {}`, we return whether we wrapped `lambda`.
However if `foo.bar` is not an object, then we basically always return `true` -
even if the descriptor for `foo.bar` is non-configurable.

For example:

 window.foo = {};
 Object.defineProperty(window.foo, "bar", {value: "fixed value"});

Now wrapping `foo.bar.lambda` will return `true`. Right?

This is another inconsistency. I'm wondering if we really need a return value?
We could always just override the `onerror` handler in the calling code, it
shouldn't matter if it is wrapped multiple times.

[hub, 2019/03/05 13:49:13]

I should have set `result` to `false` (line 714). Fixing this.

[Manish Jethani, 2019/03/06 19:07:12]

This has the opposite problem though. If we're trying to wrap `foo.bar.lambda`
and `foo` exists but `foo.bar` does not exist, it will return `false`; but later
when `foo.bar` is set, and then `foo.bar.lambda` is set, then `foo.bar.lambda`
will be wrapped. But because we returned `false` initially there will be no
`onerror` handler.

It's hard to do this right. How about we just always add an `onerror` handler
and return neither `true` nor `false` from this function?

[hub, 2019/03/06 21:22:03]

Silly me, I should just return true at then end since we did define the
property.

I'm ok to make the creation of the onerror handler unconditional.

 
-  Object.defineProperty(object, property, descriptor);
-  return true;
+  let setter = a =>

[Manish Jethani, 2019/03/04 13:28:49]

Nit: Instead of `a` this would seem better as `newValue`.

[hub, 2019/03/05 13:49:13]

Done.

+  {
+    value = a;
+    if (a && typeof a == "object")
+      wrapPropertyAccess(a, property, descriptor);
+  };
+  Object.defineProperty(object, name, {get: () => value, set: setter});
+  return result;
 }
 
 /**
  * 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.
  */