Issue 6539, 6782 - Implement support for snippets

lib/scriptInjection.js

Index: lib/scriptInjection.js
===================================================================
rename from lib/cssInjection.js
rename to lib/scriptInjection.js
--- a/lib/cssInjection.js
+++ b/lib/scriptInjection.js
@@ -10,39 +10,43 @@
  * 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 cssInjection */
+/** @module scriptInjection */

[kzar, 2018/07/19 12:24:19]

Be careful that Mercurial knows you've renamed lib/cssInjection.js to
lib/scriptInjection.js. Otherwise, the history gets lost - yes I've done that
before!

[kzar, 2018/07/19 12:24:20]

Like I mentioned on one of the other reviews, I don't feel like a CSS selector /
rule is a "script". So, maybe we can come up with a name which makes sense for
both snippet scripts and CSS rules?

[Manish Jethani, 2018/07/19 12:55:24]

Fair enough.

Since we're referring to it as "content filtering" in include.preload.js, how
about lib/contentFiltering.js?

[Manish Jethani, 2018/07/19 12:55:25]

Thanks, I'll try to do this correctly.

[kzar, 2018/07/19 13:28:55]

Sounds good.

[Manish Jethani, 2018/07/19 14:04:35]

Done.

 
 "use strict";
 
 const {RegExpFilter} = require("../adblockpluscore/lib/filterClasses");
 const {ElemHide} = require("../adblockpluscore/lib/elemHide");
 const {ElemHideEmulation} = require("../adblockpluscore/lib/elemHideEmulation");
+const {Snippets, compileScript} = require("../adblockpluscore/lib/snippets");
 const {checkWhitelisted} = require("./whitelisting");
 const {extractHostFromFrame} = require("./url");
 const {port} = require("./messaging");
 const {HitLogger} = require("./hitLogger");
 const info = require("info");
 
 // Chromium's support for tabs.removeCSS is still a work in progress and the
 // API is likely to be different from Firefox's; for now we just don't use it
 // at all, even if it's available.
 // See https://crbug.com/608854
 const styleSheetRemovalSupported = info.platform == "gecko";
 
 const selectorGroupSize = 1024;
 
 let userStyleSheetsSupported = true;
 
+let snippetsLibrarySource = "";
+let executableCode = new Map();
+
 function* splitSelectors(selectors)
 {
   // Chromium's Blink engine supports only up to 8,192 simple selectors, and
   // even fewer compound selectors, in a rule. The exact number of selectors
   // that would work depends on their sizes (e.g. "#foo .bar" has a size of 2).
   // Since we don't know the sizes of the selectors here, we simply split them
   // into groups of 1,024, based on the reasonable assumption that the average
   // selector won't have a size greater than 8. The alternative would be to
@@ -90,17 +94,17 @@
       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
+    // "content.applyFilters" 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
     // explicitly ignore the error.
     return false;
@@ -155,35 +159,84 @@
   // style sheet now.
   if (oldStyleSheet && oldStyleSheet != styleSheet)
     removeStyleSheet(tabId, frameId, oldStyleSheet);
 
   frame.injectedStyleSheets.set(groupName, styleSheet);
   return true;
 }
 
