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 = Object.getOwnPropertyDescriptor(object, name);
+    // If the property is not writable assigning will fail, so we use
+    // Object.defineProperty here instead. Assuming the property isn't
+    // inherited its other attributes (e.g. enumerable) are presereved,

[Manish Jethani, 2018/02/16 14:03:53]

Nit: "preserved" (spelling)

[kzar, 2018/02/16 16:41:08]

Done.

+    // except accessor properties since we're specifying a value.

[Manish Jethani, 2018/02/16 14:03:53]

Nit: "except accessor attributes" (?)

[kzar, 2018/02/16 16:41:08]

How's this?

[Manish Jethani, 2018/02/16 16:47:38]

Since "enumerable" is referred to as an attribute in the previous part of the
sentence, I thought the accessors ("get" and "set") should also be referred to
as attributes, but I may be wrong. Anyway this is very minor.

+    Object.defineProperty(object, name, {
+      value: function(...args)

[Manish Jethani, 2018/02/16 14:03:53]

Since we're using an object literal here now we can use the shorthand:

  Object.defineProperty(object, name, {
    value(...args)
    {
      ...
    }
  });

i.e. no function keyword

[kzar, 2018/02/16 16:41:08]

Good point, eslint picked that up I just forgot to run it. Done.

+      {

[kzar, 2018/02/16 13:36:46]

Sorry for changing the indentation here, it makes the diff a bit messy. I
haven't changed anything else inside the function though... honest.

+        let callStack = new Error().stack;
 
-    delete descriptor["get"];
-    delete descriptor["set"];
+        if (typeof args[args.length - 1] == "function")
+          return func.apply(object, args);
 
-    descriptor.value = function(...args)
-    {
-      let callStack = new Error().stack;
+        // 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();
 
-      if (typeof args[args.length - 1] == "function")
-        return func.apply(object, args);
+        let resolvePromise = null;
+        let rejectPromise = null;
 
-      // 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();
+        func.call(object, ...args, result =>
+        {
+          let error = browser.runtime.lastError;
+          if (error && !portClosedBeforeResponseError.test(error.message))
+          {
+            // 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);
 
-      let resolvePromise = null;
-      let rejectPromise = null;
+              // Add a more helpful stack trace.
+              error.stack = callStack;
+            }
 
-      func.call(object, ...args, result =>
-      {
-        let error = browser.runtime.lastError;
-        if (error && !portClosedBeforeResponseError.test(error.message))
+            rejectPromise(error);
+          }
+          else
+          {
+            resolvePromise(result);
+          }
+        });
+
+        return new Promise((resolve, reject) =>
         {
-          // 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);
-        }
-        else
-        {
-          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];
   }
 }