Dave Townsend [:mossop] Reporter
	Description • 10 years ago

When a webpage requests we install an add-on it can pass a callback to be told when the install completes. We currently give the callback an ID based on the window inner ID and an incrementing integer. This is unique for a content process but once we have more than one process it would be possible to end up with two callbacks in different processes being assigned the same ID so we might call the wrong callback for an add-on install.

Rather than using the process message manager tree for calling callbacks we should use the browser message manager.

	Dave Townsend [:mossop] Reporter
	Comment 1 • 10 years ago

There is a risk of leaking data across websites here but the chances of this happening or being able to engineer it to happen are pretty low, you'd need to somehow cause the user to open a second tab that opened a second process and managed to end up with the same inner window ID as a malicious page. This probably only needs to be a +.

	:shell escalante
	Comment 2 • 8 years ago

aswan working on something similar - check on it.

	Kris Maglione [:kmag]
	Comment 4 • 8 years ago
mozreview-review

Comment on attachment 8799993 [details]
Bug 1091924 Fix up InstallTrigger for e10s-multi

https://reviewboard.mozilla.org/r/85022/#review83868

This looks good to me, but do we still need this weird workaround in amContentHandler.js? http://searchfox.org/mozilla-central/rev/cd1be634c9309c7fc99a3fde67dd44d343875f60/toolkit/mozapps/extensions/amContentHandler.js#59-78

::: toolkit/mozapps/extensions/amInstallTrigger.js:48
(Diff revision 1)
> +    .getInterface(Ci.nsIWebNavigation)
> +    .QueryInterface(Ci.nsIDocShell)

No need for both of these. Either `.getInterface(Ci.nsIWebNavigation)` or `.getInterface(Ci.nsIDocShell)` (preferable), but not gI and then QI.

	Andrew Swan [:aswan] Assignee
	Comment 5 • 8 years ago
mozreview-review-reply

Comment on attachment 8799993 [details]
Bug 1091924 Fix up InstallTrigger for e10s-multi

https://reviewboard.mozilla.org/r/85022/#review83868

I'm not sure, there's a similar fragment here
http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/amInstallTrigger.js#76-94

	Andrew Swan [:aswan] Assignee
	Comment 6 • 8 years ago
mozreview-review-reply

Comment on attachment 8799993 [details]
Bug 1091924 Fix up InstallTrigger for e10s-multi

https://reviewboard.mozilla.org/r/85022/#review83868

With the code from both chunks above removed, all the xpinstall unit tests still pass.  I'll push a new version here and to try momentarily.

	Kris Maglione [:kmag]
	Comment 7 • 8 years ago
mozreview-review-reply

Comment on attachment 8799993 [details]
Bug 1091924 Fix up InstallTrigger for e10s-multi

https://reviewboard.mozilla.org/r/85022/#review83868

Can we try changing them to use the top-level frame message manager and see if tests still pass? I don't really like having these kinds of special cases...

	Andrew Swan [:aswan] Assignee
	Comment 9 • 8 years ago
mozreview-review-reply

Comment on attachment 8799993 [details]
Bug 1091924 Fix up InstallTrigger for e10s-multi

https://reviewboard.mozilla.org/r/85022/#review83868

Well if I remove those fragments and just use the message manager from the window where the request originates, the unit tests all pass locally, including the one that was added along with that code in bug 1200027.  That bug is written about the discovery pane which has of course undergone all sorts of changes since then.  But the unit test appears to be independent of that so ... seems okay to me.  I could also ask Mossop to take a look if you like?

	Andrew Swan [:aswan] Assignee
	Comment 10 • 8 years ago

Comment on attachment 8799993 [details]
Bug 1091924 Fix up InstallTrigger for e10s-multi

Eh looks like I was wrong about the unit test passing, let me look at this one again...

	Andrew Swan [:aswan] Assignee
	Comment 11 • 8 years ago

