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

Status: RESOLVED FIXED

(Content Services Graveyard :: Interest Dashboard, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

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

Comment 1

[Jorge Villalobos [:jorgev] (he/him)]

Dupe of bug 972507?

Comment 2

[Chris Peterson [:cpeterson]]

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.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

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/

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

I re-enabled the interest dashboard, which re-broke BugzillaJS

Comment 5

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

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.

Comment 6

[Ed Lee :Mardak]

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?

Comment 7

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

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

Comment 8

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

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.

Comment 9

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

Attachment: Make AboutProtocolChild use a unique classID each time it registers a new nsIAboutModule. r=billm (checked-in)

Comment 10

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

Attachment: Regression test to ensure that multiple shimmed nsIAboutModule's can be accessed from the content process. r=?

Comment 11

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

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.

Comment 12

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

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?

Comment 13

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

Also, I can confirm with this patch that the AMO version of Interest Dashboard no longer breaks BugzillaJS.

Comment 15

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

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.

Comment 16

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

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

Comment 17

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

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 18

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

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

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b1975d34ea5a

Comment 20

[Allison Naaktgeboren :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=?

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.

Comment 21

[Allison Naaktgeboren :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=?

Blake, would you have a look at the loadGroupNotificationCallbacks part of this patch?

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 24

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

Attachment: Regression test to ensure that multiple shimmed nsIAboutModule's can be accessed from the content process, feedback=ally, r=mrbkap

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"?

Comment 25

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

billm answered this in IRC - yeah, it was "additionally".

Comment 26

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

Thanks for the feedback / reviews!

remote:   https://hg.mozilla.org/integration/fx-team/rev/a1bfd0879763

Comment 27

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

https://hg.mozilla.org/mozilla-central/rev/a1bfd0879763

Comment 28

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

Attachment: Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1102410.html