include.preload.js - Issue 29893559: Issue 6999 - Generate style sheets in background page

Side by Side Diff: include.preload.js

Issue 29893559: Issue 6999 - Generate style sheets in background page (Closed) Base URL: https://hg.adblockplus.org/adblockpluschrome/

Patch Set: Fix comment wrapping Created Sept. 29, 2018, 10:43 a.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

View unified diff | Download patch

OLD	NEW
1 /*	1 /*

2 * This file is part of Adblock Plus <https://adblockplus.org/>,	2 * This file is part of Adblock Plus <https://adblockplus.org/>,

3 * Copyright (C) 2006-present eyeo GmbH	3 * Copyright (C) 2006-present eyeo GmbH

4 *	4 *

5 * Adblock Plus is free software: you can redistribute it and/or modify	5 * Adblock Plus is free software: you can redistribute it and/or modify

6 * it under the terms of the GNU General Public License version 3 as	6 * it under the terms of the GNU General Public License version 3 as

7 * published by the Free Software Foundation.	7 * published by the Free Software Foundation.

8 *	8 *

9 * Adblock Plus is distributed in the hope that it will be useful,	9 * Adblock Plus is distributed in the hope that it will be useful,

10 * but WITHOUT ANY WARRANTY; without even the implied warranty of	10 * but WITHOUT ANY WARRANTY; without even the implied warranty of

(...skipping 207 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
218 },	218 },

219 collapse =>	219 collapse =>

220 {	220 {

221 if (collapse)	221 if (collapse)

222 {	222 {

223 if (selector)	223 if (selector)

224 {	224 {

225 if (!collapsingSelectors.has(selector))	225 if (!collapsingSelectors.has(selector))

226 {	226 {

227 collapsingSelectors.add(selector);	227 collapsingSelectors.add(selector);

228 contentFiltering.addSelectors([selector], null, "collapsing", true);	228 contentFiltering.addSelectors([selector], "collapsing", true);

229 }	229 }

230 }	230 }

231 else	231 else

232 {	232 {

233 hideElement(element);	233 hideElement(element);

234 }	234 }

235 }	235 }

236 }	236 }

237 );	237 );

238 }	238 }

(...skipping 151 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
390 document.removeEventListener("DOMContentLoaded", this.trace);	390 document.removeEventListener("DOMContentLoaded", this.trace);

391 this.observer.disconnect();	391 this.observer.disconnect();

392 clearTimeout(this.timeout);	392 clearTimeout(this.timeout);

393 }	393 }

394 };	394 };

395	395

396 function ContentFiltering()	396 function ContentFiltering()

397 {	397 {

398 this.styles = new Map();	398 this.styles = new Map();

399 this.tracer = null;	399 this.tracer = null;

400 this.inline = true;

401	400
Sebastian Noack 2018/09/29 14:36:13 Nit: The blank line here no longer seems to help t Nit: The blank line here no longer seems to help the readability.
402 this.elemHideEmulation = new ElemHideEmulation(	401 this.elemHideEmulation = new ElemHideEmulation(this.hideElements.bind(this));

403 () => {},

404 this.hideElements.bind(this)

405 );

406 }	402 }

407 ContentFiltering.prototype = {	403 ContentFiltering.prototype = {

408 selectorGroupSize: 1024,	404 addStyleSheetInline(styleSheet, groupName = "standard", appendOnly = false)

409

410 addSelectorsInline(selectors, groupName, appendOnly = false)

411 {	405 {

412 let style = this.styles.get(groupName);	406 let style = this.styles.get(groupName);

413	407

414 if (style && !appendOnly)

415 {

416 while (style.sheet.cssRules.length > 0)

417 style.sheet.deleteRule(0);

418 }

419

420 if (selectors.length == 0)

421 return;

422

423 if (!style)	408 if (!style)

424 {	409 {

425 // Create <style> element lazily, only if we add styles. Add it to	410 // Create <style> element lazily, only if we add styles. Add it to

426 // the <head> or <html> element. If we have injected a style element	411 // the <head> or <html> element. If we have injected a style element

427 // before that has been removed (the sheet property is null), create a	412 // before that has been removed (the sheet property is null), create a

428 // new one.	413 // new one.

429 style = document.createElement("style");	414 style = document.createElement("style");

430 (document.head \|\| document.documentElement).appendChild(style);	415 (document.head \|\| document.documentElement).appendChild(style);

431	416

432 // It can happen that the frame already navigated to a different	417 // It can happen that the frame already navigated to a different

433 // document while we were waiting for the background page to respond.	418 // document while we were waiting for the background page to respond.

434 // In that case the sheet property may stay null, after adding the	419 // In that case the sheet property may stay null, after adding the

435 // <style> element.	420 // <style> element.

436 if (!style.sheet)	421 if (!style.sheet)

437 return;	422 return;

438	423

439 this.styles.set(groupName, style);	424 this.styles.set(groupName, style);

440 }	425 }

441	426

442 // Chromium's Blink engine supports only up to 8,192 simple selectors, and	427 if (appendOnly)

443 // even fewer compound selectors, in a rule. The exact number of selectors	428 style.textContent += styleSheet;
	Sebastian Noack 2018/09/29 14:36:13 I didn't benchmark it, but I'd assume using insert I didn't benchmark it, but I'd assume using insertRule() would be faster, especially in the append scenario. IF you reassign textContent on the other hand, the browser has to parse and apply the whole CSS again. Also, if you assign to textContent, a text node is created, and the browser may keep a copy of the raw CSS in memory, while with insertRule(), no node is added to the DOM, and only the parsed representation needs to be kept in memory. Manish Jethani 2018/09/29 15:58:48 The problem with using `insertRule` here: we don't Show quoted text On 2018/09/29 14:36:13, Sebastian Noack wrote: > I didn't benchmark it, but I'd assume using insertRule() would be faster, > especially in the append scenario. IF you reassign textContent on the other > hand, the browser has to parse and apply the whole CSS again. > > Also, if you assign to textContent, a text node is created, and the browser may > keep a copy of the raw CSS in memory, while with insertRule(), no node is added > to the DOM, and only the parsed representation needs to be kept in memory. The problem with using `insertRule` here: we don't know that the style sheet is always going to be a single rule; we could split the style sheet along newline characters and treat each part as a rule, but then that's another assumption the content script would be making about the background page's logic. Also it would be a bit odd to user `insertRule` for appending and `textContent` for replacement. Let's leave it like it is and revisit if it's a performance issue. Sebastian Noack 2018/09/29 16:39:30 How about making the function in core return an ar Show quoted text On 2018/09/29 15:58:48, Manish Jethani wrote: > On 2018/09/29 14:36:13, Sebastian Noack wrote: > > I didn't benchmark it, but I'd assume using insertRule() would be faster, > > especially in the append scenario. IF you reassign textContent on the other > > hand, the browser has to parse and apply the whole CSS again. > > > > Also, if you assign to textContent, a text node is created, and the browser > may > > keep a copy of the raw CSS in memory, while with insertRule(), no node is > added > > to the DOM, and only the parsed representation needs to be kept in memory. > > The problem with using `insertRule` here: we don't know that the style sheet is > always going to be a single rule; we could split the style sheet along newline > characters and treat each part as a rule, but then that's another assumption the > content script would be making about the background page's logic. How about making the function in core return an array of rules (rather than joining it by newlines in the first place) and the passing this array to the content script? This seems to be the most sensible solution to me. Show quoted text > Also it would > be a bit odd to user `insertRule` for appending and `textContent` for > replacement. What I had in mind is the old logic we had in here, where we'd use insertRule() in both cases (after using removeRule(), or if it's simpler just recreate the <style> element, in case we aren't appending). Manish Jethani 2018/09/29 17:28:04 That's not really an option, it would destroy the Show quoted text On 2018/09/29 16:39:30, Sebastian Noack wrote: > On 2018/09/29 15:58:48, Manish Jethani wrote: > > On 2018/09/29 14:36:13, Sebastian Noack wrote: > > > I didn't benchmark it, but I'd assume using insertRule() would be faster, > > > especially in the append scenario. IF you reassign textContent on the other > > > hand, the browser has to parse and apply the whole CSS again. > > > > > > Also, if you assign to textContent, a text node is created, and the browser > > may > > > keep a copy of the raw CSS in memory, while with insertRule(), no node is > > added > > > to the DOM, and only the parsed representation needs to be kept in memory. > > > > The problem with using `insertRule` here: we don't know that the style sheet > is > > always going to be a single rule; we could split the style sheet along newline > > characters and treat each part as a rule, but then that's another assumption > the > > content script would be making about the background page's logic. > > How about making the function in core return an array of rules (rather than > joining it by newlines in the first place) and the passing this array to the > content script? This seems to be the most sensible solution to me. That's not really an option, it would destroy the optimizations we have done for the most common cases. Show quoted text > > Also it would > > be a bit odd to user `insertRule` for appending and `textContent` for > > replacement. > > What I had in mind is the old logic we had in here, where we'd use insertRule() > in both cases (after using removeRule(), or if it's simpler just recreate the > <style> element, in case we aren't appending). Makes sense, but then again core can only return a style sheet. Unless you mean that `createStyleSheet` itself should return rules, but then again I don't really understand what the benefit of that would be. In the content script, you would still have two code paths, one for the full style sheet and one for a list of style rules. Sebastian Noack 2018/09/29 21:01:49 How many ms would that add to the bill if core wil Show quoted text On 2018/09/29 17:28:04, Manish Jethani wrote: > On 2018/09/29 16:39:30, Sebastian Noack wrote: > > On 2018/09/29 15:58:48, Manish Jethani wrote: > > > On 2018/09/29 14:36:13, Sebastian Noack wrote: > > > > I didn't benchmark it, but I'd assume using insertRule() would be faster, > > > > especially in the append scenario. IF you reassign textContent on the > other > > > > hand, the browser has to parse and apply the whole CSS again. > > > > > > > > Also, if you assign to textContent, a text node is created, and the > browser > > > may > > > > keep a copy of the raw CSS in memory, while with insertRule(), no node is > > > added > > > > to the DOM, and only the parsed representation needs to be kept in memory. > > > > > > The problem with using `insertRule` here: we don't know that the style sheet > > is > > > always going to be a single rule; we could split the style sheet along > newline > > > characters and treat each part as a rule, but then that's another assumption > > the > > > content script would be making about the background page's logic. > > > > How about making the function in core return an array of rules (rather than > > joining it by newlines in the first place) and the passing this array to the > > content script? This seems to be the most sensible solution to me. > > That's not really an option, it would destroy the optimizations we have done for > the most common cases. > > > > Also it would > > > be a bit odd to user `insertRule` for appending and `textContent` for > > > replacement. > > > > What I had in mind is the old logic we had in here, where we'd use > insertRule() > > in both cases (after using removeRule(), or if it's simpler just recreate the > > <style> element, in case we aren't appending). > > Makes sense, but then again core can only return a style sheet. > > Unless you mean that `createStyleSheet` itself should return rules, but then > again I don't really understand what the benefit of that would be. In the > content script, you would still have two code paths, one for the full style > sheet and one for a list of style rules. How many ms would that add to the bill if core will cache (and return) an array of rules (which have to be joined every time we inject a user stylesheet) rather than caching/returning a joined string? Perhaps, the optimization will be slightly less effective for platforms that support user stylesheets, but I'd expect caching an array of rules still being more efficient than what we are doing now where the rules have to be generated as well every time (rather than just being joined). Manish Jethani 2018/09/30 08:06:21 If you run the JS profiler in DevTools with the `g Show quoted text On 2018/09/29 21:01:49, Sebastian Noack wrote: > On 2018/09/29 17:28:04, Manish Jethani wrote: > > On 2018/09/29 16:39:30, Sebastian Noack wrote: > > > On 2018/09/29 15:58:48, Manish Jethani wrote: > > > > On 2018/09/29 14:36:13, Sebastian Noack wrote: > > > > > I didn't benchmark it, but I'd assume using insertRule() would be > faster, > > > > > especially in the append scenario. IF you reassign textContent on the > > other > > > > > hand, the browser has to parse and apply the whole CSS again. > > > > > > > > > > Also, if you assign to textContent, a text node is created, and the > > browser > > > > may > > > > > keep a copy of the raw CSS in memory, while with insertRule(), no node > is > > > > added > > > > > to the DOM, and only the parsed representation needs to be kept in > memory. > > > > > > > > The problem with using `insertRule` here: we don't know that the style > sheet > > > is > > > > always going to be a single rule; we could split the style sheet along > > newline > > > > characters and treat each part as a rule, but then that's another > assumption > > > the > > > > content script would be making about the background page's logic. > > > > > > How about making the function in core return an array of rules (rather than > > > joining it by newlines in the first place) and the passing this array to the > > > content script? This seems to be the most sensible solution to me. > > > > That's not really an option, it would destroy the optimizations we have done > for > > the most common cases. > > > > > > Also it would > > > > be a bit odd to user `insertRule` for appending and `textContent` for > > > > replacement. > > > > > > What I had in mind is the old logic we had in here, where we'd use > > insertRule() > > > in both cases (after using removeRule(), or if it's simpler just recreate > the > > > <style> element, in case we aren't appending). > > > > Makes sense, but then again core can only return a style sheet. > > > > Unless you mean that `createStyleSheet` itself should return rules, but then > > again I don't really understand what the benefit of that would be. In the > > content script, you would still have two code paths, one for the full style > > sheet and one for a list of style rules. > > How many ms would that add to the bill if core will cache (and return) an array > of rules (which have to be joined every time we inject a user stylesheet) rather > than caching/returning a joined string? If you run the JS profiler in DevTools with the `getSelectorsForDomain` version, go to the Bottom-Up tab, and open the V8 Runtime node in the tree, the first things you'll see there are DoJoin, Join, InnerArrayJoin, etc. The extension spends a significant amount of time just joining strings. We are avoiding the native join function by doing it manually [1], but it would still not be free. [1]: https://codereview.adblockplus.org/29892563/ Show quoted text > Perhaps, the optimization will be slightly less effective for platforms that > support user stylesheets, but I'd expect caching an array of rules still being > more efficient than what we are doing now where the rules have to be generated > as well every time (rather than just being joined). No, we can't avoid rule generation because the array of selectors for known domains is created afresh each time. Anyway, we've already made it more complex than it was, I think the current implementation is most optimal for the most common cases. Why do we even want to change the implementation in core just to address a special case here, which is significantly older versions of Chrome and Firefox, all versions of Edge, and _only_ for collapsing selectors. I'm OK with even disabling this feature for those browsers: the content script can send the injectSelectors message to the background page and not care if it actually worked. This would be the best option at this point. Alternatively, the content script could just call `styleSheet.split("\n")` to get the rules. Sebastian Noack 2018/09/30 14:19:24 This doesn't seem to only be about element collaps Show quoted text On 2018/09/30 08:06:21, Manish Jethani wrote: > On 2018/09/29 21:01:49, Sebastian Noack wrote: > > On 2018/09/29 17:28:04, Manish Jethani wrote: > > > On 2018/09/29 16:39:30, Sebastian Noack wrote: > > > > On 2018/09/29 15:58:48, Manish Jethani wrote: > > > > > On 2018/09/29 14:36:13, Sebastian Noack wrote: > > > > > > I didn't benchmark it, but I'd assume using insertRule() would be > > faster, > > > > > > especially in the append scenario. IF you reassign textContent on the > > > other > > > > > > hand, the browser has to parse and apply the whole CSS again. > > > > > > > > > > > > Also, if you assign to textContent, a text node is created, and the > > > browser > > > > > may > > > > > > keep a copy of the raw CSS in memory, while with insertRule(), no node > > is > > > > > added > > > > > > to the DOM, and only the parsed representation needs to be kept in > > memory. > > > > > > > > > > The problem with using `insertRule` here: we don't know that the style > > sheet > > > > is > > > > > always going to be a single rule; we could split the style sheet along > > > newline > > > > > characters and treat each part as a rule, but then that's another > > assumption > > > > the > > > > > content script would be making about the background page's logic. > > > > > > > > How about making the function in core return an array of rules (rather > than > > > > joining it by newlines in the first place) and the passing this array to > the > > > > content script? This seems to be the most sensible solution to me. > > > > > > That's not really an option, it would destroy the optimizations we have done > > for > > > the most common cases. > > > > > > > > Also it would > > > > > be a bit odd to user `insertRule` for appending and `textContent` for > > > > > replacement. > > > > > > > > What I had in mind is the old logic we had in here, where we'd use > > > insertRule() > > > > in both cases (after using removeRule(), or if it's simpler just recreate > > the > > > > <style> element, in case we aren't appending). > > > > > > Makes sense, but then again core can only return a style sheet. > > > > > > Unless you mean that `createStyleSheet` itself should return rules, but then > > > again I don't really understand what the benefit of that would be. In the > > > content script, you would still have two code paths, one for the full style > > > sheet and one for a list of style rules. > > > > How many ms would that add to the bill if core will cache (and return) an > array > > of rules (which have to be joined every time we inject a user stylesheet) > rather > > than caching/returning a joined string? > > If you run the JS profiler in DevTools with the `getSelectorsForDomain` version, > go to the Bottom-Up tab, and open the V8 Runtime node in the tree, the first > things you'll see there are DoJoin, Join, InnerArrayJoin, etc. The extension > spends a significant amount of time just joining strings. We are avoiding the > native join function by doing it manually [1], but it would still not be free. > > [1]: https://codereview.adblockplus.org/29892563/ > > > Perhaps, the optimization will be slightly less effective for platforms that > > support user stylesheets, but I'd expect caching an array of rules still being > > more efficient than what we are doing now where the rules have to be generated > > as well every time (rather than just being joined). > > No, we can't avoid rule generation because the array of selectors for known > domains is created afresh each time. Anyway, we've already made it more complex > than it was, I think the current implementation is most optimal for the most > common cases. > > Why do we even want to change the implementation in core just to address a > special case here, which is significantly older versions of Chrome and Firefox, > all versions of Edge, and _only_ for collapsing selectors. I'm OK with even > disabling this feature for those browsers: the content script can send the > injectSelectors message to the background page and not care if it actually > worked. This would be the best option at this point. Alternatively, the content > script could just call `styleSheet.split("\n")` to get the rules. This doesn't seem to only be about element collapsing, only the append scenario is, and yes that the whole CSS has to be parsed and applied again there is a minor issue. However, the major issue here is that you assign to textContent (for element collapsing as well for element hiding), which causes a text node with all the injected CSS to be created, which isn't an option for following reasons: * A copy of the raw CSS text will be kept in memory, increasing the per-document memory footprint. * The website can detect changes we do to the CSS, since changing the textContent triggered a MutationObserver while CSSOM cannot be listened for. * As more as we add to the DOM, as higher the chance this will cause side effects with random web applications. I even have a real world example: Outlook 365 uses (or at least used to use) a subdocument with an <html contenteditable> element for composing emails, and then serializes the DOM in in order to generate the mail body. So everything we add to the DOM will end up in outgoing emails. An empty <style> element would be relative harmless there, but adding 600KB of redundant CSS text to each email is bad. * While technically arguable, copyright trolls used to give ad blockers shit based on how visible there interference with any website is in DOM shown in the devtools. It might confuse other users of the devtools as well to see that long list of selectors. Manish Jethani 2018/10/01 14:01:36 If assigning `textContent` is an issue, one other Show quoted text On 2018/09/30 14:19:24, Sebastian Noack wrote: > On 2018/09/30 08:06:21, Manish Jethani wrote: > > On 2018/09/29 21:01:49, Sebastian Noack wrote: > > > On 2018/09/29 17:28:04, Manish Jethani wrote: > > > > On 2018/09/29 16:39:30, Sebastian Noack wrote: > > > > > On 2018/09/29 15:58:48, Manish Jethani wrote: > > > > > > On 2018/09/29 14:36:13, Sebastian Noack wrote: > > > > > > > I didn't benchmark it, but I'd assume using insertRule() would be > > > faster, > > > > > > > especially in the append scenario. IF you reassign textContent on > the > > > > other > > > > > > > hand, the browser has to parse and apply the whole CSS again. > > > > > > > > > > > > > > Also, if you assign to textContent, a text node is created, and the > > > > browser > > > > > > may > > > > > > > keep a copy of the raw CSS in memory, while with insertRule(), no > node > > > is > > > > > > added > > > > > > > to the DOM, and only the parsed representation needs to be kept in > > > memory. > > > > > > > > > > > > The problem with using `insertRule` here: we don't know that the style > > > sheet > > > > > is > > > > > > always going to be a single rule; we could split the style sheet along > > > > newline > > > > > > characters and treat each part as a rule, but then that's another > > > assumption > > > > > the > > > > > > content script would be making about the background page's logic. > > > > > > > > > > How about making the function in core return an array of rules (rather > > than > > > > > joining it by newlines in the first place) and the passing this array to > > the > > > > > content script? This seems to be the most sensible solution to me. > > > > > > > > That's not really an option, it would destroy the optimizations we have > done > > > for > > > > the most common cases. > > > > > > > > > > Also it would > > > > > > be a bit odd to user `insertRule` for appending and `textContent` for > > > > > > replacement. > > > > > > > > > > What I had in mind is the old logic we had in here, where we'd use > > > > insertRule() > > > > > in both cases (after using removeRule(), or if it's simpler just > recreate > > > the > > > > > <style> element, in case we aren't appending). > > > > > > > > Makes sense, but then again core can only return a style sheet. > > > > > > > > Unless you mean that `createStyleSheet` itself should return rules, but > then > > > > again I don't really understand what the benefit of that would be. In the > > > > content script, you would still have two code paths, one for the full > style > > > > sheet and one for a list of style rules. > > > > > > How many ms would that add to the bill if core will cache (and return) an > > array > > > of rules (which have to be joined every time we inject a user stylesheet) > > rather > > > than caching/returning a joined string? > > > > If you run the JS profiler in DevTools with the `getSelectorsForDomain` > version, > > go to the Bottom-Up tab, and open the V8 Runtime node in the tree, the first > > things you'll see there are DoJoin, Join, InnerArrayJoin, etc. The extension > > spends a significant amount of time just joining strings. We are avoiding the > > native join function by doing it manually [1], but it would still not be free. > > > > [1]: https://codereview.adblockplus.org/29892563/ > > > > > Perhaps, the optimization will be slightly less effective for platforms that > > > support user stylesheets, but I'd expect caching an array of rules still > being > > > more efficient than what we are doing now where the rules have to be > generated > > > as well every time (rather than just being joined). > > > > No, we can't avoid rule generation because the array of selectors for known > > domains is created afresh each time. Anyway, we've already made it more > complex > > than it was, I think the current implementation is most optimal for the most > > common cases. > > > > Why do we even want to change the implementation in core just to address a > > special case here, which is significantly older versions of Chrome and > Firefox, > > all versions of Edge, and _only_ for collapsing selectors. I'm OK with even > > disabling this feature for those browsers: the content script can send the > > injectSelectors message to the background page and not care if it actually > > worked. This would be the best option at this point. Alternatively, the > content > > script could just call `styleSheet.split("\n")` to get the rules. > > This doesn't seem to only be about element collapsing, only the append scenario > is, and yes that the whole CSS has to be parsed and applied again there is a > minor issue. > > However, the major issue here is that you assign to textContent (for element > collapsing as well for element hiding), which causes a text node with all the > injected CSS to be created, which isn't an option for following reasons: > > * A copy of the raw CSS text will be kept in memory, increasing the per-document > memory footprint. > * The website can detect changes we do to the CSS, since changing the > textContent triggered a MutationObserver while CSSOM cannot be listened for. > * As more as we add to the DOM, as higher the chance this will cause side > effects with random web applications. I even have a real world example: Outlook > 365 uses (or at least used to use) a subdocument with an <html contenteditable> > element for composing emails, and then serializes the DOM in in order to > generate the mail body. So everything we add to the DOM will end up in outgoing > emails. An empty <style> element would be relative harmless there, but adding > 600KB of redundant CSS text to each email is bad. > * While technically arguable, copyright trolls used to give ad blockers shit > based on how visible there interference with any website is in DOM shown in the > devtools. It might confuse other users of the devtools as well to see that long > list of selectors. If assigning `textContent` is an issue, one other option is to undo all the changes in include.preload.js and always pass selectors to the content script if user style sheets is not supported. This would still give the best user experience to people on modern browsers. Remember this whole issue is only because of Edge, otherwise we could have used author style sheets and this wouldn't even be a problem. Maybe we use author style sheets on Chrome and Firefox after all. In any case I think it makes no sense to undo the optimization for the most common case just for an edge case. Manish Jethani 2018/10/01 14:24:02 OK, I have a solution that would satisfy all cases Show quoted text On 2018/10/01 14:01:36, Manish Jethani wrote: > On 2018/09/30 14:19:24, Sebastian Noack wrote: > > On 2018/09/30 08:06:21, Manish Jethani wrote: > > > On 2018/09/29 21:01:49, Sebastian Noack wrote: > > > > On 2018/09/29 17:28:04, Manish Jethani wrote: > > > > > On 2018/09/29 16:39:30, Sebastian Noack wrote: > > > > > > On 2018/09/29 15:58:48, Manish Jethani wrote: > > > > > > > On 2018/09/29 14:36:13, Sebastian Noack wrote: > > > > > > > > I didn't benchmark it, but I'd assume using insertRule() would be > > > > faster, > > > > > > > > especially in the append scenario. IF you reassign textContent on > > the > > > > > other > > > > > > > > hand, the browser has to parse and apply the whole CSS again. > > > > > > > > > > > > > > > > Also, if you assign to textContent, a text node is created, and > the > > > > > browser > > > > > > > may > > > > > > > > keep a copy of the raw CSS in memory, while with insertRule(), no > > node > > > > is > > > > > > > added > > > > > > > > to the DOM, and only the parsed representation needs to be kept in > > > > memory. > > > > > > > > > > > > > > The problem with using `insertRule` here: we don't know that the > style > > > > sheet > > > > > > is > > > > > > > always going to be a single rule; we could split the style sheet > along > > > > > newline > > > > > > > characters and treat each part as a rule, but then that's another > > > > assumption > > > > > > the > > > > > > > content script would be making about the background page's logic. > > > > > > > > > > > > How about making the function in core return an array of rules (rather > > > than > > > > > > joining it by newlines in the first place) and the passing this array > to > > > the > > > > > > content script? This seems to be the most sensible solution to me. > > > > > > > > > > That's not really an option, it would destroy the optimizations we have > > done > > > > for > > > > > the most common cases. > > > > > > > > > > > > Also it would > > > > > > > be a bit odd to user `insertRule` for appending and `textContent` > for > > > > > > > replacement. > > > > > > > > > > > > What I had in mind is the old logic we had in here, where we'd use > > > > > insertRule() > > > > > > in both cases (after using removeRule(), or if it's simpler just > > recreate > > > > the > > > > > > <style> element, in case we aren't appending). > > > > > > > > > > Makes sense, but then again core can only return a style sheet. > > > > > > > > > > Unless you mean that `createStyleSheet` itself should return rules, but > > then > > > > > again I don't really understand what the benefit of that would be. In > the > > > > > content script, you would still have two code paths, one for the full > > style > > > > > sheet and one for a list of style rules. > > > > > > > > How many ms would that add to the bill if core will cache (and return) an > > > array > > > > of rules (which have to be joined every time we inject a user stylesheet) > > > rather > > > > than caching/returning a joined string? > > > > > > If you run the JS profiler in DevTools with the `getSelectorsForDomain` > > version, > > > go to the Bottom-Up tab, and open the V8 Runtime node in the tree, the first > > > things you'll see there are DoJoin, Join, InnerArrayJoin, etc. The extension > > > spends a significant amount of time just joining strings. We are avoiding > the > > > native join function by doing it manually [1], but it would still not be > free. > > > > > > [1]: https://codereview.adblockplus.org/29892563/ > > > > > > > Perhaps, the optimization will be slightly less effective for platforms > that > > > > support user stylesheets, but I'd expect caching an array of rules still > > being > > > > more efficient than what we are doing now where the rules have to be > > generated > > > > as well every time (rather than just being joined). > > > > > > No, we can't avoid rule generation because the array of selectors for known > > > domains is created afresh each time. Anyway, we've already made it more > > complex > > > than it was, I think the current implementation is most optimal for the most > > > common cases. > > > > > > Why do we even want to change the implementation in core just to address a > > > special case here, which is significantly older versions of Chrome and > > Firefox, > > > all versions of Edge, and _only_ for collapsing selectors. I'm OK with even > > > disabling this feature for those browsers: the content script can send the > > > injectSelectors message to the background page and not care if it actually > > > worked. This would be the best option at this point. Alternatively, the > > content > > > script could just call `styleSheet.split("\n")` to get the rules. > > > > This doesn't seem to only be about element collapsing, only the append > scenario > > is, and yes that the whole CSS has to be parsed and applied again there is a > > minor issue. > > > > However, the major issue here is that you assign to textContent (for element > > collapsing as well for element hiding), which causes a text node with all the > > injected CSS to be created, which isn't an option for following reasons: > > > > * A copy of the raw CSS text will be kept in memory, increasing the > per-document > > memory footprint. > > * The website can detect changes we do to the CSS, since changing the > > textContent triggered a MutationObserver while CSSOM cannot be listened for. > > * As more as we add to the DOM, as higher the chance this will cause side > > effects with random web applications. I even have a real world example: > Outlook > > 365 uses (or at least used to use) a subdocument with an <html > contenteditable> > > element for composing emails, and then serializes the DOM in in order to > > generate the mail body. So everything we add to the DOM will end up in > outgoing > > emails. An empty <style> element would be relative harmless there, but adding > > 600KB of redundant CSS text to each email is bad. > > * While technically arguable, copyright trolls used to give ad blockers shit > > based on how visible there interference with any website is in DOM shown in > the > > devtools. It might confuse other users of the devtools as well to see that > long > > list of selectors. > > If assigning `textContent` is an issue, one other option is to undo all the > changes in include.preload.js and always pass selectors to the content script if > user style sheets is not supported. This would still give the best user > experience to people on modern browsers. > > Remember this whole issue is only because of Edge, otherwise we could have used > author style sheets and this wouldn't even be a problem. Maybe we use author > style sheets on Chrome and Firefox after all. > > In any case I think it makes no sense to undo the optimization for the most > common case just for an edge case. OK, I have a solution that would satisfy all cases. We add a function to core: function splitStyleSheet(styleSheet) { ... } It will give you list of style rules. In the background page, use this function to return a list of rules instead of a sheet. Because splitting a string does not allocate new memory for each substring (only for the temporary array), this should be good for memory as well as satisfactory enough for the performance and other requirements for the non-user style sheets case. Does this sound reasonable? Sebastian Noack 2018/10/01 19:42:27 Sounds good! Show quoted text On 2018/10/01 14:24:02, Manish Jethani wrote: > OK, I have a solution that would satisfy all cases. > > We add a function to core: > > function splitStyleSheet(styleSheet) { ... } > > It will give you list of style rules. In the background page, use this function > to return a list of rules instead of a sheet. > > Because splitting a string does not allocate new memory for each substring (only > for the temporary array), this should be good for memory as well as satisfactory > enough for the performance and other requirements for the non-user style sheets > case. > > Does this sound reasonable? Sounds good!
444 // that would work depends on their sizes (e.g. "#foo .bar" has a	429 else

445 // size of 2). Since we don't know the sizes of the selectors here, we	430 style.textContent = styleSheet;

446 // simply split them into groups of 1,024, based on the reasonable

447 // assumption that the average selector won't have a size greater than 8.

448 // The alternative would be to calculate the sizes of the selectors and

449 // divide them up accordingly, but this approach is more efficient and has

450 // worked well in practice. In theory this could still lead to some

451 // selectors not working on Chromium, but it is highly unlikely.

452 // See issue #6298 and https://crbug.com/804179

453 for (let i = 0; i < selectors.length; i += this.selectorGroupSize)

454 {

455 let selector = selectors.slice(i, i + this.selectorGroupSize).join(", ");

456 style.sheet.insertRule(selector + "{display: none !important;}",

457 style.sheet.cssRules.length);

458 }

459 },	431 },

460	432

461 addSelectors(selectors, filters, groupName = "emulated", appendOnly = false)	433 addSelectors(selectors, groupName = "standard", appendOnly = false)

462 {	434 {

463 if (this.inline)	435 browser.runtime.sendMessage({

	436 type: "content.injectSelectors",

	437 selectors,

	438 groupName,

	439 appendOnly

	440 },

	441 styleSheet =>

464 {	442 {

465 // Insert the style rules inline if we have been instructed by the	443 if (styleSheet)

466 // background page to do so. This is usually the case, except on platforms	444 {

467 // that do support user stylesheets via the browser.tabs.insertCSS API	445 // Insert the style sheet inline if we have been instructed by the

468 // (Firefox 53 onwards for now and possibly Chrome in the near future).	446 // background page to do so. This is rarely the case, except on

469 // Once all supported platforms have implemented this API, we can remove	447 // platforms that do not support user stylesheets via the

470 // the code below. See issue #5090.	448 // browser.tabs.insertCSS API (Firefox <53, Chrome <66, and Edge).

471 // Related Chrome and Firefox issues:	449 // Once all supported platforms have implemented this API, we can remove

472 // https://bugs.chromium.org/p/chromium/issues/detail?id=632009	450 // the code below. See issue #5090.

473 // https://bugzilla.mozilla.org/show_bug.cgi?id=1310026	451 // Related Chrome and Firefox issues:

474 this.addSelectorsInline(selectors, groupName, appendOnly);	452 // https://bugs.chromium.org/p/chromium/issues/detail?id=632009

475 }	453 // https://bugzilla.mozilla.org/show_bug.cgi?id=1310026

476 else	454 this.addStyleSheetInline(styleSheet, groupName, appendOnly);

477 {	455 }

478 browser.runtime.sendMessage({	456 });

479 type: "content.injectSelectors",

480 selectors,

481 groupName,

482 appendOnly

483 });

484 }

485

486 // Only trace selectors that are based directly on hiding filters

487 // (i.e. leave out collapsing selectors).

488 if (this.tracer && groupName != "collapsing")

489 this.tracer.addSelectors(selectors, filters);
Sebastian Noack 2018/09/29 14:36:13 It seems ElementHidingTracer.addSelectors() is nev It seems ElementHidingTracer.addSelectors() is never called with a second argument now and the related logic can be removed now. Manish Jethani 2018/09/29 15:58:48 ElementHidingTracer is a whole pandora's box. If i Show quoted text On 2018/09/29 14:36:13, Sebastian Noack wrote: > It seems ElementHidingTracer.addSelectors() is never called with a second > argument now and the related logic can be removed now. ElementHidingTracer is a whole pandora's box. If it never gets any filters, we should clean up all the related code. I think that would have to be a separate change. Sebastian Noack 2018/09/29 16:39:30 What I was thinking of is this: diff -r ae52536f7 Show quoted text On 2018/09/29 15:58:48, Manish Jethani wrote: > On 2018/09/29 14:36:13, Sebastian Noack wrote: > > It seems ElementHidingTracer.addSelectors() is never called with a second > > argument now and the related logic can be removed now. > > ElementHidingTracer is a whole pandora's box. If it never gets any filters, we > should clean up all the related code. I think that would have to be a separate > change. What I was thinking of is this: diff -r ae52536f7389 include.preload.js --- a/include.preload.js Fri Sep 28 17:58:30 2018 +0200 +++ b/include.preload.js Sat Sep 29 18:23:52 2018 +0200 @@ -258,22 +258,19 @@ this.trace(); } ElementHidingTracer.prototype = { - addSelectors(selectors, filters) + addSelectors(selectors) { - let pairs = selectors.map((sel, i) => [sel, filters && filters[i]]); + if (document.readyState != "loading") + this.checkNodes([document], selectors); - if (document.readyState != "loading") - this.checkNodes([document], pairs); - - this.selectors.push(...pairs); + this.selectors.push(...selectors); }, - checkNodes(nodes, pairs) + checkNodes(nodes, selectors) { let selectors = []; - let filters = []; - for (let [selector, filter] of pairs) + for (let selector of selectors) { nodes: for (let node of nodes) { @@ -284,27 +281,19 @@ // priority, or haven't been circumvented in a different way. if (getComputedStyle(element).display == "none") { - // For regular element hiding, we don't know the exact filter, - // but the background page can find it with the given selector. - // In case of element hiding emulation, the generated selector - // we got here is different from the selector part of the filter, - // but in this case we can send the whole filter text instead. - if (filter) - filters.push(filter); - else - selectors.push(selector); - + selectors.push(selector); break nodes; } } } } - if (selectors.length > 0 \|\| filters.length > 0) + if (selectors.length > 0) { browser.runtime.sendMessage({ type: "hitLogger.traceElemHide", - selectors, filters + selectors, + filters: [] }); } }, But regardless of this simplification, after your changes, hitLogger.traceElemHide would only receive either filters or selectors (never both), and it would make sense to go a step further, splitting it up into two message types (which would also improve the performance of the devtools panel in particular when the filter text is known). I'm happy to just add the above change here, and save those changes to lib/hitLogger.js for another patch. Manish Jethani 2018/09/29 17:28:04 OK, but since I'm not the author of these changes Show quoted text On 2018/09/29 16:39:30, Sebastian Noack wrote: > [...] > > But regardless of this simplification, after your changes, > hitLogger.traceElemHide would only receive either filters or selectors (never > both), and it would make sense to go a step further, splitting it up into two > message types (which would also improve the performance of the devtools panel in > particular when the filter text is known). I'm happy to just add the above > change here, and save those changes to lib/hitLogger.js for another patch. OK, but since I'm not the author of these changes (nor have I been through them in detail yet), and since they really are somewhat unrelated to just generating the style sheets in the background page, maybe it should be a separate patch? Sebastian Noack 2018/09/29 21:01:49 Well, your adapting for core changes here that mak Show quoted text On 2018/09/29 17:28:04, Manish Jethani wrote: > On 2018/09/29 16:39:30, Sebastian Noack wrote: > > [...] > > > > But regardless of this simplification, after your changes, > > hitLogger.traceElemHide would only receive either filters or selectors (never > > both), and it would make sense to go a step further, splitting it up into two > > message types (which would also improve the performance of the devtools panel > in > > particular when the filter text is known). I'm happy to just add the above > > change here, and save those changes to lib/hitLogger.js for another patch. > > OK, but since I'm not the author of these changes (nor have I been through them > in detail yet), and since they really are somewhat unrelated to just generating > the style sheets in the background page, maybe it should be a separate patch? Well, your adapting for core changes here that make it redundant to pass filters along with the selectors. So I think I can expect you to remove any related redundant code, and the changes I'm asking for here are trivial. Manish Jethani 2018/09/30 08:36:38 The local `selectors` variable here needs to be re Show quoted text On 2018/09/29 16:39:30, Sebastian Noack wrote: > - checkNodes(nodes, pairs) > + checkNodes(nodes, selectors) > { > let selectors = []; The local `selectors` variable here needs to be renamed. I've renamed it to `effectiveSelectors`. Otherwise no other changes.
490 },	457 },

491	458

492 hideElements(elements, filters)	459 hideElements(elements, filters)

493 {	460 {

494 for (let element of elements)	461 for (let element of elements)

495 hideElement(element);	462 hideElement(element);

496	463

497 if (this.tracer)	464 if (this.tracer)

498 {	465 {

499 browser.runtime.sendMessage({	466 browser.runtime.sendMessage({

(...skipping 12 matching lines...) Expand all Loading...
512 },	479 },

513 response =>	480 response =>

514 {	481 {

515 if (this.tracer)	482 if (this.tracer)

516 this.tracer.disconnect();	483 this.tracer.disconnect();

517 this.tracer = null;	484 this.tracer = null;

518	485

519 if (response.trace)	486 if (response.trace)

520 this.tracer = new ElementHidingTracer();	487 this.tracer = new ElementHidingTracer();

521	488

522 this.inline = response.inline;	489 if (response.inline)

523	490 this.addStyleSheetInline(response.styleSheet.code);

524 if (this.inline)

525 this.addSelectorsInline(response.selectors, "standard");

526	491

527 if (this.tracer)	492 if (this.tracer)

528 this.tracer.addSelectors(response.selectors);	493 this.tracer.addSelectors(response.styleSheet.selectors);

529	494

530 this.elemHideEmulation.apply(response.emulatedPatterns);	495 this.elemHideEmulation.apply(response.emulatedPatterns);

531 });	496 });

532 }	497 }

533 };	498 };

534	499

535 if (document instanceof HTMLDocument)	500 if (document instanceof HTMLDocument)

536 {	501 {

537 checkSitekey();	502 checkSitekey();

538	503

(...skipping 10 matching lines...) Expand all Loading...
549 let element = event.target;	514 let element = event.target;

550 if (/^i?frame$/.test(element.localName))	515 if (/^i?frame$/.test(element.localName))

551 checkCollapse(element);	516 checkCollapse(element);

552 }, true);	517 }, true);

553 }	518 }

554	519

555 window.checkCollapse = checkCollapse;	520 window.checkCollapse = checkCollapse;

556 window.contentFiltering = contentFiltering;	521 window.contentFiltering = contentFiltering;

557 window.typeMap = typeMap;	522 window.typeMap = typeMap;

558 window.getURLsFromElement = getURLsFromElement;	523 window.getURLsFromElement = getURLsFromElement;

OLD	NEW

« no previous file with comments | « dependencies ('k') | lib/contentFiltering.js » ('j') | no next file with comments »