Opened on 03/20/2018 at 04:14:23 PM

Closed on 09/04/2018 at 08:57:04 AM

#6507 closed change (rejected)

Inject style sheet proactively from background page

Reported by: mjethani Assignee: mjethani
Priority: Unknown Milestone:
Module: Platform Keywords: circumvention, externaldependency
Cc: sebastian, kzar Blocked By: #242, #5090
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29738592/

Description (last modified by mjethani)

Background

The background page waits for the content script to send a message before it can inject the style sheet for element hiding. The message itself contains no information. Now with user style sheets (#5090, #242) the background page doesn't have to wait for the content script before injecting the style sheet, it can inject the style sheet preemptively if tabs.insertCSS with cssOrigin is supported.

The benefit of doing this mainly is for anti-circumvention.

Chromium does not run the content script on sandboxed about: frames, because they have a unique origin (this is questionable but it is the current behavior). Even after crrev.com/c/968681, it is not going to run content scripts in sandboxed data: frames.

In other words, if we have a filter ###ad and the following HTML code:

<iframe srcdoc="<div id='ad'>ANNOYING AD!!!</div>" sandbox></iframe>

That div#ad inside the <iframe> will not get hidden.

While currently neither Chromium nor Firefox will allow tabs.insertCSS on sandboxed about: and data: frames, it might be possible to convince both browsers to allow CSS (even if not JavaScript) to be injected into such frames.

What to change

Undecided.

Resolution

With the changes for snippets (#6539, #6782), we significantly refactored the code in lib/cssInjection.js (renaming it to lib/contentFiltering.js). The issue of injecting code as early as possible, as well as into about: and data: frames, is a lot more relevant for snippets than for element hiding. Since we have gained more knowledge and experience about this and are now considering the contentScripts API on Firefox (and possibly Chrome in the future) (#6826), it doesn't make sense to make this change any more.

Attachments (0)

Change History (7)

comment:1 Changed on 03/20/2018 at 04:15:11 PM by mjethani

I think we could use webNavigation.onCommitted to inject the style sheet early.

comment:2 Changed on 03/20/2018 at 04:15:31 PM by mjethani

  • Cc sebastian kzar added

comment:3 follow-up: Changed on 03/20/2018 at 09:29:36 PM by sebastian

Sounds like a good idea generally. Besides the circumvention mitigation aspect, it also avoids a round trip from the content script to the background page. I'm just wondering about the timing; onCommitted is supposed to be emitted when the browser switched to showing the new document. If the CSS isn't already injected by then, elements might flash up for a short amount of time before they get hidden. At least we should make sure that this isn't getting worse than with our current approach.

comment:4 Changed on 03/22/2018 at 02:24:20 PM by mjethani

  • Owner set to mjethani

comment:5 in reply to: ↑ 3 Changed on 04/06/2018 at 06:40:07 AM by mjethani

Replying to sebastian:

I'm just wondering about the timing; onCommitted is supposed to be emitted when the browser switched to showing the new document. If the CSS isn't already injected by then, elements might flash up for a short amount of time before they get hidden. At least we should make sure that this isn't getting worse than with our current approach.

I checked, and sometimes the onCommitted handler is called before the content script is loaded (rarely, but it happens) and usually it is called after. In any case, onCommitted comes before the content script's message reaches the background page. This should only make the situation better, not worse, at least for the best case (modern browsers with user style sheets support). Unfortunately sometimes onCommitted happens before the content script is even loaded, in that case we need to add a delay before we contact the content script. I think that's acceptable. It only happens rarely, and it affects only older browsers and DevTools.

comment:6 Changed on 04/06/2018 at 06:40:51 AM by mjethani

  • Review URL(s) modified (diff)

comment:7 Changed on 09/04/2018 at 08:57:04 AM by mjethani

  • Description modified (diff)
  • Resolution set to rejected
  • Status changed from new to closed

Add Comment

Modify Ticket

Change Properties
Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from mjethani.
 
Note: See TracTickets for help on using tickets.