Issue 6969 - Implement abort-on-property-read snippet

lib/content/snippets.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 474 matching lines @@
         (!re || re.test(target.textContent)))
     {
       event.preventDefault();
     }
   },
   true);
 }
 
 exports["prevent-inline-scripts"] = preventInlineScripts;
 
+/**
+ * Generates a random alphanumeric ID consisting of 6 base-36 digits
+ * from the range 100000..zzzzzz (both inclusive).
+ *
+ * @returns {string} The random ID.
+ */
 function randomId()
 {
   // 2176782336 is 36^6 which mean 6 chars [a-z0-9]
   // 60466176 is 36^5
-  // 2176782336 - 60466176 = 2116316160. This ensure always 6 chars
-  // for when Math.random() returns 0.0

[Manish Jethani, 2018/10/24 20:47:45]

The last line of the comment here, it's a bit misleading because it's not just
for when Math.random() returns 0.0. Right? I think we could leave this part out.

We could add that the range of values in base 36 is 100000-zzzzzz (both
inclusive).

[hub, 2018/10/24 22:17:38]

reworded the comments. Also added a proper JSDoc at the top.

+  // 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, desc)

[Manish Jethani, 2018/10/24 20:47:44]

At least for function parameters, let's stick with the current practice
throughout this file, which is to use fuller names (descriptor rather than
desc).

[hub, 2018/10/24 22:17:37]

Done.

-{
-  // simple property
-  let d = Object.getOwnPropertyDescriptor(object, property);

[Manish Jethani, 2018/10/24 20:47:45]

Maybe we should call it "currentDescriptor" instead of just "d"?

[hub, 2018/10/24 22:17:38]

Done.

-  if (!d)
-    d = {};
-  if (typeof desc.get != "undefined")
-  {
-    if (d.get != desc.get)
-      d.get = desc.get;
-  }
-  if (typeof desc.set != "undefined")
-  {
-    if (d.set != desc.set)
-      d.set = desc.set;
-  }
-  Object.defineProperty(object, property, d);
-}
-/**

[Manish Jethani, 2018/10/24 20:47:45]

Nit: We should leave a blank line here.

[hub, 2018/10/24 22:17:38]

Done.

- * Will patch a property on the window object to abort when read.

[Manish Jethani, 2018/10/24 20:47:44]

Regarding the JSDoc comment for this snippet, my thinking was that we should not
take on the burden of documenting uBlock Origin's snippets and then keeping the
documentation up to date. Instead, we can simply point to the original
implementation, like I have done here:

https://hg.adblockplus.org/adblockpluscore/file/7126b8cc2fad/lib/content/snip...

This would make it easier to maintain these snippets over time (one less thing
to worry about), while also clearly distinguishing them from our own ideas.

[hub, 2018/10/24 22:17:37]

I disagree. This is documenting this code. It is easier to read local
documentation than something somewhere else.

[Manish Jethani, 2018/10/24 23:02:09]

Alright, so we need to credit uBlock Origin for the idea. How do you propose we
do this?

[hub, 2018/10/25 14:32:25]

Added a link to uBlock.

- * It will intercept the onerror callback and block it if tagged.
- *
- * @todo handle properties of properties.
- *
- * @param {string} prop the name of the property.
- */
-function abortOnPropertyRead(prop)

[Manish Jethani, 2018/10/24 20:47:45]

Let's rename "prop" to "property" since we are using fuller names throughout
this file (e.g. "element" rather than "elem").

[hub, 2018/10/24 22:17:37]

Done.

-{
-  if (!prop)
+function wrapPropertyAccess(object, property, descriptor)
+{
+  let currentDescriptor = Object.getOwnPropertyDescriptor(object, property);
+  if (currentDescriptor && !currentDescriptor.configurable)
+    return false;
+
+  Object.defineProperty(object, property, descriptor);
+  return true;
+}
+
+/**
+ * Patches a property on the window object to abort execution when the
+ * property is read.
+ *
+ * No error is be printed to the console.
+ *
+ * The idea originates from
+ * {@link https://github.com/uBlockOrigin/uAssets/blob/80b195436f8f8d78ba713237bfc268ecfc9d9d2b/filters/resources.txt#L1703 uBlock Origin}.
+ *
+ * @param {string} property The name of the property.
+ */
+function abortOnPropertyRead(property)
+{
+  if (!property)
     return;
 
   let rid = randomId();
 
-  function canceller()

[Manish Jethani, 2018/10/24 20:47:45]

How about we just call this function "abort", which matches the name of the
snippet?

[hub, 2018/10/24 22:17:38]

Done.

+  function abort()
   {
     throw new ReferenceError(rid);
   }
 
   let {onerror} = window;

[Manish Jethani, 2018/10/24 21:01:48]

Hmm ... I think we're going to need to repeat this code as well, so maybe this
could also be a separate function:

  function makeAborter()
  {
    let magic = randomId();

    window.onerror = ...;

    return function()
    {
      throw new ReferenceError(magic);
    };
  }

Then abortOnPropertyRead would be just this:

  if (!property)
    return;

  wrapPropertyAccess(window, property, {get: makeAborter(), set: noop});

What do you think? Or is this overkill? I feel like we might need to do this
eventually.

[Manish Jethani, 2018/10/24 21:12:28]

Looking at the other patch, yeah I think we should do this, but makeAborter
should take an optional callback as an argument.

  function makeAborter(shouldAbort = () => true)
  {
    ...

    return function()
    {
      if (shouldAbort())
        throw new ReferenceError(magic);
    };
  }

[hub, 2018/10/24 22:17:38]

I think it is.

[hub, 2018/10/24 22:17:38]

All of this make the whole thing more complicated, more subject to side effects.
Simple contained pieces of code are IMHO cleaner.

[Manish Jethani, 2018/10/24 23:02:09]

So you intend to repeat this window.onerror boilerplate in every snippet that
does something like this (and you already have another one in review)? I don't
really understand how that's better, but OK.

-  window.onerror = (message, ...rest) =>
-  {
-    if (typeof message == "string" && message.includes(rid))

[Manish Jethani, 2018/10/24 23:02:09]

I think we should check specifically for "ReferenceError: [id]" here. I can
imagine some other errors getting ignored because of this by accident.

So `new RegExp("\\bReferenceError: " + id "\\b").test(message)`

[hub, 2018/10/25 14:32:26]

No. Because the message format isn't part of any standard, so it would break at
any implementation change.
I saw a bug recently in Mozilla's bugzilla where some framework assumed this and
broke when change were made in Firefox error reporting.

The chances to match the random id to another message are quite thin.

[Manish Jethani, 2018/10/25 16:12:15]

Well it's OK for this to not work and for an error to be thrown instead, the
opposite it not OK (catching and ignoring errors from other people's code). The
"magic" string is literally just a string of 6 digits, that can occur in any
error that deals with numbers.

Another idea is to use 30 digits instead of 6, which would make it more unique.
The current information content of the magic string is `log2(36) * 6 = ~31 bits`
(this assumes Math.random() is cryptographically random, which is also not the
case). We could just add more base-36 digits to make it more unique.

[hub, 2018/10/25 20:26:09]

I'll make 5 times these. Should be random enough.

-      return true;
-    if (onerror && typeof onerror == "function")

[Manish Jethani, 2018/10/24 20:47:45]

I don't think the first condition here is necessary.

[hub, 2018/10/24 22:17:37]

Done.

-      return onerror.call(this, message, ...rest);

[Manish Jethani, 2018/10/24 20:47:45]

I just realized that you can have a function that has no call method (if the
prototype has been changed). We should change this to
`(() => {}).call.call(onerror, this, message, ...rest)`, in my opinion.

[hub, 2018/10/24 22:17:38]

Actually I think I went a bit overboard. I don't even need `this`. In this case
`onerror` is a function that will just be called as is. So I'll change this to
`return onerror(message, ...rest);`.

[Manish Jethani, 2018/10/24 23:02:09]

You did not go overboard, because in strict mode the `this` for the function
points to the window object, and if you don't pass it then it will be undefined,
which could cause the function to fail (if it accesses `this`). We have to
faithfully pass on `this` to the original handler.

[hub, 2018/10/25 14:32:25]

No. If the original handler is bound to something, that bind is retained.

https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/onerror
clearly show there is not expectation to be bound to anything.

[Manish Jethani, 2018/10/25 16:12:15]

How can we assume that the original handler will be bound to anything?

Take this simple case:

  <script>
    "use strict";

    window.onerror = function()
    {
      this.location = "/error-page";
    };
  </script>

This is going to bomb if you call it the way you intend to.
It says nothing about `this`. In practice the function is called by default, as
I said, with `this` set to the window object.

[hub, 2018/10/25 20:26:09]

So call it is. BTW if the function object has no `call` method, how does it get
called?

[Manish Jethani, 2018/10/25 21:32:25]

The call method is a feature of the API, it is not required for the JS runtime
for calling the function.

For example:

  function foo() { console.log("foo"); }
  foo.__proto__ = {}
  typeof foo // "function"
  foo.call // undefined
  foo() // prints "foo"

[hub, 2018/10/26 15:19:05]

Done.

-  };
-
-  let d = {get: canceller, set() {}};

[Manish Jethani, 2018/10/24 20:47:44]

We don't need this temporary variable, in my opinion.

[Manish Jethani, 2018/10/24 20:47:45]

I feel like we're going to need this noop function a lot. What do you think
about declaring it as a separate function in the file?

  /**
   * Does nothing.
   */
  function noop()
  {
  }

Then I think this would look like:

  wrapPropertyAccess(window, property, {get: abort, set: noop});

[hub, 2018/10/24 22:17:38]

Done.

[hub, 2018/10/24 22:17:38]

I don't think this make it any clearer.

[Manish Jethani, 2018/10/24 23:02:09]

Acknowledged.

-
-  wrapPropertyAccess(window, prop, d);
+  if (wrapPropertyAccess(window, property, {get: abort, set() {}}))
+  {
+    window.onerror = (message, ...rest) =>
+    {
+      if (typeof message == "string" && message.includes(rid))
+        return true;
+      if (typeof onerror == "function")
+        return (() => {}).call.call(onerror, this, message, ...rest);
+    };
+  }
 }
 
 exports["abort-on-property-read"] = makeInjector(abortOnPropertyRead,
                                                  wrapPropertyAccess,
                                                  randomId);