1285899 - [e10s-multi] mozAddonManager events are broken for e10s-multi

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Gabor Krizsanits (INACTIVE)]

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

Comment 1

[Andrew Swan [:aswan]]

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

Comment 2

[Gabor Krizsanits (INACTIVE)]

Is this something you're actively working / will be working soon, or would you prefer me picking it up?

Comment 3

[Andrew Swan [:aswan]]

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

Comment 4

[Andrew Swan [:aswan]]

Attachment: Bug 1285899 Manager mozAddonManager AddonListeners per child

Review commit: https://reviewboard.mozilla.org/r/69080/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69080/

Comment 5

[Gabor Krizsanits (INACTIVE)]

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.

Comment 6

[Andrew Swan [:aswan]]

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

Comment 7

[Gabor Krizsanits (INACTIVE)]

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

Comment 8

[Andrew Swan [:aswan]]

Comment on attachment 8777527 [details]
Bug 1285899 Manager mozAddonManager AddonListeners per child

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69080/diff/1-2/

Comment 9

[Andrew Swan [:aswan]]

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?

Comment 10

[Gabor Krizsanits (INACTIVE)]

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.

Comment 11

[Andrew Swan [:aswan]]

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 12

[Andrew Swan [:aswan]]

Comment on attachment 8777527 [details]
Bug 1285899 Manager mozAddonManager AddonListeners per child

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69080/diff/2-3/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bddf684f5b46
Manager mozAddonManager AddonListeners per child r=krizsa

Comment 14

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

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