Bug 930787 (e10s-abp)

[e10s] Adblock Plus addon support for e10s

RESOLVED FIXED in Firefox 33

Status

()

defect
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: cpeterson, Unassigned)

Tracking

(Depends on 4 bugs, Blocks 1 bug, {addon-compat, meta})

unspecified
Firefox 33
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s-)

Details

()

WFM, AFAICT :)
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WORKSFORME
I'd like to keep this open to track the changes needed to convert adblock to use the message manager. It will make adblock faster and also fix some bugs. For example, "Open blockable items" isn't likely to work in e10s now.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
What's the timeline for this, could you point me to some documentation? From my previous experience with e10s, the required effort (both implementation and subsequent maintenance) will be massive because much of the code needs to be duplicated. This isn't something that can be done in a few hours so knowing how urgent it is would help a lot.
(In reply to Wladimir Palant from comment #3)
> What's the timeline for this, could you point me to some documentation? From
> my previous experience with e10s, the required effort (both implementation
> and subsequent maintenance) will be massive because much of the code needs
> to be duplicated. This isn't something that can be done in a few hours so
> knowing how urgent it is would help a lot.

Wladimar: e10s support is not urgent. e10s development will continue for much of 2014. So no hurries or worries! :)

I'm working with MDN to clean up our documentation, but here are some relevant links about the Message Manager (for async messaging between chrome and content processes) and Cross-Process Object Wrappers (CPOWs) for special cases that requite synchronous messages:

http://timtaubert.de/blog/2011/08/firefox-electrolysis-101/

https://developer.mozilla.org/en-US/docs/The_message_manager

https://developer.mozilla.org/en-US/docs/Cross_Process_Object_Wrappers
tracking-e10s: --- → +
Depends on: 997448
Depends on: 1007978
Depends on: 1007982
Depends on: 978961
I'm going to close this. All the dependent bugs are fixed. If there's any more Adblock-related work to be done, we can open a new bug.
Status: REOPENED → RESOLVED
Closed: 6 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Blocks: 1048057
ABP is listed as "it works" on http://arewee10syet.com/

I tested FF Nightly 34.0a1 (2014-08-21), linux x64 with both ABP 2.6.4 and their current development build and browser.tabs.remote.autostart = true.

