STR: install an app on marketplace from an e10s tab result: endless throbber in the install button, no app installed.

Comment on attachment 8555535 [details] [diff] [review] Installing an app on marketplace fails with e10s. r=? This appears to work. I stash the outerWindowID on both remote and non-remote <xul:browser>'s now, and have <xul:tabbrowser> manage a map from outerWindowIDs to <xul:browser>'s. Since the keys are integers, I can't use a WeakMap, unfortunately, so I remove them on removeTab. Hopefully there aren't places where I might leak here. Remote <xul:browser>'s also don't get outerWindowIDs right away - we wait until Browser:Init is fired in the browser-child.js script, and then set the field on the browser. This means that when we open a new remote tab, we defer adding the browser to the mapping until after Browser:Init is fired. For the WebappManager.jsm bits, I was surprised to learn that I had to send down a message to subscribe to certain messages from the child. This was not necessary with a single process. Anyhow, adding in the RegisterForMessages (and UnregisterForMessages), seems to do the trick. So, requesting review from myk on the WebappManager.jsm bits, and felipe for the rest.

Review ping

Comment on attachment 8555535 [details] [diff] [review] Installing an app on marketplace fails with e10s. r=? Review of attachment 8555535 [details] [diff] [review]: ----------------------------------------------------------------- I'm sorry for the delay reviewing! The WebappManager.jsm changes look good, and this works as expected. r=myk

Bah - I knew it was too good to be true. I'm failing some tests in a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=93c01d071ecf Digging into it, I seem to have broken installing apps from iframes. This is because we do a comparison against the browser.currentURI.spec and the "from" property of the data sent up over webapps-ask-install. Before, we get the window via the outerWindowID, and checked the window's location.href against the from property. Getting to an iframe within the content process to compare the URIs is going to be slow and awful in e10s-land. :/ myk - is there much value in doing that comparison[1] for webapps-ask-install and webapps-ask-uninstall? Is this to ensure that the user isn't being displayed an install notification for some site (either in an iframe or not) that they've browsed away from before the messages have been serviced? [1]: https://hg.mozilla.org/mozilla-central/file/be65d1fde126/browser/modules/WebappManager.jsm#l91

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #9) > myk - is there much value in doing that comparison[1] for > webapps-ask-install and webapps-ask-uninstall? Is this to ensure that the > user isn't being displayed an install notification for some site (either in > an iframe or not) that they've browsed away from before the messages have > been serviced? It's more to ensure that the user doesn't see a request to install an app that appears to have come from a different site than the one that it actually came from, because the requesting site navigated to a different site right after requesting the installation. I've cc:ed you on bug 771294, which is the reason that we implemented that check.

Alright, so having asked around in #openwebapps, it sounds like we want to be able to support having apps install from iframes on desktop. I think I can solve this by having the webapps-ask-install and webapps-ask-uninstall notifications pass around not only the outerWindowID of the content window that requested the app install, but also the outerWindowID of the root content docshell item. Something along those lines. Then, the Desktop front-end just needs to check for that latter outerWindowID to figure out which <browser> is requesting to open the notifications. Ensuring that the iframes have not browsed away in the meantime (the problem I mentioned in comment 8 and myk responded to in comment 9), I have not yet solved. Still working on solutions.

/r/3703 - Bug 1077168 - Have Webapps.js pass up the outerWindowID of the top window with each message. r=? /r/3705 - Bug 1077168 - Make WebappManager.jsm use the top window id to select which browser to show notifications in. r=? /r/3707 - Bug 1077168 - Have RemoteWebProgress dispatch DOMWindowID's to nsIWebProgressListener's. r=? /r/3709 - Bug 1077168 - Notice when any content requesting Webapp permissions browses away, and remove the notification. r=? /r/3711 - Bug 1077168 - Cancel in-flight Webapp install jobs from windows that change location. r=? Pull down these commits: hg pull review -r e7d554aad3b719e329ca3357731e46eb88d673d5

