STR: 1. Go to UI Tests app, window.open test 2. Click [window.close()] Expected: 1. window_manager.js received mozbrowserclose event and kill the UI Tests app Actual: 1. nothing. Note: The popup frame and background service frame can be closed within the UI tests -- event listeners in three scripts (window_manager, background_service, and popup_manager) all received the event and the frame is closed correctly. The only difference between these frames I can think of is that the app frame is not created with window.open. Impact: I was going to use window.close() to finish the disposition inline web activities flow (to close the completed "picker" frame); I can workaround it by have the requester app to open itself again (mozApps.getSelf().oncomplete -> app.launch()), but that's not a good flow IMO.

Indeed, window.close() does nothing in the Normal Web (tm) if you weren't opened from script. I'm not convinced we should depart from that behavior for mozbrowser.

(In reply to Justin Lebar [:jlebar] from comment #1) > Indeed, window.close() does nothing in the Normal Web (tm) if you weren't > opened from script. I'm not convinced we should depart from that behavior > for mozbrowser. So web apps are not allowed to close itself?

> So web apps are not allowed to close itself? Is that a common idiom on mobile devices? None of the apps on my android phone have an option to close themselves.

This doesn't seem like a Basecamp blocker based on comment #1. Actually, based on comment #1, it seems like it's not a bug.

CCing Josh for input. (In reply to Justin Lebar [:jlebar] from comment #3) > > So web apps are not allowed to close itself? > > Is that a common idiom on mobile devices? None of the apps on my android > phone have an option to close themselves. It's not common but it does happen. There are some apps, when encountering unrecoverable error, closes itself after a prompt. I am more concern on some other cases where we (system app) created the frame but we rely on the app to tell us when to close it. One of them being the background service frame, the other being the inline activity frame. Currently we impose a hack (grep for |reopenApp|) and rely on the requesting app to tell us when to close the inline frame. (In reply to Justin Lebar [:jlebar] from comment #1) > Indeed, window.close() does nothing in the Normal Web (tm) if you weren't > opened from script. I'm not convinced we should depart from that behavior > for mozbrowser. I didn't really respond to this. The Normal Web (tm) behavior is correct, but I think it should be handled by Browser app in Gaia; Gecko should not assume and impose this behavior to System app.

> The Normal Web (tm) behavior is correct, but I think it should be handled by Browser app in Gaia; > Gecko should not assume and impose this behavior to System app. The question is less about imposing behavior, I think, and more about the degree to which we want uniformity of behavior across the web platform. A browser app should not be able to change the way the web works. > I am more concern on some other cases where we (system app) created the frame but we rely on the > app to tell us when to close it. The question is whether you should be using window.close for this notification. That's not the only option, although I agree it's a pretty natural one, and I don't have a better suggestion. I'd like to get someone else's opinion on this, because I'm still not entirely sold. Mounir, how would you feel about firing an event when a window calls close() but we would normally not allow it to close? We'll have to distinguish this from the case in which a window calls close() and we /would/ normally allow it to close. Do you need this for V1?

> It's not common but it does happen. There are some apps, when encountering unrecoverable error, closes itself after a prompt. Interesting questions... * Do we allow apps to suicide? * If so, do we let them do it with window.close? > It's not common but it does happen. There are some apps, when encountering unrecoverable error, closes itself after a prompt. I assumed the System did the closing? The apps encounter an error, and then the system decides how to handle that state. In general I do not like the idea of taking power out of the System (and user's) hands, and turning it over to the individual app. I think suiciding gives apps too much power. The app should be able to request a suicide, but System should retain final decision making authority. Apps are not windows, to be closed casually. I also agree with Justin's comments re: not altering default web behaviors, although I defer to the experts (you guys) on that one.

> The app should be able to request a suicide, but System should retain final decision making > authority. We'll likely meet this criterion with any solution we choose, due to the way the browser API is designed.

I think apps should be allowed to close themselves for three reasons: 1. While apps are mostly like the normal web, they are different in some ways. In particular we generally allow apps to do things that use resources or "can annoy" the user. 2. Apps can effectively close themselves no matter what we do. They can just navigate to an empty page or remove the DOM from their own page. So stopping window.close() just gives developers a hurdle they can easily jump over. 3. The fact that we forbid window.close() on the web is mostly due to lazyness. Ideally we should allow it, but make it so that it doesn't close the window/tab, but rather replaces it with an empty page. That way the closed window doesn't cause the user to lose any navigation session history, or prevent them from keeping to use the window for navigating elsewhere. That said, I don't think this is a priority.

I tend to agree with Jonas: we can make window.close() working for apps. Let's make that working if appStatus != NOT_INSTALLED. That way, no regular content will be able to abuse that. (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #0) > I was going to use window.close() to finish the disposition inline web > activities flow (to close the completed "picker" frame); I can workaround it > by have the requester app to open itself again (mozApps.getSelf().oncomplete > -> app.launch()), but that's not a good flow IMO. I'm wondering which tab/app/window would call window.close() in your example? Ideally, the activity handler should be opened by the system app (or anything similar) and closing the activity handler should be done by the exact same app. The handling app shouldn't have to do that because it's an implementation detail and that should not leak into the app.

Okay. I think that this feature, as described in comment 10, is a pretty simple change to nsGlobalWindow::OpenInternal.

> We'll likely meet this criterion with any solution we choose, due to the way the browser API is designed. I can accept allowing apps to close themselves, so long system retains control of how that closure is presented to the user. For example, if foreground app closes self, we might want to animate that closure, perhaps into an error message.

Doesn't feel like a blocker. Feel free to disagree :)

I would like to raise a bit the priority, i kind of need that for the first run experience.

(In reply to Mounir Lamouri (:mounir) from comment #10) > I tend to agree with Jonas: we can make window.close() working for apps. > Let's make that working if appStatus != NOT_INSTALLED. That way, no regular > content will be able to abuse that. > > (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #0) > > I was going to use window.close() to finish the disposition inline web > > activities flow (to close the completed "picker" frame); I can workaround it > > by have the requester app to open itself again (mozApps.getSelf().oncomplete > > -> app.launch()), but that's not a good flow IMO. > > I'm wondering which tab/app/window would call window.close() in your > example? Ideally, the activity handler should be opened by the system app > (or anything similar) and closing the activity handler should be done by the > exact same app. The handling app shouldn't have to do that because it's an > implementation detail and that should not leak into the app. I realized I've failed to answer this question. The example would be the [X] button on the contact picker. It currently sends an empty string to the Message app for it to call |reopenApp|) to dismiss the picker. Even with a successful contact pick, System app still rely on Message app to call |reopenApp| to know when to dismiss the picker. This would also count as yet another leakage of implementation detail I guess. I think the other approach in this example would be more mozChromeEvents from System Message API/Web Activities API to give System app more information (e.g. activity-onerror, activity-onsuccess, activity-onsuccess-return ...). We can spawn to another bug for that.

(In reply to Vivien Nicolas (:vingtetun) from comment #14) > I would like to raise a bit the priority, i kind of need that for the first > run experience. We're basically not working on anything that's not a basecamp blocker, at this point. Do you think this needs to be re-nom'ed?

Tim's use case is for activities with inline disposition. His workaround (having the requesting app re-launch itself when it gets a response) is incredibly horrible. I cringe to imaging asking external developers to develop apps that have to do that. That workaround must not stand. So we've got to fix this bug, or find another solution to Tim's problem. I don't know anything about how activities are implemented, but this seems like something that could be handled by having shell.js send mozChromeEvents to the system app so that the window manager can do the right thing when an activity is completed. Fabrice, does it sound like that could work?

(In reply to David Flanagan [:djf] from comment #17) > Tim's use case is for activities with inline disposition. His workaround > (having the requesting app re-launch itself when it gets a response) is > incredibly horrible. I cringe to imaging asking external developers to > develop apps that have to do that. That workaround must not stand. > > So we've got to fix this bug, or find another solution to Tim's problem. I > don't know anything about how activities are implemented, but this seems > like something that could be handled by having shell.js send mozChromeEvents > to the system app so that the window manager can do the right thing when an > activity is completed. Fabrice, does it sound like that could work? Thanks for the comment, just want to note that I didn't invent that workaround; I simply copied the |reopenApp| code from Message app, which I believe is first developed by TEF devs.

I guess this is all we need?

Comment on attachment 664682 [details] [diff] [review] Patch, v1 r=me, I guess

I'm on PTO for the next while, so if someone else wants to land this, be my guest!

Arrr...not a Try run in sight. https://tbpl.mozilla.org/?tree=Try&rev=8b01728a245a

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/5ed440d28b53 - I know your landing life sucks, but the first push after this landed which was unbroken enough to actually run a test ran https://tbpl.mozilla.org/php/getParsedLog.php?id=15612753&tree=Mozilla-Inbound, asserting and dying in test_browserElement_oop_CloseFromOpener.html, and when I went looking for someone to back out, there you were.

Android, from whom we're all always eager to hear, says https://tbpl.mozilla.org/php/getParsedLog.php?id=15613950&tree=Mozilla-Inbound

We should just disable the test on Android, but I can do that when I get back.

Pushed to try to check whether the macosx failure is real (I'd guess it's unrelated) https://tbpl.mozilla.org/?tree=Try&rev=98b0fc70e159

And backed out, because it appears that this assertion failure is intermittent. :-/ https://hg.mozilla.org/integration/mozilla-inbound/rev/2e4866b959fc

Can't remember whether this was clear yet while I was totally forgetting that I'd backed this patch out for this very same assertion just five days ago, but, Linux32 was much much more prone to hitting it this landing, like 50%, and will be easier to tell whether it's gone.

I think this test failure has nothing to do with the nsGlobalWindow change. Instead, the new CloseApp test plus CloseFromOpener may be exposing a bug in ContentParent somehow...

Let's see if bug 797515 does the trick. https://tbpl.mozilla.org/?tree=Try&rev=5b687f5f2991

Sigh, -u linux, not -u linux32. https://tbpl.mozilla.org/?tree=Try&rev=f7cc946ee1f5

Guess that didn't work. Backed out for M2 orange. https://hg.mozilla.org/integration/mozilla-inbound/rev/16ac5cbc7793 https://tbpl.mozilla.org/php/getParsedLog.php?id=15792667&tree=Mozilla-Inbound 199 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/browser-element/mochitest/test_browserElement_inproc_CloseApp.html | Should have gotten app frame close by now. 588 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/browser-element/mochitest/test_browserElement_oop_CloseApp.html | Should have gotten app frame close by now.

Bitten by bug 777384, which I myself pushed. :( I will land this!

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9d8e474c709 https://hg.mozilla.org/integration/mozilla-inbound/rev/bd97a43a4603 Quick, people.m.o just went down so nobody is on IRC; time to land!