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

Status: RESOLVED FIXED

(Firefox :: Extension Compatibility, defect, P4)

Description

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 1

[Wladimir Palant]

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

[B.J. Herbison]

Does comment 1 mean e10s won't be enabled by default until a future version of Adblock Plus uses the future extension interfaces?

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 4

[Wladimir Palant]

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.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(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

[Paul [pwd]]

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.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

My understanding is that the two browser block the same things. If that's incorrect, please give examples.

Comment 8

[Paul [pwd]]

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.

Comment 9

[Wladimir Palant]

(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.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 11

[Wladimir Palant]

(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.

Comment 12

[Wladimir Palant]

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.

Comment 13

[Wladimir Palant]

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.

Comment 14

[David Teller [:Yoric] - still alive but not very active]

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?

Comment 15

[Wladimir Palant]

Most of the time - yes. Our testers are saying that this warning is still being triggered under some conditions.

Comment 16

[Wladimir Palant]

Adblock Plus 2.7 was released a week ago, so this can be considered fixed by any means.