Ok, I think this addresses the test failures - I'll push to try next. felipe - r/3705 is very similar to the last incarnation of this bug fix, but that it uses the top window ID instead of the oid in order to determine which browser to show the notification from. The rest is (what I believe to be) minor plumbing. sicking - I've noticed your name on many reviews for changes landing on dom/apps/Webapps.js and dom/apps/Webapps.jsm, so I'm sending the review request for that last commit your way. The commit message[1] should help explain what I'm trying to do, but feel free to ask me if you have any questions. And I'm using MozReview because of the multiple-commits, but let me know if you'd like it split up into Bugzilla patches instead. [1]: https://reviewboard.mozilla.org/r/3711/

Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ea74f8d3cf6

Review ping

Comment on attachment 8563679 [details] MozReview Request: bz://1077168/mconley /r/3703 - Bug 1077168 - Have Webapps.js pass up the outerWindowID of the top window with each message. r=? /r/3705 - Bug 1077168 - Make WebappManager.jsm use the top window id to select which browser to show notifications in. r=? /r/3707 - Bug 1077168 - Have RemoteWebProgress dispatch DOMWindowID's to nsIWebProgressListener's. r=? /r/3709 - Bug 1077168 - Notice when any content requesting Webapp permissions browses away, and remove the notification. r=? /r/3711 - Bug 1077168 - Cancel in-flight Webapp install jobs from windows that change location. r=? Pull down these commits: hg pull review -r e7d554aad3b719e329ca3357731e46eb88d673d5

https://reviewboard.mozilla.org/r/3709/#review3195 ::: browser/modules/WebappManager.jsm (Diff revision 1) > + notification.remove(); same as above ::: browser/modules/WebappManager.jsm (Diff revision 1) > + notification.remove(); the listener should also be removed here together with the notification, right?

https://reviewboard.mozilla.org/r/3703/#review3197 Ship It!

https://reviewboard.mozilla.org/r/3705/#review3193 ::: browser/modules/WebappManager.jsm (Diff revision 1) > - if (win && win.location.href == data.from) { > + if (browser) { this location.href == data.from is removed here and not added anywhere else. Isn't this a problem? IIRC this was used to ensure that the website hadn't changed while these messages were received (i.e., it's still the same that triggered the app install). I thought there was even a test for this..

https://reviewboard.mozilla.org/r/3707/#review3201 Ship It!

