Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#521 closed change (fixed)

Inject our stylesheet on per-site basis rather than globally

Reported by: trev Assignee: trev
Priority: P3 Milestone: Adblock-Plus-2.8-for-Firefox
Module: Adblock-Plus-for-Firefox Keywords:
Cc: fhd, arthur, greiner, mapx, saroyanm, Compuitguy, trev, famlam Blocked By: #4139, #4221, #4227, #4500
Blocking: #524 Platform: Firefox
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

http://codereview.adblockplus.org/6201308310667264/
https://codereview.adblockplus.org/29346613/

Description (last modified by trev)

Background

Currently we register a large global stylesheet in Firefox, it is being applied to every single webpage. According to https://bugzilla.mozilla.org/show_bug.cgi?id=988266 there is no memory use advantage however, essentially a copy of this stylesheet is being injected into each document.

What to change

We can stick to a single stylesheet file for now and merely decide individually on whether we want to inject it into a webpage. We can listen to some notification like content-document-global-created and use nsIDOMWindowUtils.loadSheet() to inject the stylesheet. This will solve several issues:

  • No stylesheet is injected into whitelisted pages and pages where element hiding is disabled - meaning also no side-effects from element hiding that were unavoidable so far.
  • The ugly protocol whitelist for generic rules can go, we simply won't inject the stylesheet into chrome:// documents and such.
  • More consistent behavior - changes to element hiding rules apply only after page reload, same as with other filters.
  • No hang when Adblock Plus is enabled or disabled globally coming from the stylesheet being injected into all open tabs simultaneously.
  • We can get rid of the current ugly approach to implement exceptions via -moz-binding once bug 349813 is fixed.

Change History (52)

comment:1 Changed 5 years ago by tschuster

I will try to prototype this.

comment:2 Changed 5 years ago by trev

  • Description modified (diff)

comment:3 Changed 5 years ago by trev

  • Owner set to tschuster

comment:4 follow-up: Changed 5 years ago by tschuster

  • The ugly protocol whitelist for generic rules can go, we simply won't inject the stylesheet into chrome:// documents and such.

I think if we use "content-document-global-created" we are guaranteed not to inject into chrome :)

  • More consistent behavior - changes to element hiding rules apply only after page reload, same as with other filters.

Perfect, if we had to apply changes immediately this would turn out be rather difficult.

comment:5 Changed 5 years ago by mapx

  • Cc smultron45@… added

comment:6 in reply to: ↑ 4 Changed 5 years ago by trev

Replying to tschuster:

I think if we use "content-document-global-created" we are guaranteed not to inject into chrome :)

content-document-global-created is about the distinction between chrome and content meaning which side of the security boundary you are on - it isn't about privileges. Privileged documents can be loaded into the content area (e.g. Add-ons Manager page).

comment:7 Changed 5 years ago by trev

  • Blocking 524 added

comment:8 Changed 5 years ago by tschuster

  • Review URL(s) modified (diff)

comment:9 Changed 5 years ago by tschuster

I just uploaded a patch for this. It seems to work correctly for me, but probably not a final version.

comment:10 Changed 5 years ago by philll

By tschuster:
So, just to update this bug. I recently uploaded a version of this patch that is almost finished. Sadly this regresses memory usage, because now we have more style-sheet memory per window. I talked to bz about this and he suggested that we add an API to Gecko, which would allow us to cache one global stylesheet and just load a reference to it in every window. I want to spend some time to figure out if this is something I can do easily. That new API should put as at slightly better memory usage. And as a next step we can probably split the css in two, one global css that gets loaded in every window and one css with just the domain specific rules, if they exist.

comment:11 Changed 5 years ago by trev

  • Blocking 717 added

comment:12 Changed 5 years ago by trev

  • Component changed from Extensions-for-Adblock-Plus to Adblock-Plus-for-Firefox

comment:13 Changed 5 years ago by trev

  • Blocking 717 removed

comment:14 Changed 5 years ago by arthur

  • Platform set to Unknown

Would this fix the follwing issue?

  1. Update EasyList
  2. Add these 3 filters: ##.banner300x250, ##.banner300x100 & ###sponsored-ad
  3. Go to this page and notice the anti adblock message
  4. Add these 3 whitelists: rai.tv#@#.banner300x250, rai.tv#@#.banner300x100 & rai.tv#@##sponsored-ad
  5. Reload the page and notice the anti adblock message still appears
  6. Remove the filters and whitelists and notice the message doesn't show up, the video appears instead

This doesn't happen in Chrome. It works when you add the whitelists.

comment:15 Changed 5 years ago by trev

Yes, it would fix that issue.

comment:16 Changed 5 years ago by philll

  • Platform changed from Unknown to Firefox

comment:17 Changed 5 years ago by saroyanm

  • Cc manvel@… added

comment:18 Changed 5 years ago by arthur

  • Cc arthur added

comment:19 Changed 5 years ago by arthur

Is it possible to estimate when this will be ready? In order to counteract a certain video fallback ad mechanism that is being used on some big German sites I have to disable generic element hiding filters because an element hiding whitelist doesn't work as it should in Firefox.

comment:20 Changed 5 years ago by tschuster

It would be interested in understanding how my patch helps there. Anyway I just uploaded a new version. I am not sure when it's read, this depends on what Wladimir or anybody else finds during review and how complicated this is to address.

comment:21 Changed 5 years ago by archon810

Looks like https://adblockplus.org/forum/viewtopic.php?f=2&t=25867&p=109480#p109480 is affected by this bug. Is there any idea if it's going to be fixed any time soon please?

Thank you.

comment:22 Changed 4 years ago by archon810

Hi,

Any plans to fix this issue? It's still affecting the plugin and our sites.

