Closed Bug 814583 Opened 12 years ago Closed 12 years ago

Fix broken lockscreen after tutorial due to mozbrowserclose being fired twice

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-basecamp:+)

RESOLVED FIXED
blocking-basecamp +

People

(Reporter: alive, Assigned: ochameau)

References

Details

Attachments

(2 files)

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.
What is FTU?
(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
Assignee: nobody → schien
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.
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.
(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.
> 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.
(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.
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.
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().
Attachment #685493 - Flags: feedback?(justin.lebar+bug)
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.
Attachment #685493 - Flags: feedback?(justin.lebar+bug) → feedback-
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.
blocking-basecamp: ? → -
(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?
Attached file Pull request 6690 (deleted) —
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!
Assignee: schien → poirot.alex
Attachment #686174 - Flags: review?(alive)
Renominating with a gaia workaround and explained in previous comment how bad was the resulting error of these duplicated events.
blocking-basecamp: - → ?
Component: General → Gaia::System
If we're changing this bug into "fix the Gaia issue", could you please modify the summary to reflect this?
Summary: mozbrowserclose is fired twice, causing buggy behavior in window manager → Fix broken lockscreen after tutorial due to mozbrowserclose being fired twice
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.
Attachment #686174 - Flags: review?(alive) → review+
blocking-basecamp: ? → +
Priority: -- → P2
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: