Issue 4579 - Ignore runtime.lastError caused by wrapper

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 25 matching lines @@
     "tabs.query",
     "tabs.reload",
     "tabs.sendMessage",
     "tabs.update",
     "webNavigation.getAllFrames",
     "webRequest.handlerBehaviorChanged",
     "windows.create",
     "windows.update"
   ];
 
-  // Errors that occur only when we show an interest in the response from an

[Manish Jethani, 2017/10/19 10:13:23]

We should keep this array because there are more such errors.

[kzar, 2017/10/19 10:17:49]

Maybe a Set would be a better data structure to use?

I also wonder if we should export this, so it's transparent to the rest of the
code what we're ignoring. But I guess we can do that when it becomes necessary.

[Manish Jethani, 2017/10/19 10:24:46]

Regarding the error message itself, I should go back and check earlier Chrome
versions. I'll also include Ollie here to verify the behavior on Edge.

[Manish Jethani, 2017/10/19 10:30:32]

Good point, done.
Sounds like it might be a good idea, though I agree let's wait until it's
actually needed.

[Sebastian Noack, 2017/10/19 18:30:18]

We can easily turn it into an array, set or regexp when needed. But as long as
we only checking for a single string, I think we should not over-engineer our
data structures for some potential future use case.

But yeah, we should probably figure out the behavior (and error message if any)
on Microsoft Edge first.

[kzar, 2017/10/19 18:37:39]

Yea I'm pretty sure we all agree there, if we can just compare one string we
should do that instead.

[Sebastian Noack, 2017/10/19 22:08:31]

Ollie just confirmed, that on Microsoft Edge no error occurs if no response is
sent. So it seems this is the only error message string we have to check for.
Hence a Set or array seems unnecessary.

-  // API call.
-  const noFulfillmentErrors = [
-    "The message port closed before a response was received."
-  ];
+  // Since we add a callback for all messaging API calls in our wrappers,
+  // Chrome assumes we're interested in the response; when there's no response,
+  // it sets runtime.lastError
+  const portClosedBeforeResponseError =
+    // Older versions of Chrome have a typo:
+    // https://crrev.com/c33f51726eacdcc1a487b21a13611f7eab580d6d
+    /^The message port closed before a res?ponse was received\.$/;
 
   function wrapAPI(api)
   {
     let object = browser;
     let path = api.split(".");
     let name = path.pop();
 
     for (let node of path)
     {
       object = object[node];
 
       if (!object)
         return;
     }
 
     let func = object[name];
     object[name] = 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();
 
       return new Promise((resolve, reject) =>
       {
         func.call(object, ...args, result =>
         {
           let error = browser.runtime.lastError;
-          if (error && !noFulfillmentErrors.includes(error.message))
+          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);
+
+              // Add a more helpful stack trace.
+              error.stack = callStack;
+            }
+
             reject(error);
+          }
           else
+          {
             resolve(result);
+          }
         });
       });
     };
   }
 
   function shouldWrapAPIs()
   {
     try
     {
       return !(browser.storage.local.get([]) instanceof Promise);
@@ skipping 19 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];
   }
 }