814583 - Fix broken lockscreen after tutorial due to mozbrowserclose being fired twice

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect, P2)

Description

[(inactive after 6/18) Alive Kuo [:alive]]

https://bugzilla.mozilla.org/show_bug.cgi?id=781452#c3
When working in bug 813541, I find something is wrong in window manager when the FTU is terminated.

The root cause is as title.

We didn't have one app to call window.close() to terminate itself before,
but now FTU does.

What the buggy behavior is,
1. Window Manager is calling setDisplayedApp to homescreen from FTU, and set the callback as removeFrame
2. setDisplayedApp calls closeWindow
3. closeWindow wait a while(100ms) and start the frame transition by set the class 'closing' to closeFrame(which is FTU iframe now)
4. Another mozbrowsererror coming, but this time displayedApp is already homescreen.
5. kill calls removeFrame immediately, which remove the FTU iframe.
6. Callback in No.3 tries to do something to the FTU iframe but fails.
7. Callback in No.3 tries to call removeFrame again but fails.

Comment 1

[Justin Lebar (not reading bugmail)]

What is FTU?

Comment 2

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Justin Lebar [:jlebar] from comment #1)
> What is FTU?

First Time Usage app, which sets up some configuration for the user by quickly next step.
https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/ftu/js/ui.js#L120

Comment 3

[Justin Lebar (not reading bugmail)]

I don't know how you're triggering two mozbrowserclose events, unless you're calling .close() on the frame twice.

But this should be very simple to work around in Gaia; I might try to do that, if it was me.

Comment 4

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

window.close()　is triggered twice because of consecutive click events, by quickly tapping the button on the last page of FTU. 
http://dxr.mozilla.org/mozilla-central/dom/base/nsGlobalWindow.cpp.html#l6566
DispatchCustomEvent("DOMWindowClose") will return false because |preventDefault| is invoked in BrowserElementChild.js. If nsGlobleWindow::FinalClose is invoked, we'll be able to prevent close twice by |mHavePendingClose|.
I think we also need to guarantee the correctness of reentering nsGlobalWindow::Close under the |preventDefault| scenario.
Setting |mHavePendingClose = true| if DispatchCustomEvent returns false should resolve this bug.

Comment 5

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Justin Lebar [:jlebar] from comment #3)
> I don't know how you're triggering two mozbrowserclose events, unless you're
> calling .close() on the frame twice.
> 
> But this should be very simple to work around in Gaia; I might try to do
> that, if it was me.
I have already a workaround about this in gaia in my local branch.(but is not in the build now).
However, the count of calling window.close() equals to how many events seems we are getting seems not a correct behavior to me.

Comment 6

[Justin Lebar (not reading bugmail)]

> However, the count of calling window.close() equals to how many events seems we are 
> getting seems not a correct behavior to me.

This happens because of the way window.close() is implemented in the browser API.

When the embedder receives mozbrowserclose, the embedder must either remove the frame from the DOM or do nothing, which constitutes "blocking" the window.close().  It's therefore hard to tell whether a second window.close() should result in a second mozbrowserclose event.

Shih-Chiang may be onto something in comment 4; I'd be happy to look at a patch.  But I don't think this should be a high priority for me to fix myself, since you can work around this bug without too much pain.

Comment 7

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Justin Lebar [:jlebar] from comment #6)
> This happens because of the way window.close() is implemented in the browser
> API.
> 
> When the embedder receives mozbrowserclose, the embedder must either remove
> the frame from the DOM or do nothing, which constitutes "blocking" the
> window.close().  It's therefore hard to tell whether a second window.close()
> should result in a second mozbrowserclose event.
> 
> Shih-Chiang may be onto something in comment 4; I'd be happy to look at a
> patch.  But I don't think this should be a high priority for me to fix
> myself, since you can work around this bug without too much pain.

Agree on there is a simple fix for this, but the time I lost on checking this item is not simple :)
Anyway others or me will send a patch for gaia if you folks determine not to fix or modify this. I agree fix in gecko is in-efficient.

Comment 8

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Even window.close() is called, the embedder can still decide not to remove the frame. If the first closing attempt is blocked and the mozbrowserclose event can only be fired once, the app will not be able to try to close itself again. Maybe we could let the mozbrowserclose be fired again if previous one was canceled by the embedder.

Comment 9

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: prevent reenter nsGlobalWindow::Close

Still set |mHavePendingClose| to true while someone prevent the default action for DOMWindowClose.

NOTE: This patch is based on an assumption that invoke |preventDefault()| on DOMWindowClose event cannot be used for cancelling window.close().

Comment 10

[Justin Lebar (not reading bugmail)]

Comment on attachment 685493 [details] [diff] [review]
prevent reenter nsGlobalWindow::Close

I think this is what Patrick (comment 8) and I (comment 6) were explaining won't work.

BrowserElementChild.js always calls PreventDefault on DOMWindowClose.  Then it's up to the embedder to carry out the mozbrowserclose event (by removing the frame from the DOM) or not (by doing nothing).

As I understand it, this code assumes that we will always carry out the close event.

Comment 11

[Doug Turner (:dougt)]

Since there is a workaround, we are minusing blocking-basecamp.  We'd like to see this fixed, but we are not going to block on fixing this.  If there is a low risk patch, please nom that patch for uplift.  Thanks.

Comment 12

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Justin Lebar [:jlebar] from comment #10)
> BrowserElementChild.js always calls PreventDefault on DOMWindowClose.  Then
> it's up to the embedder to carry out the mozbrowserclose event (by removing
> the frame from the DOM) or not (by doing nothing).
Well then, I think it is an expected behavior for sending out DOMWindowClose multiple times, since we can use preventDefault to cancel the window.close().

Otherwise, we'll need another notification from the one who call preventDefault, either through function call or custom event, so that we can prevent (or allow) to send DOMWindowClose again, and I can imagine more timing issues on this approach.

So, do we have a conclusion that this bug needs to be solved in Gaia part?

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 6690

So I confirm what alive and other have seen after the FTU.
On current master, after a make reset-gaia in order to have the FTU. Right after the tutorial, the lockscreen is completely destroyed. It won't unlock and when you try, sometime it unlock without pressing the round unlock button and launch a random app that you can't escape. (i.e. home button doesn't work)

Here is a gaia patch in order to prevent such huge failure because of unharmfull duplicated events!

Comment 14

[Alexandre Poirot [:ochameau]]

Renominating with a gaia workaround and explained in previous comment how bad was the resulting error of these duplicated events.

Comment 15

[Justin Lebar (not reading bugmail)]

If we're changing this bug into "fix the Gaia issue", could you please modify the summary to reflect this?

Comment 16

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 686174 [details]
Pull request 6690

r=me, although my original thought is to hook something on the app frame dataset.
It's not bad to hook on runningApps I think.

Comment 17

[Alexandre Poirot [:ochameau]]

Landed:
https://github.com/mozilla-b2g/gaia/commit/3c4d9e4c789f5f325dd4c601883818ebeb5a7111