In neither case the "open blockable items" view showed any entries after loading a page.
Disabling e10s restored functionality.
Depends on: 1057467
Thanks, 8472. I opened a new bug to track the issue you found: bug 1057467
(In reply to The 8472 from comment #6)
> ABP is listed as "it works" on http://arewee10syet.com/
> 
> I tested FF Nightly 34.0a1 (2014-08-21), linux x64 with both ABP 2.6.4 and
> their current development build and browser.tabs.remote.autostart = true.
> 
> In neither case the "open blockable items" view showed any entries after
> loading a page.
> Disabling e10s restored functionality.

Erm, this isn't "Fixed" at all. While it may not be crashing, ABP doesn't actually do anything (It doesn't block any ads, which is kinda it's raison d'être). It should definitely NOT be listed as "It works" on arewee10syet.com (as it doesn't). Can we re-open this, or should this discussion continue in #1057467 (and if so, shouldn't arewee10syet.com be updates to point to that bug?)
Robert: what websites are you testing? What version of ABP are you running? ABP 2.6.4 with e10s enabled works as expected for me on cnn.com and nytimes.com.
(In reply to Chris Peterson (:cpeterson) from comment #9)
> Robert: what websites are you testing? What version of ABP are you running?
> ABP 2.6.4 with e10s enabled works as expected for me on cnn.com and
> nytimes.com.

Here's a side-by-side view of CNN.com, left is e10s (Note the underline), right is normal. Note the "Death Row Stories" ad on the e10s version that's blocked on the normal one. Same thing happens on every other page I come across. Normal blocks the ads, e10s doesn't.

https://www.dropbox.com/s/7uq7palwcykqspj/Screenshot%202014-09-02%2013.08.27.png?dl=0

As mentioned, this is using ABP version 2.6.4.3861
(In reply to Robert Johnston from comment #10)
> Here's a side-by-side view of CNN.com, left is e10s (Note the underline),
> right is normal. Note the "Death Row Stories" ad on the e10s version that's
> blocked on the normal one. Same thing happens on every other page I come
> across. Normal blocks the ads, e10s doesn't.

Which Adblock Plus filter lists are you using? When I installed the ABP development version (2.6.4.3861) over the stable version, some of my filter lists (like EasyList) seemed to have gotten disabled. But when I created a new, clean profile and installed the ABP development version, the EasyList was enabled as the default filter list, as expected. Can you verify that your filter lists are enabled?

> As mentioned, this is using ABP version 2.6.4.3861

oh, you hadn't mentioned you were testing the development version. Can you reproduce the problem with the stable version (2.6.4) published on addons.mozilla.org? Or in a new profile?
(In reply to Chris Peterson (:cpeterson) from comment #11)
> (In reply to Robert Johnston from comment #10)
> > Here's a side-by-side view of CNN.com, left is e10s (Note the underline),
> > right is normal. Note the "Death Row Stories" ad on the e10s version that's
> > blocked on the normal one. Same thing happens on every other page I come
> > across. Normal blocks the ads, e10s doesn't.
> 
> Which Adblock Plus filter lists are you using? When I installed the ABP
> development version (2.6.4.3861) over the stable version, some of my filter
> lists (like EasyList) seemed to have gotten disabled. But when I created a
> new, clean profile and installed the ABP development version, the EasyList
> was enabled as the default filter list, as expected. Can you verify that
> your filter lists are enabled?

Both lists (Easylist and Warning Removal List) are enabled and working. As mentioned, this was the same Firefox session running with 2 windows on a dual monitor computer, one using e10s and one not. It's not cut together, both windows were open at the same time, in the same browser, on the same page. the only difference was that one had e10s enabled, and the other didn't.

> > As mentioned, this is using ABP version 2.6.4.3861
> 
> oh, you hadn't mentioned you were testing the development version. Can you
> reproduce the problem with the stable version (2.6.4) published on
> addons.mozilla.org? Or in a new profile?

I was using 2.6.4 stable and had this issue. I figured that the development version would fix it, or at least be a little more up-to-date so any other issues wouldn't be interfering.
(In reply to Robert Johnston from comment #12)
> Both lists (Easylist and Warning Removal List) are enabled and working. As
> mentioned, this was the same Firefox session running with 2 windows on a
> dual monitor computer, one using e10s and one not. It's not cut together,
> both windows were open at the same time, in the same browser, on the same
> page. the only difference was that one had e10s enabled, and the other
> didn't.

To enable "full e10s", you must set the about:config pref "browser.tabs.remote.autostart" to true and restart Nightly. The "File > Open e10s Window" menu item does open a new e10s process, but it does not enable the add-on compatibility checks that "browser.tabs.remote.autostart" pref does. We should probably remove the "Open e10s Window" menu item because it is old test code.
(In reply to Chris Peterson (:cpeterson) from comment #13)
> (In reply to Robert Johnston from comment #12)
> > Both lists (Easylist and Warning Removal List) are enabled and working. As
> > mentioned, this was the same Firefox session running with 2 windows on a
> > dual monitor computer, one using e10s and one not. It's not cut together,
> > both windows were open at the same time, in the same browser, on the same
> > page. the only difference was that one had e10s enabled, and the other
> > didn't.
> 
> To enable "full e10s", you must set the about:config pref
> "browser.tabs.remote.autostart" to true and restart Nightly. The "File >
> Open e10s Window" menu item does open a new e10s process, but it does not
> enable the add-on compatibility checks that "browser.tabs.remote.autostart"
> pref does. We should probably remove the "Open e10s Window" menu item
> because it is old test code.

I was actually setting that pref to true and opening a new window. However, toggling that preference and restarting the browser leads to exactly the same situation - When e10s is enabled (browser.tabs.remote.autostart = true), ads are not blocked. When e10s is disabled (browser.tabs.remote.autostart = false), ads are blocked as usual.
For me, ads are being blocked with |browser.tabs.remote.autostart = true|. However, Blockable items are broken. The following line triggers "unable to modify interposed property content" message:

>  Object.defineProperty(window, "content", { get: () => getBrowser(mainWin).contentWindow });

Note that this code isn't running in the browser window - it's a frame inside the browser window, one where the content property normally shouldn't be defined. With e10s enabled it is defined however (value is null) and apparently cannot be redefined.
I filed https://issues.adblockplus.org/ticket/1301 to clean up this code and stop faking window.content property. However, the priority of the issue is very low. I can raise it if not being able to redefine window.content isn't considered an issue worth fixing on Mozilla's end.
If it helps (it probably won't), the Browser Console when opening cnn.com with e10s enabled looks like this:

NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties.get] FileUtils.jsm:63
Permission denied to pass object to chrome
Permission denied to pass object to chrome
uncaught exception: undefined
uncaught exception: undefined
Permission denied to pass object to chrome
uncaught exception: undefined
Permission denied to pass object to chrome
uncaught exception: undefined
TypeError: window.safari is undefined safaripush.js:102
Permission denied to pass object to chrome
uncaught exception: undefined
Permission denied to pass object to chrome
Permission denied to pass object to chrome
uncaught exception: undefined
Permission denied to pass object to chrome
Permission denied to pass object to chrome
Permission denied to pass object to chrome
Permission denied to pass object to chrome
uncaught exception: undefined
Permission denied to pass object to chrome
uncaught exception: undefined
uncaught exception: undefined
uncaught exception: undefined
Permission denied to pass object to chrome
Permission denied to pass object to chrome
uncaught exception: undefined
Permission denied to pass object to chrome
uncaught exception: undefined
uncaught exception: undefined
Permission denied to pass object to chrome
Permission denied to pass object to chrome
Use of Mutation Events is deprecated. Use MutationObserver instead. vrs.js:1
uncaught exception: undefined
Permission denied to pass object to chrome
uncaught exception: undefined
Permission denied to pass object to chrome
uncaught exception: undefined
Permission denied to pass object to chrome
uncaught exception: undefined
Permission denied to pass object to chrome
uncaught exception: undefined
Error: Permission denied to access property 'toString'
Permission denied to pass object to chrome
uncaught exception: undefined
Permission denied to pass object to chrome
uncaught exception: undefined
Permission denied to pass object to chrome
uncaught exception: undefined
ReferenceError: cnnad_id is not defined hplib-min.js:23
Permission denied to pass object to chrome
uncaught exception: undefined
Permission denied to pass object to chrome
uncaught exception: undefined
Use of getPreventDefault() is deprecated.  Use defaultPrevented instead. jquery-1.7.2.min.js:3
Empty string passed to getElementById(). hplib-min.js:11
Permission denied to pass object to chrome
Permission denied to pass object to chrome
Permission denied to pass object to chrome
uncaught exception: undefined
Permission denied to pass object to chrome
Permission denied to pass object to chrome
uncaught exception: undefined
Permission denied to pass object to chrome
uncaught exception: undefined
Permission denied to pass object to chrome
uncaught exception: undefined
Permission denied to pass object to chrome
uncaught exception: undefined

Doing the same with e10s disabled produces the following:

content is null tab-events.js:40
Using //@ to indicate sourceMappingURL pragmas is deprecated. Use //# instead jquery-1.10.2.min.js:1
TypeError: window.safari is undefined safaripush.js:102
content is null tab-events.js:40
Using //@ to indicate sourceMappingURL pragmas is deprecated. Use //# instead jquery-1.10.2.min.js:1
Use of Mutation Events is deprecated. Use MutationObserver instead. vrs.js:1
content is null tab-events.js:40
Use of getPreventDefault() is deprecated.  Use defaultPrevented instead. jquery-1.7.2.min.js:3
"repainting..." hplib-min.js:23
"ad-48df790ef051ce72" hplib-min.js:23
"ad-76fb9165d4dc83bd" hplib-min.js:23
"ad-9c86913b431b114f" hplib-min.js:23
"ad-3ba9728dd75a6f0e" hplib-min.js:23
"ad-977b69f1d2ca7a72" hplib-min.js:23
"ad-691428c40a9e49e6" hplib-min.js:23
"ad-60ea2f29a4a38689" hplib-min.js:23
"ad-f9de57574daeedc1" hplib-min.js:23
"ad-f72763bd1282af92" hplib-min.js:23
"ad-6310e5bc3ba9cc0f" hplib-min.js:23
content is null tab-events.js:40
Empty string passed to getElementById(). hplib-min.js:11
content is null tab-events.js:40
Using //@ to indicate sourceMappingURL pragmas is deprecated. Use //# instead jquery-1.10.2.min.js:1
content is null tab-events.js:40
Using //@ to indicate sourceMappingURL pragmas is deprecated. Use //# instead jquery-1.10.2.min.js:1
content is null tab-events.js:40
Using //@ to indicate sourceMappingURL pragmas is deprecated. Use //# instead jquery-1.10.2.min.js:1
content is null tab-events.js:40
Using //@ to indicate sourceMappingURL pragmas is deprecated. Use //# instead jquery-1.10.2.min.js:1
content is null tab-events.js:40
Using //@ to indicate sourceMappingURL pragmas is deprecated. Use //# instead jquery-1.10.2.min.js:1
content is null tab-events.js:40
This bug should be reopened as it does not function with e10s enabled.
Yep, not working with Nightly 35.0a1 (2014-09-11), element hiding works but blocking does not, see http://simple-adblock.com/faq/testing-your-adblocker/ (matching with my personal experience, some things are blocked, others are not)
This bug is a meta bug. Let's track this regression in bug 1042710.
Alias: e10s-abp
Depends on: 1042710
I filed bug 1066837 to fix the problem in comment 16.

I'm not sure what to do about these other reports. I can't reproduce them myself. If you're having problems, can you try a fresh profile? Then we could try to figure out what exactly is causing the problem.
@billm - good point, checked back today, it works at home on my Linux (all green on testing-your-adblocker and a clean spiegel.de). going to try a new profile on Monday on Windows in the office and report back. (I'll keep a copy of the old profile for further investigations, though)
(In reply to Bill McCloskey (:billm) from comment #21)
> I filed bug 1066837 to fix the problem in comment 16.
> 
> I'm not sure what to do about these other reports. I can't reproduce them
> myself. If you're having problems, can you try a fresh profile? Then we
> could try to figure out what exactly is causing the problem.

I just poked at it again and found something interesting:

- Turn on e10s by default. Restart. Works, Ads are blocked.

- Turn off e10s, open up an e10s window. Ads are not blocked.

Reproducible every time.
(In reply to Leman Bennett [Omega] from comment #23)
> (In reply to Bill McCloskey (:billm) from comment #21)
> > I filed bug 1066837 to fix the problem in comment 16.
> > 
> > I'm not sure what to do about these other reports. I can't reproduce them
> > myself. If you're having problems, can you try a fresh profile? Then we
> > could try to figure out what exactly is causing the problem.
> 
> I just poked at it again and found something interesting:
> 
> - Turn on e10s by default. Restart. Works, Ads are blocked.
> 
> - Turn off e10s, open up an e10s window. Ads are not blocked.
> 
> Reproducible every time.

Yes, please see comment 13. That is a known issue.
Depends on: 1066729
New profile solved it for me.
Cheers, A.
Depends on: 1105516
Re-opening. Telemetry indicates that adp regularly throws an exception in the file contentPolicy.js line 404.

This makes up nearly a fifth of all reported addon js exceptions.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
We are also seeing contentPolicy.js line 625 throw an exception.
The file objectTabs.js line 302 also throws as well.
objectTabs.js also throws an exception on line 364.
Depends on: 1118457
Depends on: 1118459
Depends on: 1118462
Depends on: 1118465
tracking-e10s: --- → -
Depends on: 1151335
Depends on: 1153387
Depends on: 1156124
it no longer blocks the pub
I confirm that Ad Block Plus does not block anything on Firefox 40 (currently Aurora) with e10s activated.
Confirm that ABP works with latest Nightly.
(In reply to laz2727 from comment #32)
> Confirm that ABP works with latest Nightly.

Yep it works for me ;)
Since this is the meta bug for Adblock Block Plus adding some tracking information:

Assignee: :tracy
Link to add-on: https://addons.mozilla.org/en-US/firefox/addon/adblock-plus/?src=ss
Contact info for add-on: https://addons.mozilla.org/en-US/firefox/user/Wladimir-Palant/
- http://adblockplus.org/
Bug #: This bug 930787 and depends bugs
Add-on ID: {d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d}
How well does it work?: 60%
See depends bugs
Any obvious performance problems? likely to have performance problems as this uses CPOWs heavily.
Chromium version: https://chrome.google.com/webstore/detail/adblock-plus/cfhdojbkjhnklbpkdaibdccddilifddb
Depends on: 1186548
No longer depends on: 1186548
Depends on: 1187099
Depends on: 1192585
Depends on: 1228093
No longer depends on: 1228093
Tracy, a new version of Adblock was released that should have much better performance. Could you go through the performance-related dependencies here and close them if you can't reproduce?
Flags: needinfo?(twalker)
I'll look at the dependencies, but within a minute or so of installing ABP 2.7, on latest Nightly, I got the "Adblock Plus might be making Nightly run slowly" warning bar.
> I'll look at the dependencies, but within a minute or so of installing ABP 2.7, on latest Nightly, I got the "Adblock Plus might be making Nightly run slowly" warning bar.

I'm still getting the slow addon warning all the time for ABP.
Hmm, that suggests a false positive in the slow add-on warning. As far as I know, Adblock Plus 2.7 doesn't use any CPOWs unless you do some unusual things.

David, can you look into this?
Flags: needinfo?(dteller)
Looks like the slow addon warning isn't triggered if you disable the "Count filter hits" option (which is enabled by default).
I haven't used Firefox enough since I changed that pref, so I can't really say for certain.
Yes, it appears the remaining perf issue is centered on the "Count filter hits" option. Disabling it dramatically improves page load performance at reported sites like http://www.jshine.net/astronomy/dark_sky/ (bug 1156124) and https://en.wikipedia.org/wiki/List_of_country_calling_codes (bug 1192585)
Flags: needinfo?(twalker)
(In reply to Marco Castelluccio [:marco] from comment #39)
> Looks like the slow addon warning isn't triggered if you disable the "Count
> filter hits" option (which is enabled by default).
> I haven't used Firefox enough since I changed that pref, so I can't really
> say for certain.

I just got the slow addon warning again, even with "Coult filter hits" disabled.
Firefox is noticeably faster at loading pages anyway with that option disabled.
(In reply to [:tracy] Tracy Walker - QA Mentor from comment #40)
> Yes, it appears the remaining perf issue is centered on the "Count filter
> hits" option. Disabling it dramatically improves page load performance at
> reported sites like http://www.jshine.net/astronomy/dark_sky/ (bug 1156124)
> and https://en.wikipedia.org/wiki/List_of_country_calling_codes (bug 1192585)

This is weird, the code path dependent on this setting never leaves the chrome process - no additional IPC traffic, no reason why it should be different with E10S. It does cause additional I/O however, particularly on sites with many hits, so my immediate suspect would be OS.File being significantly slower with E10S for some reason. I'll look into this.
I filed https://issues.adblockplus.org/ticket/3473 on this issue (not related to E10S at all), it should be resolved soon. Adblock Plus 2.7.1 is to be released on January 19, by then we will hopefully stop using shims altogether.
From the top of my head, I can't think of any reason why OS.File would be slower than e10s, but hey, it's a brand new world in which we have little experience so far. Wladimir, if you find anything, I'm definitely interested. Note that it doesn't have to be a CPOW to cause the slow add-on warning, just a large use of CPU will also trigger it.

Just to be sure, when you write that the code path doesn't leave the chrome process, have you checked if it doesn't indirectly perform a CPOW call somewhere?

Finally, I'm working on my side to try and bring down not-user-visible slowdown alerts.
Flags: needinfo?(dteller)
That was merely a guess, nothing that I could confirm. The code path triggered a few hundred file writes due to a bug on our end (already fixed in the latest development build, will be fixed with Adblock Plus 2.7.1 for stable releases). There is no IPC, CPOW or anything else E10S-related involved. I'm not sure why this issue got more visible with E10S, probably simply because of the chrome process being blocked and unable to respond to messages
No longer depends on: 1187099
No longer depends on: 1266974
Depends on: 1280263
Depends on: 1280265
Depends on: 1280368
No longer depends on: 1192585
Depends on: 1299766
Depends on: 1266974
Assignee: wmccloskey → nobody
AdBlock Plus is being ported to a WebExtension in bug 1226547. Don't think this bug serves much value anymore.
Status: REOPENED → RESOLVED
Closed: 5 years ago2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.