(In reply to :Felipe Gomes from comment #21) > https://reviewboard.mozilla.org/r/3705/#review3193 > > ::: browser/modules/WebappManager.jsm > (Diff revision 1) > > - if (win && win.location.href == data.from) { > > + if (browser) { > > this location.href == data.from is removed here and not added anywhere else. > Isn't this a problem? > IIRC this was used to ensure that the website hadn't changed while these > messages were received (i.e., it's still the same that triggered the app > install). > > I thought there was even a test for this.. There was - I address this in the later patch that I'm having myk review. Basically, we have two cases: the requesting frame browses away before the notification is shown, and after the notification is shown. The after case is handled by the progress listener you saw me add in WebappsManager.jsm. The before case is trickier, and I had to modify Webapps.jsm and Webapps.js to account for it.

https://reviewboard.mozilla.org/r/3705/#review3225 > this location.href == data.from is removed here and not added anywhere else. Isn't this a problem? > IIRC this was used to ensure that the website hadn't changed while these messages were received (i.e., it's still the same that triggered the app install). > > I thought there was even a test for this.. I responded in the bug, but I should respond here as well: Checking for the outer window location won't work because we can't get to the outer window in the e10s case. I had to find an alternative solution. I address this case in the later patch that I'm having myk review. Basically, we have two cases: the requesting frame browses away before the notification is shown, and after the notification is shown. The after case is handled by the progress listener you saw me add in WebappsManager.jsm. The before case is trickier, and I had to modify Webapps.jsm and Webapps.js to account for it.

https://reviewboard.mozilla.org/r/3705/#review3227 Ship It!

https://reviewboard.mozilla.org/r/3709/#review3229 > the listener should also be removed here together with the notification, right? The listener is removed in the eventCallback for the "removed" event. Or did I miss something?

https://reviewboard.mozilla.org/r/3709/#review3231 > The listener is removed in the eventCallback for the "removed" event. Or did I miss something? ah ok, the manual call to remove will also trigger that eventCallback? If yes, then that looks fine Out of curiosity, why do you check for the existence of `gBrowser`?

https://reviewboard.mozilla.org/r/3709/#review3233 Ship It!

Comment on attachment 8563679 [details] MozReview Request: bz://1077168/mconley Looks great, thanks for digging into this!

https://reviewboard.mozilla.org/r/3709/#review3277 > ah ok, the manual call to remove will also trigger that eventCallback? If yes, then that looks fine > > Out of curiosity, why do you check for the existence of `gBrowser`? I guess it's because `getBrowserForID` can, in the fallback case, query Services.wm for some outer window with a particular ID, and that outer window is not guaranteed to have a gBrowser. It's very much an edge case, but I think it's a possible one.

Thanks for the reviews! I don't want to land this until after the merge though - especially since 38 is an ESR. I'll wait until after the uplift is complete.

Thanks for the reviews! remote: https://hg.mozilla.org/integration/fx-team/rev/860e858f8c7f remote: https://hg.mozilla.org/integration/fx-team/rev/068607e38210 remote: https://hg.mozilla.org/integration/fx-team/rev/1fa341ee5820 remote: https://hg.mozilla.org/integration/fx-team/rev/9d74f10975e8 remote: https://hg.mozilla.org/integration/fx-team/rev/9fbebdeeeedb

https://hg.mozilla.org/mozilla-central/rev/860e858f8c7f https://hg.mozilla.org/mozilla-central/rev/068607e38210 https://hg.mozilla.org/mozilla-central/rev/1fa341ee5820 https://hg.mozilla.org/mozilla-central/rev/9d74f10975e8 https://hg.mozilla.org/mozilla-central/rev/9fbebdeeeedb

Ugh, now we're relying on this feature (outerWindowId on <browser>) for Loop's tab sharing, which we'd like to uplift to Aurora. Mike, can you provide a risk assessment to have the patches here uplifted to Aurora? Thanks!

(In reply to Mike de Boer [:mikedeboer] from comment #35) > Ugh, now we're relying on this feature (outerWindowId on <browser>) for > Loop's tab sharing, which we'd like to uplift to Aurora. Mike, can you > provide a risk assessment to have the patches here uplifted to Aurora? > > Thanks! Hey - I wouldn't take _all_ of the patches. I think you want https://hg.mozilla.org/mozilla-central/rev/068607e38210 - specifically, just some fragment of it. I should have been better at splitting up my changes. :/ Luckily, I don't think this one is too bad. Basically, in 068607e38210, I think you just want the fragment that adds outerWindowId to the XBL binding: https://hg.mozilla.org/mozilla-central/rev/068607e38210#l4.1 If you care about <browser>'s with remote="true" (I have no idea if you do), then you'll need the change to remote-browser.xml, as well as the change in browser-child.js. That _should_ give you all of the things you need. Would you like me to split up the patch for you?

Alright, that sounds like a goo plan. I already looked at the patches and it's certainly not complicated. I'll create a bug, put up the extracted logic there in a patch and request review from you. ;-)

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #32) > remote: https://hg.mozilla.org/integration/fx-team/rev/9d74f10975e8 The eventCallback doesn't work at all. The patch in bug 1167146 should fix that.

Oh yikes - sorry about that! I seem to recall testing that pretty thoroughly, so that's pretty surprising, tbh.

Adding dev-doc-needed keyword because this added the outerWindowID property on xul:browser and this one is still undocumented.

Comment on attachment 8618391 [details] MozReview Request: Bug 1077168 - Have Webapps.js pass up the outerWindowID of the top window with each message. r=? https://reviewboard.mozilla.org/r/3701/#review85980

Comment on attachment 8618387 [details] MozReview Request: Bug 1077168 - Cancel in-flight Webapp install jobs from windows that change location. r=? https://reviewboard.mozilla.org/r/3711/#review85998