-port.on("elemhide.getSelectors", (message, sender) =>
+function getExecutableCode(script)
+{
+  let code = executableCode.get(script);
+  if (code)
+    return code;
+
+  code = compileScript(script, [snippetsLibrarySource]);
+
+  executableCode.set(script, code);
+  return code;
+}
+
+function executeScript(script, tabId, frameId)
+{
+  try
+  {
+    browser.tabs.executeScript(tabId, {
+      code: getExecutableCode(script),
+      frameId,
+      matchAboutBlank: true,
+      runAt: "document_start"
+    })
+    .catch(error =>
+    {
+      // Sometimes a frame is added and removed very quickly, in such cases we
+      // simply ignore the error.
+      if (error.message == "The frame was removed.")
+        return;
+
+      throw error;
+    });
+  }
+  catch (error)

[kzar, 2018/07/19 12:24:21]

So we re-throw the exception, catch it again, then ignore it?

[Manish Jethani, 2018/07/19 12:55:24]

No, the error thrown from the .catch() handler is asynchronous, it doesn't
appear here. We rethrow that so it appears in the console so we know what's
going on.

I added the .catch() handler not to ignore all errors but only to ignore the one
error that happens too often and doesn't mean anything to us. If you think that
we should instead ignore all errors over there, then that would be a different
thing. I personally think it's better to see the errors in the console, it
doesn't mean anything to the user and is only for developers.

[kzar, 2018/07/19 13:28:55]

OK.
Which one's that?
Yea me too, I'm generally against hiding exceptions unless there's a good reason
to do so.

[Manish Jethani, 2018/07/19 14:04:34]

It's the "The frame was removed." error.

[kzar, 2018/07/19 14:52:34]

That's the above catch. I thought we were talking about this one (line 199). I
guess I still don't understand what it's for.

[Manish Jethani, 2018/07/19 15:33:38]

Ah, I see what you mean. Yeah, I probably just added that catch as a precaution,
I can remove it and see what happens. But we will have to have a general
catch-all-and-ignore somewhere because we still want the rest of the code to
proceed even if the call to executeCode fails.

What I don't like about it here is that we're just ignoring the error, not even
logging it (so we don't know what happened).

[kzar, 2018/07/19 15:37:45]

Yea, ideally there would be an entry in the developer tools that a snippet
attempted to execute for a website, but that there was an exception. But
perhaps, for now, we should note this as a limitation and simply log the
exception to the console instead? We could prepend the message with some more
information about the snippet and the arguments passed to it.

[Manish Jethani, 2018/07/19 15:44:05]

Can we simply log the error?

  catch (error)
  {
    console.error(error);
  }

Let me try a snippet that throws an error and see what happens. I'll get back.

[Manish Jethani, 2018/07/19 15:54:08]

OK, so when a snippet throws an error that error is logged independently,
because of course the snippet runs as a its own content script.

I have added a comment here explaining why we catch and ignore all errors. We do
it for the same reason we do it for tabs.insertCSS

[kzar, 2018/07/19 16:07:40]

Alright, fair enough.

+  {
+  }
+}
+
+port.on("content.applyFilters", (message, sender) =>
 {
   let selectors = [];
   let emulatedPatterns = [];
   let trace = HitLogger.hasListener(sender.page.id);
   let inline = !userStyleSheetsSupported;
 
+  let {elemhide = false, snippets = false} = message.filterTypes ||

[kzar, 2018/07/19 12:24:19]

Is this syntax supported by the browsers we support? I've not seen it used
before.

In fact, the `= false` parts seems redundant, it'll end up as `undefined` which
is falsey anyway. Wouldn't this work the same?

let {elemhide, snippets} = message.filterTypes || {elemhide: true, snippets:
true};

[Manish Jethani, 2018/07/19 12:55:25]

Yeah, that's just object destructuring with default values. It's quite old, I'm
pretty sure it's supported by all our platforms.
Yeah, it would be the same. I guess I can remove the default assignments. Let me
do that in the next update.

[Manish Jethani, 2018/07/19 14:04:35]

Done.

+                                             {elemhide: true, snippets: true};
+
   if (!checkWhitelisted(sender.page, sender.frame, null,
-                        RegExpFilter.typeMap.DOCUMENT |
-                        RegExpFilter.typeMap.ELEMHIDE))
+                        RegExpFilter.typeMap.DOCUMENT))
   {
     let hostname = extractHostFromFrame(sender.frame);
-    let specificOnly = checkWhitelisted(sender.page, sender.frame, null,
-                                        RegExpFilter.typeMap.GENERICHIDE);
+
+    if (snippets)
+    {
+      for (let script of Snippets.getScriptsForDomain(hostname))
+        executeScript(script, sender.page.id, sender.frame.id);
+    }
 
-    selectors = ElemHide.getSelectorsForDomain(hostname, specificOnly);
+    if (elemhide && !checkWhitelisted(sender.page, sender.frame, null,
+                                      RegExpFilter.typeMap.ELEMHIDE))

[kzar, 2018/07/19 12:24:20]

It kind of sucks we have to call checkWhitelisted an extra time, but I don't see
an alternative if #@# exceptions don't apply to snippet filters.

I wonder if we could tweak the logic, so that the extra check is only made if
`snippets` is truthy?

[Manish Jethani, 2018/07/19 12:55:24]

#@# exception will be applied within Snippets.getScriptsForDomain already so
that's not the issue. The issue is that $elemhide doesn't not apply to snippet
filters. I'm not even sure how this is going to go, but I don't think we need an
equivalent of $elemhide for snippets.
The only time snippets will be falsy is when this is called from the composer. I
don't think it's worth complicating the code for that case alone. What do you
think?

[kzar, 2018/07/19 13:28:57]

Alright, fair enough I guess.

+    {
+      let specificOnly = checkWhitelisted(sender.page, sender.frame, null,

[kzar, 2018/07/19 12:24:18]

If we don't check for generichide/genericblock for snippet filters, do we at
least ensure that snippet filters have a domain when they're created?

[Manish Jethani, 2018/07/19 12:55:25]

Currently there are no such checks. I'll mention this in the issue. I don't
think we need an equivalent of $generichide for snippets, we already have
$document if we want to whitelist entirely. But I suppose requiring domains in a
snippet filter is a good idea.

[kzar, 2018/07/19 13:28:54]

Yea, I definitely think so. Please could you open an issue for that?

[Manish Jethani, 2018/07/19 14:04:35]

Done.

https://issues.adblockplus.org/ticket/6797

[kzar, 2018/07/19 14:52:34]

Thanks

+                                          RegExpFilter.typeMap.GENERICHIDE);
+      selectors = ElemHide.getSelectorsForDomain(hostname, specificOnly);
 
-    for (let filter of ElemHideEmulation.getRulesForDomain(hostname))
-      emulatedPatterns.push({selector: filter.selector, text: filter.text});
+      for (let filter of ElemHideEmulation.getRulesForDomain(hostname))
+        emulatedPatterns.push({selector: filter.selector, text: filter.text});
+    }
   }
 
   if (!inline && !updateFrameStyles(sender.page.id, sender.frame.id,
                                     selectors, "standard"))
   {
     inline = true;
   }
 
@@ -201,8 +254,15 @@
   return response;
 });
 
 port.on("elemhide.injectSelectors", (message, sender) =>
 {
   updateFrameStyles(sender.page.id, sender.frame.id, message.selectors,
                     message.groupName, message.appendOnly);
 });
