Noissue - Explicitly ignore asynchronous errors from tabs.insertCSS

lib/cssInjection.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 52 matching lines @@
 
 function createStyleSheet(selectors)
 {
   return Array.from(createRules(selectors)).join("\n");
 }
 
 function addStyleSheet(tabId, frameId, styleSheet)
 {
   try
   {
-    browser.tabs.insertCSS(tabId, {
+    let promise = browser.tabs.insertCSS(tabId, {
       code: styleSheet,
       cssOrigin: "user",
       frameId,
       matchAboutBlank: true,
       runAt: "document_start"
-    })
-    .catch(() => {});

[Sebastian Noack, 2018/03/12 22:41:25]

To clarify, so the catch-block below is essentially only hit on Firefox, and the
catch-callback is essentially only called on Chromium?

Why is it necessary to filter for error messages that contain "cssOrigin" on
Firefox, but not on Chromium?

[Manish Jethani, 2018/03/13 06:09:47]

On both Firefox and Chrome 65 the error for "cssOrigin" is synchronous, so we're
handling that correctly for both platforms. Additionally, on Chrome 66+ I am
getting the error for "frame not found" (see stack trace) asynchronously. This
is the one that clogs up the log.

We won't be getting the "cssOrigin" error asynchronously because Chrome 66+
doesn't throw that error.

[Sebastian Noack, 2018/03/13 15:38:45]

I see. Perhaps add some comments here, clarifying when we get an asynchronous
error, and when insertCSS() throws synchronously.

[Manish Jethani, 2018/03/13 17:14:20]

Done.

+    });
+
+    // See error handling notes in the catch block.
+    promise.catch(() => {});

[Manish Jethani, 2018/03/13 22:08:38]

I actually find it a lot cleaner to have a promise variable here, it lets us
make this a separate JS statement and attach a comment to it.

[Sebastian Noack, 2018/03/13 23:02:21]

Well, now where you you just refer to the other comment, you could as well just
use an end-of-line comment here:

  browser.tabs.insertCSS(tabId, {
    ...
  }).catch(() => {});  // see below

[Manish Jethani, 2018/03/13 23:08:30]

Ack, but I do prefer this if you don't mind.

   }
   catch (error)
   {
+    // If the error is about the "cssOrigin" option, this is an older version
+    // of Chromium (65 and below) or Firefox (52 and below) that does not
+    // support user style sheets.
     if (/\bcssOrigin\b/.test(error.message))
       userStyleSheetsSupported = false;
 
+    // For other errors, we simply return false to indicate failure.
+    //
+    // One common error that occurs frequently is when a frame is not found
+    // (e.g. "Error: No frame with id 574 in tab 266"), which can happen when
+    // the code in the parent document has removed the frame before the
+    // background page has had a chance to respond to the content script's
+    // "elemhide.getSelectors" message. We simply ignore such errors, because
+    // otherwise they show up in the log too often and make debugging
+    // difficult.
+    //
+    // Also note that the missing frame error is thrown synchronously on
+    // Firefox, while on Chromium it is an asychronous promise rejection. In
+    // the latter case, we cannot indicate failure to the caller, but we still

[Sebastian Noack, 2018/03/13 23:02:21]

Well, we could signal failure, by returning a promise from this function. But
this seems to make the calling code somewhat complex. However, I'm unsure what
the consequence of ignoring failure on Chrome is, here.

[Manish Jethani, 2018/03/13 23:08:30]

That's not necessary at least for the case of the frame gone missing.
The consequence in the worst case should be that no style sheet is injected
anywhere. I think perhaps we should only ignore the specific error about missing
frames here and let any other errors get logged to the console, but in any case
the consequence isn't too bad.

[Manish Jethani, 2018/03/13 23:10:30]

Also note that returning a promise from here would really complicate the
implementation of updateFrameStyles. We should avoid that.

+    // explicitly ignore the error.
     return false;
   }
 
   return true;
 }
 
 function removeStyleSheet(tabId, frameId, styleSheet)
 {
   if (!styleSheetRemovalSupported)
     return;
@@ skipping 83 matching lines @@
     response.inlineEmulated = true;
 
   return response;
 });
 
 port.on("elemhide.injectSelectors", (message, sender) =>
 {
   updateFrameStyles(sender.page.id, sender.frame.id, message.selectors,
                     message.groupName);
 });