1007982 - [e10s] Adblock Plus blocks Twitter "Follow" button and #hashtags in e10s window

Status: RESOLVED FIXED

(Firefox :: Extension Compatibility, defect)

Description

[Chris Peterson [:cpeterson]]

STR:
1. Install Adblock Plus addon.
2. In both an e10s and non-e10s window, load this tweet: https://twitter.com/Android/status/464510080575340544
3. As expected, the tweets look the same: they have "Follow" buttons and their text contains #hashtags: "To kick off #NationalPhotoMonth, try your hand at capturing the full picture with #Panorama on #GoogleCamera."

4. In a non-e10s window (to avoid bug 1007978), load https://easylist.adblockplus.org/en/
5. Install "Fanboy's Social Blocking List".
6. Now refresh the e10s and non-e10s tweet windows.
7. RESULT: The #hashtags are missing in the e10s window: "To kick off , try your hand at capturing the full picture with on ."

8. Install "Fanboy's Annoyance List", a superset of the "Fanboy's Social Blocking List".
9. Now refresh the e10s and non-e10s tweet windows.
10. RESULT: The "Follow" button is missing in the e10s window!

Comment 1

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

For the first part (missing hashtags), it looks like this exception rule isn't being properly applied:

twitter.com#@#.twitter-hashtag

It's definitely in the list of blocked items when in a normal window, but missing in an e10s window.

Hey Vladamir! Does ABP have some sort of logging facility to help me figure out why this exception isn't being applied?

Comment 2

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

Ah, OK - I think both of these problems are related.

What's going on is that ABP registers an nsIAboutModule to handle about:abp-elemhidehit. ABP then adds a stylesheet which targets the things it wants to hide or show, and sets the -moz-binding on those elements to point at that URL plus some identifier in a query.

Example:

-moz-binding: url('about:abp-elemhidehit?694256434358#dummy');

In the non-e10s case, this URL resolves just fine, and returns either a binding set with #dummy, or an empty binding set. I guess one causes the element to show up properly, and the other one hides it.

In the e10s case, the nsIAboutModule isn't registered in the content process, so that -moz-binding URL returns a 404.

billm thinks an interpose on Ci.nsIComponentRegistrar might make sense here.

Comment 3

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

So I think we've figured this one out. billm says he can tackle this one, since he's pretty good at writing the interpose glue, and this one sounds a tad complicated for a newbie at it like me (especially if we want this for the m1 deadline).

billm has kindly offered to deal with this.

Comment 4

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

Attachment: about-handler-shim

This turned out to be a little more complex than I had expected. Some of the core methods on nsIInputStream are marked [noscript] (namely ReadSegments), which means that they can't be called via CPOWs. Consequently, I had to do more wrapping than I would like, and the end result is pretty Adblock-specific. However, I doubt there are many other add-ons that need to do this sort of thing.

Comment 5

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

Comment on attachment 8457648 [details] [diff] [review]
about-handler-shim

Review of attachment 8457648 [details] [diff] [review]:
-----------------------------------------------------------------

This all looks fine, and I've confirmed that it fixes the bug. Great job!

I have a question on protocol deregistration in AboutProtocolParent - see below. Once I know more about that, I can give an r+ or r-. Thanks!

::: toolkit/components/addoncompat/RemoteAddonsChild.jsm
@@ +6,5 @@
>  
>  const Ci = Components.interfaces;
>  const Cc = Components.classes;
>  const Cu = Components.utils;
> +const Cr = Components.results;

Whew - glad you caught this, because we were already using Cr in this file in some error handlers.

@@ +14,5 @@
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "BrowserUtils",
> +                                  "resource://gre/modules/BrowserUtils.jsm");
> +
> +XPCOMUtils.defineLazyServiceGetter(this, "systemPrincipal",

Nit: We might want to capitalize this or something to help indicate that it's some global.

@@ +119,5 @@
>                                           Ci.nsISupportsWeakReference]),
>  
>    track: function(path, count) {
>      let catMan = Cc["@mozilla.org/categorymanager;1"].getService(Ci.nsICategoryManager);
> +    if (count == 1) {

Another good catch!

@@ +224,5 @@
> +// This shim protocol handler is used when content fetches an about: URL.
> +function AboutProtocolInstance(contractID)
> +{
> +  this._contractID = contractID;
> +  this._uriFlags = undefined;

This is basically a no-op. Is there a reason to use undefined over null, since it seems you want to be explicit about the assignment?

@@ +307,5 @@
> +      registerFactory.unregisterFactory(this._classID, this);
> +    }
> +  },
> +};
> +AboutProtocolChild.init();

I bitrotted you a bit with bug 1039586 - this will need to get moved into the "makeReady" function I added.

::: toolkit/components/addoncompat/RemoteAddonsParent.jsm
@@ +197,5 @@
> +  unregisterFactory: function(class_, factory) {
> +    for (let i = 0; i < this._protocols.length; i++) {
> +      if (this._protocols[i].factory == factory) {
> +        NotificationTracker.remove(["about-protocol", this._protocols[i].contractID]);
> +        break;

Shouldn't we also remove the protocol from this._protocols as well?

Comment 6

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

Attachment: about-handler-shim v2

Here's a new version with comments addressed. I just forgot to remove the element from the list when deregistering. I defined _protocols in the constructor because I think it's nice to see all the fields of a class in one place. Also, it can make the JS engine run the code a little more efficiently (although it doesn't really matter here).

Comment 7

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

Comment on attachment 8458093 [details] [diff] [review]
about-handler-shim v2

Review of attachment 8458093 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine, thanks for taking this, Bill.

Just one last nit about the _uriFlags = undefined no-op. I'm not going to harp on it though - if you've got a good reason, just go for it and land.

::: toolkit/components/addoncompat/RemoteAddonsChild.jsm
@@ +222,5 @@
> +// This shim protocol handler is used when content fetches an about: URL.
> +function AboutProtocolInstance(contractID)
> +{
> +  this._contractID = contractID;
> +  this._uriFlags = undefined;

What about setting this to null? I think I'd prefer that over undefined.

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f9ac2060b54

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9f9ac2060b54