[e10s] Adblock Plus spends a lot of time in ContentPolicyChild.shouldLoad over the shims

RESOLVED FIXED

Status

()

Firefox
Extension Compatibility
P4
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mconley, Assigned: Gabor Krizsanits (INACTIVE))

Tracking

(Blocks: 1 bug, {addon-compat, meta, perf})

Firefox Tracking Flags

(Not tracked)

Details

(URL)

See https://bugzilla.mozilla.org/show_bug.cgi?id=1135719#c11 for a link to a profile.

Per page load (especially on news sites, apparently), the content process was blocked, spending about 600ms in ContentPolicyChild.shouldLoad.

billm / gabor: is there more tuning that we can do in the shims to make this better?
(Assignee)

Updated

3 years ago
Assignee: nobody → gkrizsanits

Updated

3 years ago
Keywords: perf

Comment 1

3 years ago
The proper solution here would be to move our content policy into the child process, so that the messaging required is minimized. However, in the light of https://blog.mozilla.org/addons/2015/08/21/the-future-of-developing-firefox-add-ons/ we are no longer sure that this is a well-invested effort.

Comment 2

3 years ago
Does comment 1 mean e10s won't be enabled by default until a future version of Adblock Plus uses the future extension interfaces?
Wladimir, we'd like to work with you to make Adblock stop using shims. Using WebExtensions is one possibility. The Chrome versus of Adblock Plus already works pretty well with WebExtensions (except for https://issues.adblockplus.org/ticket/2689). However, it lacks some of the functionality of the Firefox version. I'm not sure how hard that would be to fix.

If you would rather stick with the existing Firefox Adblock code for now, that's fine. One option is to start using the WebRequest JSM. Another option is to use a process script to run the content policy and forward everything to the parent process.

We've already discussed additions to the WebRequest API in bug 1166496. The part about including the entire chain of URLs is still missing. If you need this, I can implement it. Somehow the Chrome version of Adblock Plus gets around this--I'm not sure how.

Wladimir, please let me know which route you would like to take. I'm happy to help whichever way you want to go.
Flags: needinfo?(trev.moz)

Comment 4

3 years ago
Our Chrome version isn't something that we would want to use on Firefox even if it works. It lacks significantly in terms of functionality, partly because of API limitations and partly because bringing it on par with our much older Firefox version is a slow process that we have to approach with limited resources.

Also, so far our conclusion was that browser-provided WebRequest API would always be a suboptimal solution for us. Ideally, we would be able to access DOM nodes that the request refers to - e.g. in order to hide them or to remember the requests associated with them. The former is somewhat broken in Adblock Plus for Chrome right now, the latter completely missing. Our conclusion was that we want to implement something similar to the WebRequest API ourselves, by making our content policy run in the content process (along with logic that is specific to Adblock Plus).