So I'm not sure I've got this right but I think it looks like taking that special case code causes browser_discovery_install.js to fail here:
http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/AddonManager.jsm#2188
The installing principal is the content page (https://example.com in the test) and the browser is the outer one that holds about:addons which has the system principal.  The former obviously does not subsume the latter.
My head is spinning trying to figure out what this test is for exactly but its failing.

I'm not sure if this test is still necessary, I think it dates back to when we had an AMO page that used InstallTrigger framed inside about:addons, which (I don't think) we do anymore since mozAddonManager came around.

So, a) drop browser_discovery_install.js, b) restore the special-case code, or c) something else I'm not clever enough to figure out?
(ccing Mossop as well)

	Kris Maglione [:kmag]
	Comment 12 • 8 years ago

We just need to send the message on the root message manager for the about:addons case. It's still a workaround, but at least it's a more direct workaround.

	Andrew Swan [:aswan] Assignee
	Comment 14 • 8 years ago

Kris and I went over this more on IRC last night and concluded that we need to keep the special-case code in place if we want to support this case, which I suspect we do.
Here's the try run from the initial revision of the patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e9f484dad09
The actual changes from that are small enough that I'm not going to do another run:
https://reviewboard.mozilla.org/r/85022/diff/1-3/

	Kris Maglione [:kmag]
	Comment 15 • 8 years ago
mozreview-review

Comment on attachment 8799993 [details]
Bug 1091924 Fix up InstallTrigger for e10s-multi

https://reviewboard.mozilla.org/r/85022/#review84198

r=me with the stricter try-catch.

::: toolkit/mozapps/extensions/amInstallTrigger.js:145
(Diff revision 3)
> -    window.QueryInterface(Components.interfaces.nsIInterfaceRequestor);
> -    let utils = window.getInterface(Components.interfaces.nsIDOMWindowUtils);
> +    try {
> +      this._mediator = new RemoteMediator(window);
> -    this._mediator = new RemoteMediator(utils.currentInnerWindowID);
> -
> -    return window.InstallTriggerImpl._create(window, this);
> +      return window.InstallTriggerImpl._create(window, this);
> +    } catch (ex) {

Hm. This seems a bit too broad. If we're ignoring errors, we should probably only ignore them from the RemoteMediator constructor.

	Andrew Swan [:aswan] Assignee
	Comment 16 • 8 years ago
mozreview-review-reply

Comment on attachment 8799993 [details]
Bug 1091924 Fix up InstallTrigger for e10s-multi

https://reviewboard.mozilla.org/r/85022/#review84198

> Hm. This seems a bit too broad. If we're ignoring errors, we should probably only ignore them from the RemoteMediator constructor.

If creating InstallTrigger throws what are we supposed to do?
I suspect the generated wrappers that call this just catch and do the same thing (don't inject InstallTrigger) if `_create()` throws prior to this change.

	Kris Maglione [:kmag]
	Comment 17 • 8 years ago
mozreview-review-reply

Comment on attachment 8799993 [details]
Bug 1091924 Fix up InstallTrigger for e10s-multi

https://reviewboard.mozilla.org/r/85022/#review84198

> If creating InstallTrigger throws what are we supposed to do?
> I suspect the generated wrappers that call this just catch and do the same thing (don't inject InstallTrigger) if `_create()` throws prior to this change.

If it throws, we should still fail. But we should see the error rather than ignoring it.

	Pulsebot
	Comment 19 • 8 years ago

Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f903be214d27
Fix up InstallTrigger for e10s-multi r=kmag

	Carsten Book [:Tomcat]
	Comment 20 • 8 years ago
bugherder

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

	Andrei Vaida [:avaida]
	Comment 21 • 7 years ago

Andrew, is this something that manual QA could help test?

	Andrew Swan [:aswan] Assignee
	Comment 22 • 7 years ago

I don't think so, as Mossop mentions in comment 1, the buggy behavior isn't practical to deliberately trigger.  I think our only real option is all the automated testing we have to ensure that the mechanics of installs are working properly coupled with manual review of the message passing design to confirm that is is inherently e10s-multi-safe (which should be covered by Kris's review of the patch)