Closed
Bug 1285899
Opened 8 years ago
Closed 8 years ago
[e10s-multi] mozAddonManager events are broken for e10s-multi
Categories
(Toolkit :: Add-ons Manager, defect)
Toolkit
Add-ons Manager
Tracking
()
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: gkrizsanits, Assigned: aswan)
References
Details
(Whiteboard: [e10s-multi:M1] triaged)
Attachments
(1 file)
with multiple content processes something goes wrong with mozAddonManager events and I get errors like: INFO TEST-UNEXPECTED-FAIL | toolkit/mozapps/extensions/test/browser/browser_webapi_addon_listener.js | Uncaught exception - at chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_webapi_addon_listener.js:14 - SyntaxError: JSON.parse: unexpected end of data at line 1 column 1 of the JSON data
Reporter | ||
Updated•8 years ago
|
Blocks: e10s-multi
Whiteboard: [e10s-multi:M?]
Assignee | ||
Comment 1•8 years ago
|
||
The logic here is overly simplistic: http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/addonManager.js#230-253 It creates a single listener in the parent process to send events to a child process, but it falls apart with multiple child processes. It should be straightforward to fix...
Assignee: nobody → aswan
Updated•8 years ago
|
Whiteboard: [e10s-multi:M?] → [e10s-multi:M?] triaged
Reporter | ||
Updated•8 years ago
|
Whiteboard: [e10s-multi:M?] triaged → [e10s-multi:M1] triaged
Reporter | ||
Comment 2•8 years ago
|
||
Is this something you're actively working / will be working soon, or would you prefer me picking it up?
Flags: needinfo?(aswan)
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #2) > Is this something you're actively working / will be working soon, or would > you prefer me picking it up? It depends on how soon "soon" is. I've got some 50-related things to wrap up this week but was planning to pick this up early next week. If its more pressing than that, feel free to take it.
Flags: needinfo?(aswan)
Assignee | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/69080/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/69080/
Assignee | ||
Updated•8 years ago
|
Attachment #8777527 -
Flags: review?(gkrizsanits)
Reporter | ||
Comment 5•8 years ago
|
||
https://reviewboard.mozilla.org/r/69080/#review66686 ::: toolkit/mozapps/extensions/addonManager.js:169 (Diff revision 1) > AddonManagerPrivate.backgroundUpdateTimerHandler(); > }, > > - addonListener: null, > + // Maps message manager instances for content processes to the associated > + // AddonListener instances. > + addonListeners: new Map(), Could you turn this into a WeakMap? I see no reason to hold onto the process message managers here. Also I think we should have an observer for message-manager-disconnect that calls removeAddonListener on the related listener to prevent messages to be sent after disconnect.
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #5) > Could you turn this into a WeakMap? I see no reason to hold onto the process > message managers here. This failed with the exception "cannot use the given object as a weak map key". The message manager itself doesn't need to be the key though if there is some sort of unique identifier I can easily access from the message manager?
Flags: needinfo?(gkrizsanits)
Reporter | ||
Comment 7•8 years ago
|
||
(In reply to Andrew Swan [:aswan] from comment #6) > (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #5) > > Could you turn this into a WeakMap? I see no reason to hold onto the process > > message managers here. > > This failed with the exception "cannot use the given object as a weak map > key". The message manager itself doesn't need to be the key though if there > is some sort of unique identifier I can easily access from the message > manager? Sigh. Yeah, we don't actually have any DOM nodes for process message managers only for frame message managers. And even that is not that reliable since windows can just swap their fmm's... We could probably fix this but I don't think it's worth the time, the observer should be enough, let's go with the strong map. And sorry for wasting your time with this.
Flags: needinfo?(gkrizsanits)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•8 years ago
|
||
No problem, I added cleanup of these listeners to the message-manager-disconnect and -closed handlers. That code doesn't have tests though, are there existing tests of cleanup in disconnect handlers that would be a good model or is that more trouble than its worth?
Reporter | ||
Comment 10•8 years ago
|
||
mozreview-review |
Comment on attachment 8777527 [details] Bug 1285899 Manager mozAddonManager AddonListeners per child https://reviewboard.mozilla.org/r/69080/#review67524 ::: toolkit/mozapps/extensions/addonManager.js:265 (Diff revision 2) > } > return undefined; > }, > > + childClosed(target) { > + AddonManager.webAPI.clearInstallsFrom(target); You also want to call removeAddonListener here, no? After the mm is closed no more receiveMessage will come in from that target... I would also do an addonListeners.has(target) check like in the receiveMessage code. For clearInstallsFrom that does not seem to be necessary since it does the check on the target internally.
Attachment #8777527 -
Flags: review?(gkrizsanits) → review+
Assignee | ||
Comment 11•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8777527 [details] Bug 1285899 Manager mozAddonManager AddonListeners per child https://reviewboard.mozilla.org/r/69080/#review67524 > You also want to call removeAddonListener here, no? After the mm is closed no more receiveMessage will come in from that target... > > I would also do an addonListeners.has(target) check like in the receiveMessage code. For clearInstallsFrom that does not seem to be necessary since it does the check on the target internally. yep, thanks for catching that.
Comment hidden (mozreview-request) |
Comment 13•8 years ago
|
||
Pushed by aswan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bddf684f5b46 Manager mozAddonManager AddonListeners per child r=krizsa
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bddf684f5b46
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Assignee | ||
Updated•8 years ago
|
Iteration: --- → 51.1 - Aug 15
You need to log in
before you can comment on or make changes to this bug.
Description
•