Thank you.

comment:23 Changed 4 years ago by greiner

  • Cc greiner added

comment:24 Changed 4 years ago by mapx

  • Cc mapx saroyanm added; smultron45@… manvel@… removed

comment:25 Changed 4 years ago by Compuitguy

How can we vote/cc for this ?

comment:26 Changed 4 years ago by mapx

it's under review (last activity 8 months ago ...) https://codereview.adblockplus.org/6201308310667264/

comment:27 Changed 4 years ago by Compuitguy

Yes, saw that

So there is no possibility to vote/cc for issues ?

comment:28 Changed 4 years ago by mapx

  • Cc Compuitguy added

comment:29 Changed 4 years ago by Compuitguy

Thanks mapx

BTW,

There is progress on this bugzilla issue which is related

77999 – share rule processors for XUL, scoped, UA, and user stylesheets

comment:30 Changed 4 years ago by Compuitguy

77999 – share rule processors for XUL, scoped, UA, and user stylesheets

Status: RESOLVED FIXED

Target Milestone: mozilla41

From the bug:

ABP is enabled, using its default blocklist, we save a bit over 2 MB per document. (This is for every displayed document, including iframes, except one, since we are now sharing this data between documents.)

Without ABP, we just have the standard UA style sheets that Firefox adds to every document, and in that case we save about 120 KB per document.

We also save the time required to build up these tables, but I haven't measured that.

comment:31 Changed 4 years ago by trev

  • Cc trev added
  • Ready unset
  • Tester set to Unknown

@tschuster: Does fixing this still make sense given that bug 77999 has been resolved?

comment:32 Changed 4 years ago by tschuster

  • Resolution set to rejected
  • Status changed from new to closed

Not really. I see no way this is going to reduce memory below that what we have after fixing bug 77999.

comment:33 Changed 4 years ago by arthur

Still there is the issue where Google ads embedded with the responsive Adsense code will not show up if you disable Adblock Plus for a specific domain (e. g. http://www.thewildernist.org/2015/07/history-bison-southeastern-north-america/, tested with Nightly 42.0a1 (20150723030207) and Adblock Plus 2.6.9.3966 with EasyList only). Whitelisting those in Acceptable Ads in Firefox doesn't work then.

Should I create a new issue for that?

comment:34 Changed 4 years ago by mapx

still:

test using 42.0a2 (2015-08-15)
tested also in 41.0b1

go to http://apkone.net/
now you should add:
@@.png#$domain=apkone.net

keeping easylist enabled does not work
disabling easylist and adding easylist without hiding filters ==> will work

Last edited 4 years ago by mapx (previous) (diff)

comment:35 Changed 4 years ago by mapx

  • Resolution rejected deleted
  • Status changed from closed to reopened

comment:36 Changed 4 years ago by jobp

  • Cc fam.lam@… added

comment:37 Changed 4 years ago by arthur

  • Cc famlam added; fam.lam@… removed

comment:38 Changed 4 years ago by tschuster

"keeping easylist enabled does not work" What do you mean exactly?
When visting this site I get some kind of warning about using an Adblocker, which makes this site unusable. Is it that?

comment:39 Changed 4 years ago by mapx

if you use easylist and add the new filter
@@.png#$domain=apkone.net

the site will work fine in ABP for chrome but not in ABP for firefox and this due to the old bug with stylesheets. It will work only if you use easylist without hiding filters. And this even if in easylist we have the whitelisting filter

@@||apkone.net^$elemhide

see also https://issues.adblockplus.org/ticket/521#comment:14

and the comments of Wladimir (not only here but also on blog) which indicates this issue as a consequence of 521

Last edited 4 years ago by mapx (previous) (diff)

comment:41 follow-up: Changed 4 years ago by tschuster

Mhm I guess per-site filtering would probably fix this, but this doesn't mean we can't fix it in another way. I will look into it.

comment:42 in reply to: ↑ 41 Changed 3 years ago by mapx

Replying to tschuster:

Mhm I guess per-site filtering would probably fix this, but this doesn't mean we can't fix it in another way. I will look into it.

still nothing ? very old bug.

another test case:
http://www.cinema2satu.net/

the filters already in easylist are working fine in ABP for chrome
in firefox still present the anti adblock message
tested also adding

@@||cinema2satu.net^$generichide

does not work.

It's working only switching to easylist without element hiding.

comment:43 Changed 3 years ago by trev

  • Priority changed from P2 to P3
  • Ready set

We still want to tackle this but I think P3 is more realistic.

comment:44 Changed 3 years ago by trev

  • Blocked By 4139 added
  • Owner changed from tschuster to trev

Tom, hope you don't mind me taking this. I've done some preparation work for this and #524 in #4139, it should be fairly simple now.

comment:45 Changed 3 years ago by trev

  • Review URL(s) modified (diff)
  • Status changed from reopened to reviewing

comment:46 Changed 3 years ago by abpbot

comment:47 Changed 3 years ago by trev

  • Milestone set to Adblock-Plus-for-Firefox-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:48 Changed 3 years ago by mapx

error in console on START (effect: the hiding filters are not working)

NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIDOMWindowUtils.addSheet]

row code error (elemhide.js:346)

        utils.addSheet(this.sheet, Ci.nsIStyleSheetService.USER_SHEET);
Last edited 3 years ago by mapx (previous) (diff)

comment:49 Changed 3 years ago by trev

  • Blocked By 4221 added

comment:50 Changed 3 years ago by trev

  • Blocked By 4227 added

comment:51 Changed 3 years ago by trev

@mapx: I created #4227 for this regression.

comment:52 Changed 3 years ago by trev

  • Blocked By 4500 added
Note: See TracTickets for help on using tickets.