Issue 6798 - Implement hide-if-shadow-contains snippet

lib/content/snippets.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 90 matching lines @@
  * <code>display: none !important</code>.
  *
  * @param {HTMLElement} element The HTML element to hide.
  */
 function hideElement(element)
 {
   element.style.setProperty("display", "none", "important");
 
   // Listen for changes to the style property and if our values are unset
   // then reset them.
   new MutationObserver(() =>

[kzar, 2018/07/19 13:02:09]

I wonder if we should just remove the element, instead of hiding it and then
adding a mutation observer to re-hide?

[Manish Jethani, 2018/07/19 13:30:09]

I think it's probably best to mess as little as required with the structure of
the document. We already know that it's OK to set the display to none but we
don't know what effects it might have if we remove elements, so just to be safe
maybe we should stick to the familiar approach.

I can easily imagine a site breaking because the code referred to
lastElementChild [1] and it did not reference the element that it expected. This
happening with the style property is much less likely.

By the way I would like to explore the option of using user style sheets here at
some point, but we don't have time for that right now.

[kzar, 2018/07/19 13:38:11]

Acknowledged.

   {
     if (element.style.getPropertyValue("display") != "none" ||
         element.style.getPropertyPriority("display") != "important")
     {
       element.style.setProperty("display", "none", "important");
     }
   })
   .observe(element, {attributes: true, attributeFilter: ["style"]});
 }
 
