Issue 6388 - Wrap inherited function properties as well

polyfill.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 60 matching lines @@
       object = object[node];
 
       if (!object)
         return;
     }
 
     let func = object[name];
     if (!func)
       return;
 
-    let descriptor;
-    let objectProto = object;
-    do

[Manish Jethani, 2018/02/15 13:17:51]

By the way I find this nicer too:

    let descriptor;
    for (let proto = object;
         !(descriptor = Object.getOwnPropertyDescriptor(proto, name));
         proto = Object.getPrototypeOf(proto));

So the extra variable stays local to the loop, and also we don't check if proto
is non-null, only descriptor.

-    {
-      descriptor = Object.getOwnPropertyDescriptor(objectProto, name);
-    }
-    while (!descriptor && (objectProto = Object.getPrototypeOf(objectProto)));

[Manish Jethani, 2018/02/15 09:59:24]

I think we should have a separate function called getPropertyDescriptor right
above wrapAsyncAPIs that looks like this:

  function getPropertyDescriptor(object, name)
  {
    return Object.getOwnPropertyDescriptor(object, name) ||
           getPropertyDescriptor(Object.getPrototypeOf(object), name);
  }

This is elegant and will not be a performance issue like recursion normally is
since the prototype chain is not very long.

Note that Object.getPrototypeOf here is guaranteed to return non-null (since we
already checked that the object contains the property), we can avoid that check.

-
-    delete descriptor["get"];
-    delete descriptor["set"];
-
-    descriptor.value = function(...args)
-    {
-      let callStack = new Error().stack;
-
-      if (typeof args[args.length - 1] == "function")
-        return func.apply(object, args);
-
-      // If the last argument is undefined, we drop it from the list assuming
-      // it stands for the optional callback. We must do this, because we have
-      // to replace it with our own callback. If we simply append our own
-      // callback to the list, it won't match the signature of the function and
-      // will cause an exception.
-      if (typeof args[args.length - 1] == "undefined")
-        args.pop();
-
-      let resolvePromise = null;
-      let rejectPromise = null;
-
-      func.call(object, ...args, result =>
+    // If the property is not writable assigning it will fail, so we use
+    // Object.defineProperty here instead. Assuming the property isn't
+    // inherited its other attributes (e.g. enumerable) are preserved,
+    // except for accessor attributes (e.g. get and set) which are discarded
+    // since we're specifying a value.
+    Object.defineProperty(object, name, {
+      value(...args)
       {
-        let error = browser.runtime.lastError;
-        if (error && !portClosedBeforeResponseError.test(error.message))
+        let callStack = new Error().stack;
+
+        if (typeof args[args.length - 1] == "function")
+          return func.apply(object, args);
+
+        // If the last argument is undefined, we drop it from the list assuming
+        // it stands for the optional callback. We must do this, because we have
+        // to replace it with our own callback. If we simply append our own
+        // callback to the list, it won't match the signature of the function
+        // and will cause an exception.
+        if (typeof args[args.length - 1] == "undefined")
+          args.pop();
+
+        let resolvePromise = null;
+        let rejectPromise = null;
+
+        func.call(object, ...args, result =>
         {
-          // runtime.lastError is already an Error instance on Edge, while on
-          // Chrome it is a plain object with only a message property.
-          if (!(error instanceof Error))
+          let error = browser.runtime.lastError;
+          if (error && !portClosedBeforeResponseError.test(error.message))
           {
-            error = new Error(error.message);
-
-            // Add a more helpful stack trace.
-            error.stack = callStack;
+            // runtime.lastError is already an Error instance on Edge, while on
+            // Chrome it is a plain object with only a message property.
+            if (!(error instanceof Error))
+            {
+              error = new Error(error.message);
+
+              // Add a more helpful stack trace.
+              error.stack = callStack;
+            }
+
+            rejectPromise(error);
           }
-
-          rejectPromise(error);
-        }
-        else
+          else
+          {
+            resolvePromise(result);
+          }
+        });
+
+        return new Promise((resolve, reject) =>
         {
-          resolvePromise(result);
-        }
-      });
-
-      return new Promise((resolve, reject) =>
-      {
-        resolvePromise = resolve;
-        rejectPromise = reject;
-      });
-    };
-
-    Object.defineProperty(object, name, descriptor);
+          resolvePromise = resolve;
+          rejectPromise = reject;
+        });
+      }
+    });
   }
 
   function wrapRuntimeOnMessage()
   {
     let {onMessage} = browser.runtime;
     let {addListener, removeListener} = onMessage;
 
     onMessage.addListener = function(listener)
     {
       if (typeof listener != "function")
@@ skipping 83 matching lines @@
 
   // Workaround since HTMLCollection, NodeList, StyleSheetList, and CSSRuleList
   // didn't have iterator support before Chrome 51.
   // https://bugs.chromium.org/p/chromium/issues/detail?id=401699
   for (let object of [HTMLCollection, NodeList, StyleSheetList, CSSRuleList])
   {
     if (!(Symbol.iterator in object.prototype))
       object.prototype[Symbol.iterator] = Array.prototype[Symbol.iterator];
   }
 }