+
+fetch(browser.extension.getURL("/snippets.js"), {cache: "no-cache"})

[kzar, 2018/07/19 12:24:20]

What if fetch throws?

[Manish Jethani, 2018/07/19 12:55:24]

I don't see how it could throw in practice if the file exists, but in the worst
case it'll log to the console and there won't be any snippets available (it
fails gracefully).

[kzar, 2018/07/19 13:28:55]

Acknowledged.

+.then(response => response.ok ? response.text() : "")

[kzar, 2018/07/19 12:24:21]

Nit: We normally put some indentation before the `.then`.

[Manish Jethani, 2018/07/19 12:55:25]

I thought when we break foo.bar.lambda() then (1) the continuation should start
with a dot and (2) it should be aligned with the start of the expression.

  foo.bar
  .lambda()

Rather than:

  foo.bar
    .lambda()

This is pretty much how it's done in other projects. I couldn't find any
reference to any such rule in our style guide though.

[kzar, 2018/07/19 13:28:56]

Well, with your example it would be more like this:

foo.bar
   .lambda()

But who cares? Not me to be honest.
There isn't one.

+.then(text =>

[kzar, 2018/07/19 12:24:20]

Why not just do the assignment in the previous function?

.then(response =>
{
  if (response.ok)
    snippetsLibrarySource = response.text();
});

(That way I also avoid assigning it on failure, I figure it's better to keep the
old version rather than trashing it.

[Manish Jethani, 2018/07/19 12:55:26]

response.text() returns a promise, which then resolves with the actual text,
asynchronously.

By the way, async/await is going to make these things so much easier to write
and maintain.

[kzar, 2018/07/19 13:28:54]

Ah right, the indirection makes sense then, fair enough.

+{
+  snippetsLibrarySource = text;
+});