Closed Bug 1102410 Opened 10 years ago Closed 10 years ago

AboutProtocolChild attempts to register the same factory twice for multiple nsIAboutModule's

Categories

(Content Services Graveyard :: Interest Dashboard, defect)

x86
macOS
defect
Not set
normal

Tracking

(e10sm4+)

RESOLVED FIXED
Tracking Status
e10s m4+ ---

People

(Reporter: blassey, Assigned: mconley)

References

Details

Attachments

(3 files, 1 obsolete file)

BugzillaJS started working for me last week (presumably when the page-mod patches landed) and the world was again glorious.

On tuesday of this week I noticed that it was no longer adding the "open all in tabs" link to my bugzilla query pages. Also, now whenever I load a bugzilla page I see the following on my console:

NS_ERROR_FACTORY_EXISTS: Component returned failure code: 0xc1f30100 (NS_ERROR_FACTORY_EXISTS) [nsIComponentRegistrar.registerFactory] RemoteAddonsChild.jsm:128:0
The original bug 972507 was presumably fixed by the page-mod fixes. Let's close the original bug and use this new bug to track the recent regression because they probably have different root causes.
Assignee: nobody → mconley
disabling my other addons fixed this.  I think I installed the interest dashboard at roughly the same time this stopped working, so... there's that.

https://www.mozilla.org/en-US/firefox/interest-dashboard/
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WORKSFORME
Status: RESOLVED → REOPENED
Component: Extension Compatibility → Interest Dashboard
Product: Firefox → Content Services
Resolution: WORKSFORME → ---
Summary: BugzillaJS stopped working with e10s → Interest Dashboard breaks BugzillaJS
I re-enabled the interest dashboard, which re-broke BugzillaJS
Status: REOPENED → ASSIGNED
So I've dug into this a bit.

First of all, I've noticed that the next release of Interest Dashboard (which is currently in the review queue on AMO) does not cause this problem.

I've bisected, and found that the revision that fixed the problem was this one:

https://github.com/mozilla/interest-dashboard/tree/873566fcf6d5660eed7867a127497783ea4a9d2e

bisecting that change, it appears as if the fix occurred in Application.js when DevMenu's factory was removed.

