Issue 29833630: Issue 6798 - Implement hide-if-shadow-contains snippet (Closed)
|
Created:
July 19, 2018, 2:29 a.m. by Manish Jethani Modified:
July 19, 2018, 4:59 p.m. CC:
Sebastian Noack Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionThis snippet is useful for websites like yandex.ru that tuck the "реклама" label inside a closed shadow root.
Try this filter:
yandex.ru#$#hide-if-shadow-contains реклама li
Also see #5650.
Patch Set 1 #
Total comments: 15
Patch Set 2 : Remove premature hardening #
Total comments: 5
MessagesTotal messages: 15
Patch Set 1 I'll file an issue on Trac, but meanwhile see the description here.
https://codereview.adblockplus.org/29833630/diff/29833631/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29833630/diff/29833631/lib/content/snippet... lib/content/snippets.js:111: new MutationObserver(() => I wonder if we should just remove the element, instead of hiding it and then adding a mutation observer to re-hide? https://codereview.adblockplus.org/29833630/diff/29833631/lib/content/snippet... lib/content/snippets.js:171: originalAttachShadow.apply.bind(originalAttachShadow); 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. https://codereview.adblockplus.org/29833630/diff/29833631/lib/content/snippet... lib/content/snippets.js:183: if (!host || !root) 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?
https://codereview.adblockplus.org/29833630/diff/29833631/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29833630/diff/29833631/lib/content/snippet... lib/content/snippets.js:111: new MutationObserver(() => On 2018/07/19 13:02:09, kzar wrote: > I wonder if we should just remove the element, instead of hiding it and then > adding a mutation observer to re-hide? 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. https://codereview.adblockplus.org/29833630/diff/29833631/lib/content/snippet... lib/content/snippets.js:171: originalAttachShadow.apply.bind(originalAttachShadow); On 2018/07/19 13:02:09, kzar wrote: > 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. 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. > 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. Agreed. https://codereview.adblockplus.org/29833630/diff/29833631/lib/content/snippet... lib/content/snippets.js:183: if (!host || !root) On 2018/07/19 13:02:09, kzar wrote: > 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? 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.
https://codereview.adblockplus.org/29833630/diff/29833631/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29833630/diff/29833631/lib/content/snippet... lib/content/snippets.js:111: new MutationObserver(() => On 2018/07/19 13:30:09, Manish Jethani wrote: > On 2018/07/19 13:02:09, kzar wrote: > > I wonder if we should just remove the element, instead of hiding it and then > > adding a mutation observer to re-hide? > > 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. Acknowledged. https://codereview.adblockplus.org/29833630/diff/29833631/lib/content/snippet... lib/content/snippets.js:171: originalAttachShadow.apply.bind(originalAttachShadow); On 2018/07/19 13:30:09, Manish Jethani wrote: > 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. 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.
https://codereview.adblockplus.org/29833630/diff/29833631/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29833630/diff/29833631/lib/content/snippet... lib/content/snippets.js:171: originalAttachShadow.apply.bind(originalAttachShadow); On 2018/07/19 13:38:11, kzar wrote: > On 2018/07/19 13:30:09, Manish Jethani wrote: > > > 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. > > This reply kind of implies that you didn't write the code. 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... > 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. 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.
https://codereview.adblockplus.org/29833630/diff/29833631/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29833630/diff/29833631/lib/content/snippet... lib/content/snippets.js:171: originalAttachShadow.apply.bind(originalAttachShadow); On 2018/07/19 14:20:54, Manish Jethani wrote: > 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. 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. > 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. 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.
https://codereview.adblockplus.org/29833630/diff/29833631/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29833630/diff/29833631/lib/content/snippet... lib/content/snippets.js:171: originalAttachShadow.apply.bind(originalAttachShadow); On 2018/07/19 15:02:52, kzar wrote: > On 2018/07/19 14:20:54, Manish Jethani wrote: > > 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. OK, but why can't you just do this: originalFunction.call(this, param1, param2); Why do you have to bind originalFunction.call there? > > 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. > > 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. 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.
https://codereview.adblockplus.org/29833630/diff/29833631/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29833630/diff/29833631/lib/content/snippet... lib/content/snippets.js:171: originalAttachShadow.apply.bind(originalAttachShadow); On 2018/07/19 15:28:34, Manish Jethani wrote: > OK, but why can't you just do this: > > originalFunction.call(this, param1, param2); > > Why do you have to bind originalFunction.call there? In case the website messes with `Function.prototype.call`! Yes, this all gets crazy fast! > Yes, the attribution/copyright stuff applies in general, not specifically for > snippets. Agreed. I also was talking about all reviews, not just snippet reviews.
https://codereview.adblockplus.org/29833630/diff/29833631/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29833630/diff/29833631/lib/content/snippet... lib/content/snippets.js:171: originalAttachShadow.apply.bind(originalAttachShadow); On 2018/07/19 15:33:33, kzar wrote: > On 2018/07/19 15:28:34, Manish Jethani wrote: > > OK, but why can't you just do this: > > > > originalFunction.call(this, param1, param2); > > > > Why do you have to bind originalFunction.call there? > > In case the website messes with `Function.prototype.call`! Yes, this all gets > crazy fast! 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 > > Yes, the attribution/copyright stuff applies in general, not specifically for > > snippets. > > Agreed. I also was talking about all reviews, not just snippet reviews. Acknowledged.
https://codereview.adblockplus.org/29833630/diff/29833631/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29833630/diff/29833631/lib/content/snippet... lib/content/snippets.js:171: originalAttachShadow.apply.bind(originalAttachShadow); On 2018/07/19 15:39:57, Manish Jethani wrote: > 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. 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.
Patch Set 2: Remove premature hardening https://codereview.adblockplus.org/29833630/diff/29833631/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29833630/diff/29833631/lib/content/snippet... lib/content/snippets.js:171: originalAttachShadow.apply.bind(originalAttachShadow); On 2018/07/19 15:52:40, kzar wrote: > On 2018/07/19 15:39:57, Manish Jethani wrote: > > 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. > > 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. Fair enough, let's keep it simple for now and let's see how it goes. Done.
https://codereview.adblockplus.org/29833630/diff/29834607/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29833630/diff/29834607/lib/content/snippet... lib/content/snippets.js:163: let originalAttachShadow = Element.prototype.attachShadow; With my suggestions below we can remove this line too. https://codereview.adblockplus.org/29833630/diff/29834607/lib/content/snippet... lib/content/snippets.js:167: if (!originalAttachShadow) `if (!Element.prototype.attachShadow)` https://codereview.adblockplus.org/29833630/diff/29834607/lib/content/snippet... lib/content/snippets.js:207: let root = originalAttachShadow.apply(this, args); `let root = this.attachShadow(...args);`
https://codereview.adblockplus.org/29833630/diff/29834607/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29833630/diff/29834607/lib/content/snippet... lib/content/snippets.js:207: let root = originalAttachShadow.apply(this, args); On 2018/07/19 16:11:41, kzar wrote: > `let root = this.attachShadow(...args);` Isn't this going to be an infinite loop? this.attachShadow would refer in turn to this very function, if I am not mistaken.
LGTM https://codereview.adblockplus.org/29833630/diff/29834607/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29833630/diff/29834607/lib/content/snippet... lib/content/snippets.js:207: let root = originalAttachShadow.apply(this, args); On 2018/07/19 16:15:10, Manish Jethani wrote: > On 2018/07/19 16:11:41, kzar wrote: > > `let root = this.attachShadow(...args);` > > Isn't this going to be an infinite loop? this.attachShadow would refer in turn > to this very function, if I am not mistaken. Oh yea, good point we're overwriting it ourselves. Fair enough. 
|
