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"
-    })
-    // Some errors on Chromium are asynchronous promise rejections
-    // (e.g. "Error: No frame with id 574 in tab 266"). We explicitly ignore
-    // these, because otherwise they show up in the log too often and make
-    // debugging difficult.
-    .catch(() => {});
+    });
+
+    // 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;
 
-    // Sometimes a frame gets removed from the document between the time the
-    // content script sends the "elemhide.getSelectors" message and the time
-    // the background page tries to inject a style sheet, which causes
-    // tabs.insertCSS to throw a frame-not-found error. We must handle any such
-    // errors.

[Sebastian Noack, 2018/03/13 18:40:58]

This is only the case on Firefox, while Chromium gives an asynchronous error in
this case, right? Mind making this explicit in the comment here?

Now, I also realize, that this means in case the frame doesn't exist anymore, on
Chrome we return true, but on Firefox we return false?

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

Done.

+    // 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);
 });