Closed Bug 1007982 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Extension Compatibility, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 33
Tracking Status
e10s + ---
firefox32 --- affected

People

(Reporter: cpeterson, Assigned: billm)

References

()

Details

(Keywords: reproducible)

Attachments

(1 file, 1 obsolete file)

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!
Assignee: nobody → mconley
No longer blocks: e10s-addons
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?
Flags: needinfo?(trev.moz)
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.
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.
Assignee: mconley → wmccloskey
Flags: needinfo?(trev.moz)
Attached patch about-handler-shim (obsolete) — Splinter Review
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.
Attachment #8457648 - Flags: review?(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?
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).
Attachment #8457648 - Attachment is obsolete: true
Attachment #8457648 - Flags: review?(mconley)
Attachment #8458093 - Flags: review?(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.
Attachment #8458093 - Flags: review?(mconley) → review+
https://hg.mozilla.org/mozilla-central/rev/9f9ac2060b54
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
See Also: → 1054162
You need to log in before you can comment on or make changes to this bug.