More precisely, if one were to take the busted version on AMO (that's https://github.com/mozilla/interest-dashboard/tree/5cf5a9ac817d486dfa8cfa5b9442dbaec14eac35), and remove the line: "Factory(DevMenu.factory);", BugzillaJS would work again.

That factory registers an nsIAboutModule. I suspect our add-on shims are bugging out here because this add-on is installing two nsIAboutModules - installing only one of DevMenu.factory and AboutYou.factory fixes the issue. I'll try to write up a minimal test case to confirm.
Thanks for digging into this! Here's links directly to the lines you mentioned:

https://github.com/mozilla/interest-dashboard/blob/5cf5a9ac817d486dfa8cfa5b9442dbaec14eac35/lib/Application.js#L359

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #5)
> That factory registers an nsIAboutModule. I suspect our add-on shims are
> bugging out here because this add-on is installing two nsIAboutModules -
> installing only one of DevMenu.factory and AboutYou.factory fixes the issue.
Just to clarify, (I had trouble parsing that sentence :p), you mean using the factory for one or the other but not both.

It could very well be an issue with how we use the factory.. maybe needing to change some identifier to avoid collision?
(In reply to Ed Lee :Mardak from comment #6)
> Thanks for digging into this! Here's links directly to the lines you
> mentioned:
> 
> https://github.com/mozilla/interest-dashboard/blob/
> 5cf5a9ac817d486dfa8cfa5b9442dbaec14eac35/lib/Application.js#L359
> 
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #5)
> > That factory registers an nsIAboutModule. I suspect our add-on shims are
> > bugging out here because this add-on is installing two nsIAboutModules -
> > installing only one of DevMenu.factory and AboutYou.factory fixes the issue.
> Just to clarify, (I had trouble parsing that sentence :p), you mean using
> the factory for one or the other but not both.
> 
> It could very well be an issue with how we use the factory.. maybe needing
> to change some identifier to avoid collision?

Possibly - although I'm more inclined to believe that perhaps our shimming is going wrong somehow. I'll let you know more soon.
Ok, so it looks like our shims are to blame.

Assuming we have a browser starting up with BugzillaJS and Interest Dashboard already installed, here's what happens:

The very first browser-child.js starts up… and it calls RemoteAddonsChild.init on itself.
Because this is the first time RemoteAddonsChild.init has been called, we call RemoteAddonsChild.makeReady to initialize the shims.

NotificationTracker.init is called, which populates the NotificationTracker with all of the information about shims it should hook up.

ContentPolicyChild.init is then called. That goes just fine.

AboutProtocolChild.init is called - and here's where things go wrong. The Interest Dashboard (at least the version on AMO) registers two about: pages, so there are two factories that need registering. The problem is that we attempt to register the same AboutProtocolChild as the factory each time. Repeated calls result in an exception being thrown because a factory has already been registered at the same ID.

So we throw an exception, and so makeReady fails out. That's why subsequent creations of browser-child.js's result in makeReady getting called again - because makeReady didn't succeed, RemoteAddonsChild just assumed that it had never started up before, and calls makeReady again for that new browser-child.js.

So I think we just want to make AboutProtocolChild register a unique factory per AboutProtocolInstance.
Summary: Interest Dashboard breaks BugzillaJS → AboutProtocolChild attempts to register the same factory twice for multiple nsIAboutModule's
Attachment #8529117 - Attachment is obsolete: true
Comment on attachment 8529117 [details] [diff] [review]
Make AboutProtocolChild use a unique classID each time it registers a new nsIAboutModule. r=billm (checked-in)

Silly bzexport.
Attachment #8529117 - Attachment is obsolete: false
Attachment #8529117 - Flags: review?(wmccloskey)
Comment on attachment 8529118 [details] [diff] [review]
Regression test to ensure that multiple shimmed nsIAboutModule's can be accessed from the content process. r=?

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

::: toolkit/components/addoncompat/RemoteAddonsChild.jsm
@@ +203,5 @@
>        uri: this.URI.spec,
>        contractID: this._contractID
>      }, {
>        notificationCallbacks: this.notificationCallbacks,
> +      loadGroupNotificationCallbacks: this.loadGroup ? this.loadGroup.notificationCallbacks : null,

This is the only bit of this patch I'm not sure about. It looks like when we try to instantiate this channel via XMLHttpRequest, we don't set a loadGroup... or something like that. The loadGroup is null if I don't make this change and run the test, so we fail out here. 

From my understanding, it's the responsibility of the caller to set the loadGroup. Does a null loadGroup even make sense, or have I possibly uncovered a networking bug here?
Attachment #8529118 - Flags: review?(wmccloskey)
Also, I can confirm with this patch that the AMO version of Interest Dashboard no longer breaks BugzillaJS.
Comment on attachment 8529117 [details] [diff] [review]
Make AboutProtocolChild use a unique classID each time it registers a new nsIAboutModule. r=billm (checked-in)

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

::: toolkit/components/addoncompat/RemoteAddonsChild.jsm
@@ +340,5 @@
> +  _generateClassID: function() {
> +    let uuid = Cc["@mozilla.org/uuid-generator;1"]
> +                .getService(Ci.nsIUUIDGenerator)
> +                .generateUUID()
> +                .toString();

Based on this code, I think we just want to return the result of generateUUID():
http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/autocomplete.js#47
Then we don't need the string manipulation.
Attachment #8529117 - Flags: review?(wmccloskey) → review+
Comment on attachment 8529118 [details] [diff] [review]
Regression test to ensure that multiple shimmed nsIAboutModule's can be accessed from the content process. r=?

ally wants more review requests! :D
Attachment #8529118 - Flags: review?(wmccloskey) → review?(ally)
Comment on attachment 8529118 [details] [diff] [review]
Regression test to ensure that multiple shimmed nsIAboutModule's can be accessed from the content process. r=?

ally - note my comment above about notificationCallbacks. I'm not 100% sure I'm doing the right thing here.
Comment on attachment 8529117 [details] [diff] [review]
Make AboutProtocolChild use a unique classID each time it registers a new nsIAboutModule. r=billm (checked-in)

remote:   https://hg.mozilla.org/integration/fx-team/rev/b1975d34ea5a
Attachment #8529117 - Attachment description: Make AboutProtocolChild use a unique classID each time it registers a new nsIAboutModule. r=? → Make AboutProtocolChild use a unique classID each time it registers a new nsIAboutModule. r=billm (checked-in)
Whiteboard: [fixed-in-fx-team][keep-open]
https://hg.mozilla.org/mozilla-central/rev/b1975d34ea5a
Whiteboard: [fixed-in-fx-team][keep-open] → [keep-open]
Comment on attachment 8529118 [details] [diff] [review]
Regression test to ensure that multiple shimmed nsIAboutModule's can be accessed from the content process. r=?

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

I think the test looks fine, but I'm not qualified to review the channel part of this patch.
Attachment #8529118 - Flags: review?(ally) → feedback+
Comment on attachment 8529118 [details] [diff] [review]
Regression test to ensure that multiple shimmed nsIAboutModule's can be accessed from the content process. r=?

Blake, would you have a look at the loadGroupNotificationCallbacks part of this patch?
Attachment #8529118 - Flags: review?(mrbkap)
Comment on attachment 8529118 [details] [diff] [review]
Regression test to ensure that multiple shimmed nsIAboutModule's can be accessed from the content process. r=?

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

Yeah, not all requests are guaranteed to have loadgroups associated with them. Your XHR requests are being created in a JS component, from which we can't get a loadgroup.
Attachment #8529118 - Flags: review?(mrbkap) → review+
Hey billm - yesterday, you said in IRC:

15:59 billm: mconley: I think this code should just set loadGroup to null when the child had no load group: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/addoncompat/RemoteAddonsParent.jsm#244

which I've included in this patch. I assume however, that you didn't mean I should revert the other loadGroup change I made? I ask, because even if I don't set loadGroup in the parent, in the child, we'll still attempt to access this.loadGroup.notificationCallbacks if this.loadGroup is null, which is no good.

So can I assume this was an "additionally" as opposed to an "instead of"?
Attachment #8529118 - Attachment is obsolete: true
Flags: needinfo?(wmccloskey)
Attachment #8534555 - Flags: review+
billm answered this in IRC - yeah, it was "additionally".
Flags: needinfo?(wmccloskey)
Thanks for the feedback / reviews!

remote:   https://hg.mozilla.org/integration/fx-team/rev/a1bfd0879763
Whiteboard: [keep-open]
https://hg.mozilla.org/mozilla-central/rev/a1bfd0879763
Status: ASSIGNED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: