Issue 5090 - Use user stylesheets for element hiding if possible

lib/elemHideHelper.js

+/*
+ * This file is part of Adblock Plus <https://adblockplus.org/>,
+ * Copyright (C) 2006-2017 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
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+/** @module elemHideHelper */
+
+"use strict";
+
+const {RegExpFilter} = require("filterClasses");
+const {ElemHide} = require("elemHide");
+const {checkWhitelisted} = require("whitelisting");
+const {extractHostFromFrame} = require("url");
+const {port} = require("messaging");
+const devtools = require("devtools");
+
+let userStylesheetsSupported = true;
+
+function hideElements(tabId, frameId, selectors)
+{
+  return new Promise(resolve =>
+  {
+    let code = selectors.join(", ") + "{display: none !important;}";
+
+    try
+    {
+      chrome.tabs.insertCSS(tabId,
+        {
+          code,
+          cssOrigin: "user",
+          frameId,
+          matchAboutBlank: true
+        },
+        () =>
+        {
+          resolve(!chrome.runtime.lastError);

[Sebastian Noack, 2017/05/24 18:14:06]

Asynchronous errors are still silenced, as they only get logged if
chrome.runtime.lastError is not accessed by any callback. Do we even need this
callback here? Otherwise the whole logic wouldn't need to be asynchronous in the
first place.

[Manish Jethani, 2017/05/25 02:52:26]

We need to resolve the promise, and if it failed then we need to report that to
the caller. The other option is to ignore the error and let the caller believe
that everything went OK, but then they won't try to inject the styles inline,
ads won't be blocked.

[Sebastian Noack, 2017/05/30 15:50:31]

What is the scenario in which we'd get an asynchronous error here? Is it worth
to consider to handle it? Or can we perhaps do something better than relying on
content scripts and author stylesheets? Note that if it wouldn't be for Chrome
and older Firefox versions, we wouldn't even care to keep that fallback around.

[Manish Jethani, 2017/05/31 06:21:39]

I couldn't get Firefox to give an error in the response, but I think it's
supposed to happen when the CSS is invalid. Firefox reports the error in the
console, but it doesn't report it in chrome.runtime.lastError, nor in the catch
handler when you use browser.tabs.insertCSS. Maybe you're right, we could just
ignore any error.

[Sebastian Noack, 2017/05/31 11:13:30]

In the case of invalid CSS, falling back to the content script wouldn't be much
better, it will merely move the error from one place to another. What we should
ideally do in case of an invalid selector, regardless whether we use insertCSS()
or content scripts to inject it, is bisecting the list of selectors until we
have all selectors injected except the broken one(s). This, however, is
unrelated of the change here.

[Manish Jethani, 2017/05/31 14:41:15]

OK, that makes sense. Let's ignore any possible error in the callback then, at
least for now. I'll make the change.

+        }
+      );
+    }
+    catch (error)
+    {
+      if (/\bError processing cssOrigin\b/.test(error.message) == -1)
+        throw error;
+
+      userStylesheetsSupported = false;
+      resolve(false);
+    }
+  });
+}
+
+port.on("get-selectors", (msg, sender) =>
+{
+  let selectors;
+  let trace = devtools && devtools.hasPanel(sender.page);
+
+  if (!checkWhitelisted(sender.page, sender.frame,
+                        RegExpFilter.typeMap.DOCUMENT |
+                        RegExpFilter.typeMap.ELEMHIDE))
+  {
+    let specificOnly = checkWhitelisted(sender.page, sender.frame,
+                                        RegExpFilter.typeMap.GENERICHIDE);
+    selectors = ElemHide.getSelectorsForDomain(
+      extractHostFromFrame(sender.frame),
+      specificOnly ? ElemHide.SPECIFIC_ONLY : ElemHide.ALL_MATCHING
+    );
+  }
+  else
+  {
+    selectors = [];
+  }
+
+  if (!userStylesheetsSupported)
+    return {selectors, trace, inject: true};
+
+  if (selectors.length == 0)

[Sebastian Noack, 2017/05/24 18:14:05]

Why do we have to treat no selectors as a special case?

[Manish Jethani, 2017/05/25 02:52:26]

I thought you said a little bit earlier in the review that if the selectors list
was empty then we could skip calling insertCSS entirely. I do agree, hence the
special case.

[Sebastian Noack, 2017/05/30 15:50:31]

Yes, but the content script is already checking for this condition.

[Manish Jethani, 2017/05/31 06:21:39]

First we try to insert the selectors in the background page, then we send the
selectors to the content script. The content script is not in the picture until
later.

+  {
+    if (trace)
+      return {selectors, trace, inject: false};
+
+    return {trace, inject: false};
+  }
+
+  return hideElements(sender.page.id, sender.frame.id, selectors)
+  .then(success =>
+  {
+    let response = {trace, inject: !success};
+
+    if (trace || !success)
+      response.selectors = selectors;
+
+    return response;
+  });
+});
+
+port.on("hide-elements", (msg, sender) =>
+{
+  if (!msg.selectors || msg.selectors.length == 0)
+    return true;
+
+  return hideElements(sender.page.id, sender.frame.id, msg.selectors);
+});