@@ skipping 31 matching lines @@
  * Hides any HTML element or one of its ancestors matching a CSS selector if
  * the text content of the element's shadow contains a given string.
  *
  * @param {string} search The string to look for in every HTML element's
  *   shadow.
  * @param {string} selector The CSS selector that an HTML element must match
  *   for it to be hidden.
  */
 function hideIfShadowContains(search, selector = "*")
 {
   let originalAttachShadow = Element.prototype.attachShadow;

[kzar, 2018/07/19 16:11:41]

With my suggestions below we can remove this line too.

 
   // If there's no Element.attachShadow API present then we don't care, it must
   // be Firefox or an older version of Chrome.
   if (!originalAttachShadow)

[kzar, 2018/07/19 16:11:41]

`if (!Element.prototype.attachShadow)`

     return;
-
-  let applyOriginalAttachShadow =
-    originalAttachShadow.apply.bind(originalAttachShadow);

[kzar, 2018/07/19 13:02:09]

This looks like an attempt at hardening the code that the snippet injects into
the website from the website itself. Since our script runs last this seems like
a lost cause. They could just mess with `Element.prototype.attachShadow` instead
of the element's `attachShadow` property for example.

Furthermore, there's a whole bunch of other things the website could mess with
to screw with our logic, e.g. `Function.prototype.bind`. If we're going to take
care to keep track of one API, shouldn't we do the same for the others? But,
like I say, that seems like a lost cause anyway.

[Manish Jethani, 2018/07/19 13:30:09]

Yeah, I'm not really sure why this is needed. Even if we were injecting our code
before any other code had a chance to run, why would we need this? I'm sure
there's a good reason, I just don't know it. Anyway, let me simplify this in my
next update.
Agreed.

[kzar, 2018/07/19 13:38:11]

This reply kind of implies that you didn't write the code.

I don't want to get into the whole attribution, copyright etc stuff again. But
to be clear, I'm just reviewing (here and in your other reviews) only the
technical details, with the assumption that you wrote all the code yourself. If
that's not the case I again defer to Sebastian/Felix about what we should do.
Please don't take a "looks good" from me to indicate I think the
attribution/legal side is OK.

[Manish Jethani, 2018/07/19 14:20:54]

No, it does not apply anything like that. I did write the code. I just used the
same technique we use in inject.preload.js [1]. This is our technique.

[1]:
https://github.com/adblockplus/adblockpluschrome/blob/aaaf8ea87f9847ad8d37a52...
This is one hundred percent code written by me for Adblock Plus.

Please see the original proof-of-concept here:

https://codereview.adblockplus.org/29762707/

Yes, I would really prefer if you moved on from this copyright stuff and focused
on the code. If you have any issues, please raise them via the appropriate
channels.

[kzar, 2018/07/19 15:02:52]

Gotya, if the idea/code came from our inject.preload.js code instead of some
other project then that makes sense and is fine.

FWIW I wrote that code with the assumption that our injected code would always
run before the website's code. That's why we went to crazy lengths to avoid
sabotage by the website, since in theory it was possible to make something
bullet-proof. But yea I agree, like you say it no longer makes sense here.
Look I was just clarifying my position that I am not reviewing the legalities or
attribution of your code, and that it is your responsibility to check with
Sebastian or Felix regarding that stuff. If you hadn't replied it would have
been the last you heard from me about it.

[Manish Jethani, 2018/07/19 15:28:34]

OK, but why can't you just do this:

  originalFunction.call(this, param1, param2);

Why do you have to bind originalFunction.call there?
Thanks for clarifying.

Yes, the attribution/copyright stuff applies in general, not specifically for
snippets. We probably use the same techniques as other ad blockers all over the
place, regardless of who first came up with any given technique. I have not
looked closely at how other ad blockers work, but I would be very, very
surprised if every one of them had not copied some of Adblock Plus's techniques.
We are all working on the same problems, the solutions are bound to be the same.

[kzar, 2018/07/19 15:33:33]

In case the website messes with `Function.prototype.call`! Yes, this all gets
crazy fast!
Agreed. I also was talking about all reviews, not just snippet reviews.

[Manish Jethani, 2018/07/19 15:39:57]

I see, in that case I'd like to keep it. Reason: only inline scripts will run
before our injected code. This means we still defend against external scripts
loaded via <script src="...">. It's not a binary thing, I think the stronger our
defenses the better it is.

For what it's worth even inject.preload.js is not safe from inline scripts [1]

[1]: https://issues.adblockplus.org/ticket/6469#comment:16
Acknowledged.

[kzar, 2018/07/19 15:52:40]

Well, in that case, what about all the other APIs we're using without wrapping?
For example, `HTMLElement.prototype.style`. Wrapping only this one is pointless.

If you want to have a go at hardening this script with me, I'll try and help,
but I vote we do it later. Like you said before, this is just a rough initial
version. In the meantime, I vote to remove this `applyOriginalAttachShadow`
stuff.

[Manish Jethani, 2018/07/19 16:04:58]

Fair enough, let's keep it simple for now and let's see how it goes.

Done.

 
   // Mutation observers mapped to their corresponding shadow roots and their
   // hosts.
   let shadows = new WeakMap();
 
   function observe(mutations, observer)
   {
     let {host, root} = shadows.get(observer) || {};
 
     // Since it's a weak map, it's possible that either the element or its
     // shadow has been removed.
     if (!host || !root)

[kzar, 2018/07/19 13:02:09]

I thought WeakMap just avoided memory leaks if the key is removed, but seems
like you're saying it's possible that only the value was removed? If so,
couldn't we leak memory?

[Manish Jethani, 2018/07/19 13:30:09]

This is just a hypothesis, but I think that if either the element or its shadow
is removed, any internal references to the observer (which is on the shadow
root) would be released as well. At that point the reference in the weak map
here would be the only reference.

Yeah I do wonder if there's a better way to do this.

       return;
 
     // If the shadow contains the given text, check if the host or one of its
     // ancestors matches the selector; if a matching element is found, hide
     // it.
     if (root.textContent.includes(search))
     {
       let element = host;
 
       do
       {
         if (element.matches(selector))
         {
           hideElement(element);
           break;
         }
       }
       while (element = element.parentElement);
     }
   }
 
   Object.defineProperty(Element.prototype, "attachShadow", {
     value(...args)
     {
       // Create the shadow root first. It doesn't matter if it's a closed
       // shadow root, we keep the reference in a weak map.
-      let root = applyOriginalAttachShadow(this, args);
+      let root = originalAttachShadow.apply(this, args);

[kzar, 2018/07/19 16:11:41]

`let root = this.attachShadow(...args);`

[Manish Jethani, 2018/07/19 16:15:10]

Isn't this going to be an infinite loop? this.attachShadow would refer in turn
to this very function, if I am not mistaken.

[kzar, 2018/07/19 16:28:51]

Oh yea, good point we're overwriting it ourselves. Fair enough.

 
       // Listen for relevant DOM mutations in the shadow.
       let observer = new MutationObserver(observe);
       observer.observe(root, {
         childList: true,
         characterData: true,
         subtree: true
       });
 
       // Keep references to the shadow root and its host in a weak map. If
       // either the shadow is detached or the host itself is removed from the
       // DOM, the mutation observer too will be freed eventually and the entry
       // will be removed.
       shadows.set(observer, {host: this, root});
 
       return root;
     }
   });
 }
 
 exports["hide-if-shadow-contains"] = makeInjector(hideIfShadowContains,
                                                   hideElement);