Closed
Bug 1102410
Opened 9 years ago
Closed 9 years ago
AboutProtocolChild attempts to register the same factory twice for multiple nsIAboutModule's
Categories
(Content Services Graveyard :: Interest Dashboard, defect)
Tracking
(e10sm4+)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
e10s | m4+ | --- |
People
(Reporter: blassey, Assigned: mconley)
References
Details
Attachments
(3 files, 1 obsolete file)
2.69 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
8.88 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
9.88 KB,
application/zip
|
Details |
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•9 years ago
|
||
Dupe of bug 972507?
Comment 2•9 years ago
|
||
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.
Blocks: e10s-addons, 972507
Keywords: regression,
regressionwindow-wanted
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → mconley
Reporter | ||
Comment 3•9 years ago
|
||
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: 9 years ago
Resolution: --- → WORKSFORME
Reporter | ||
Updated•9 years ago
|
Status: RESOLVED → REOPENED
Component: Extension Compatibility → Interest Dashboard
Keywords: regression,
regressionwindow-wanted
Product: Firefox → Content Services
Resolution: WORKSFORME → ---
Summary: BugzillaJS stopped working with e10s → Interest Dashboard breaks BugzillaJS
Reporter | ||
Comment 4•9 years ago
|
||
I re-enabled the interest dashboard, which re-broke BugzillaJS
Assignee | ||
Updated•9 years ago
|
Status: REOPENED → ASSIGNED
Assignee | ||
Comment 5•9 years ago
|
||
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•9 years ago
|
||
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?
Assignee | ||
Comment 7•9 years ago
|
||
(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.
Assignee | ||
Comment 8•9 years ago
|
||
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
Assignee | ||
Comment 9•9 years ago
|
||
Assignee | ||
Comment 10•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8529117 -
Attachment is obsolete: true
Assignee | ||
Comment 11•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Attachment #8529117 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 12•9 years ago
|
||
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)
Assignee | ||
Comment 13•9 years ago
|
||
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+
Assignee | ||
Comment 16•9 years ago
|
||
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)
Assignee | ||
Comment 17•9 years ago
|
||
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.
Assignee | ||
Comment 18•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Whiteboard: [fixed-in-fx-team][keep-open]
Comment 19•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b1975d34ea5a
Whiteboard: [fixed-in-fx-team][keep-open] → [keep-open]
Comment 20•9 years ago
|
||
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 21•9 years ago
|
||
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 22•9 years ago
|
||
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+
Assignee | ||
Comment 24•9 years ago
|
||
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+
Assignee | ||
Comment 25•9 years ago
|
||
billm answered this in IRC - yeah, it was "additionally".
Flags: needinfo?(wmccloskey)
Assignee | ||
Comment 26•9 years ago
|
||
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: 9 years ago → 9 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 28•9 years ago
|
||
bugnotes |
http://people.mozilla.org/~mconley2/bugnotes/bug-1102410.html
You need to log in
before you can comment on or make changes to this bug.
Description
•