This would solve the problem nicely, and we already had it on our roadmap. However, it's still a significant effort - and given Mozilla's change of course we aren't sure yet what our priorities should be. The way it sounds, this effort would be pretty much wasted, and we are supposed to start working on a complete rewrite of Adblock Plus instead.
Flags: needinfo?(trev.moz)
(In reply to Wladimir Palant from comment #4)
> Also, so far our conclusion was that browser-provided WebRequest API would
> always be a suboptimal solution for us. Ideally, we would be able to access
> DOM nodes that the request refers to

I'd rather not add APIs unless absolutely necessary, so I want to make sure this isn't something that couldn't be done in another way.

Also, this would be a pretty fragile API. Let's suppose the add-on runs in its own process. You would presumably get some sort of identifier for a DOM node that you could send to the content process. Then there would have to be a function in the content process to turn the identifier into a DOM node. That function could fail if the DOM node has already been removed from the DOM and garbage collected. Also, the DOM node could have been modified since the original decision to block it.

>  - e.g. in order to hide them or to
> remember the requests associated with them. The former is somewhat broken in
> Adblock Plus for Chrome right now

I guess you're talking about the checkCollapse code? How is it broken?

> , the latter completely missing.

Could this functionality be done in the same way as checkCollapse?

> This would solve the problem nicely, and we already had it on our roadmap.
> However, it's still a significant effort - and given Mozilla's change of
> course we aren't sure yet what our priorities should be. The way it sounds,
> this effort would be pretty much wasted, and we are supposed to start
> working on a complete rewrite of Adblock Plus instead.

I think that extending the Chrome extension is the best long-term investment. However, I'm happy to help whichever way you guys want to go. I'm willing to contribute code if you think it's useful.

Comment 6

3 years ago
In light of "Our Chrome version isn't something that we would want to use on Firefox even if it works. It lacks significantly in terms of functionality, partly because of API limitations and partly because bringing it on par with our much older Firefox version is a slow process that we have to approach with limited resources." I cannot believe what I've just read. ABP for Chrome doesn't block as much as it does on Firefox, can we not suggest regressing key add-ons. As of now, the most viable actual solution is the additional API.
My understanding is that the two browser block the same things. If that's incorrect, please give examples.

Comment 8

3 years ago
What springs to mind is that video adverts are removed/circumvented on Firefox but not on Chrome. But its a moot point when you defer to what Wladimir said when he explicitly stated ABP for Chrome "lacks significantly in terms of functionality" compared to its Firefox counterpart. It was just before you said:

(In reply to Bill McCloskey (:billm) from comment #3)
> Wladimir, please let me know which route you would like to take. I'm happy
> to help whichever way you want to go.
Flags: needinfo?(trev.moz)

Comment 9

3 years ago
(In reply to Bill McCloskey (:billm) from comment #7)
> My understanding is that the two browser block the same things. If that's
> incorrect, please give examples.

As far as blocking is concerned, I think that Chrome is mostly on par with Firefox by now. There are some issues distinguishing types: objects and object subrequests cannot be distinguished, HTML5 audio/video and font downloads use type "other", objects will also occasionally be reported as "other" due to a bug (https://code.google.com/p/chromium/issues/detail?id=410382).

There are also issues identifying the context of a request, meaning: which frame initiated this request and what was its parent frame. The API relies on extensions tracking frame IDs, that's a very fragile system and it has issues around about:blank and data: frames.

But blocking does mostly work, the issues are relatively minor. Where things completely break down, that's element hiding. Chrome has no concept of global user stylesheets, and even if it were possible - without @-moz-document it wouldn't help much. Attaching user stylesheets to individual documents isn't supported either, it only works on per-tab basis (mind you, frame IDs have been added around Chrome 16/17 and most APIs haven't been updated for that yet). Besides, the user stylesheets aren't allowed to use !important meaning that the website can always override them. So we are forced to inject a <style> tag into the DOM which causes all kinds of issues.
Flags: needinfo?(trev.moz)
Regarding collapsing, I wonder if we could have an event fire in the extension content page if the load of an element is blocked. It seems like that would be a much cleaner abstraction than what happens right now with collapsing. The event handler wouldn't be able to do any blocking itself; it would just respond to it.

(In reply to Wladimir Palant from comment #9)
> But blocking does mostly work, the issues are relatively minor. Where things
> completely break down, that's element hiding. Chrome has no concept of
> global user stylesheets, and even if it were possible - without
> @-moz-document it wouldn't help much. Attaching user stylesheets to
> individual documents isn't supported either, it only works on per-tab basis
> (mind you, frame IDs have been added around Chrome 16/17 and most APIs
> haven't been updated for that yet). Besides, the user stylesheets aren't
> allowed to use !important meaning that the website can always override them.
> So we are forced to inject a <style> tag into the DOM which causes all kinds
> of issues.

That sounds terrible. It sounds like maybe tabs.insertCSS [1] might do what you need if it accepted a frameId option?

[1] https://developer.chrome.com/extensions/tabs#method-insertCSS
Flags: needinfo?(trev.moz)

Comment 11

3 years ago
(In reply to Bill McCloskey (:billm) from comment #10)
> Regarding collapsing, I wonder if we could have an event fire in the
> extension content page if the load of an element is blocked. It seems like
> that would be a much cleaner abstraction than what happens right now with
> collapsing. The event handler wouldn't be able to do any blocking itself; it
> would just respond to it.

As long as this event is invisible to the webpage, it would be the optimal solution I think. Currently, we are listening to the "error" and "load" events but not all elements fire one. Also, we don't know whether the element was really blocked so we try to deduce type/URL and ask the background page whether that URL should be blocked and collapsed. If the event already implied that the element is blocked and also had the corresponding info (type/URL) things would become much simpler and more reliable. Ideally, webRequest would just specify some arbitrary data to be attached to the event. That should do for both collapsing and blockable items list.

> That sounds terrible. It sounds like maybe tabs.insertCSS [1] might do what
> you need if it accepted a frameId option?
> 
> [1] https://developer.chrome.com/extensions/tabs#method-insertCSS

The important issues here are https://crbug.com/63979 and https://crbug.com/162096. As long as these exists, tabs.insertCSS is unusable for us.
Flags: needinfo?(trev.moz)

Comment 12

3 years ago
For reference, our content policy implementation refactoring is currently under review (https://issues.adblockplus.org/ticket/3208). It will break many other parts of the extension however (see "blocked by" list) so that the main part will only land on a branch for now. We'll merge the branch once the other parts have been made E10S-aware as well.
Priority: -- → P4

Comment 13

3 years ago
The changes landed in the Adblock Plus development builds two weeks ago, Adblock Plus 2.7 is scheduled for release tomorrow. Up to you when you want to mark this issue as resolved.
Wladimir, for a few weeks, ABP has triggered the Slow Add-on Watcher pretty much all the time. Do you think that will fix the issue?
Flags: needinfo?(trev.moz)

Comment 15

3 years ago
Most of the time - yes. Our testers are saying that this warning is still being triggered under some conditions.
Flags: needinfo?(trev.moz)

Comment 16

3 years ago
Adblock Plus 2.7 was released a week ago, so this can be considered fixed by any means.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.