The default bug view has changed. See this FAQ.

Handle window.close() via DOMWindowClose

VERIFIED FIXED in Firefox 11

Status

()

Firefox for Android
General
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: mfinkle, Assigned: mfinkle)

Tracking

unspecified
Firefox 12
x86
Linux
Points:
---

Firefox Tracking Flags

(firefox11 fixed, firefox12 fixed)

Details

Attachments

(1 attachment)

Created attachment 584922 [details] [diff] [review]
patch

While working on the orange failures in bug 712761, we discovered that Fennec crashes if a DOM window tries to close via window.close()

This might be the real reasons for the orange failures.

This patch borrows a bit from the original XUL Fennec bug to support DOMWindowCLose (bug 504352).

The big difference here is Java has the smarts to handle closing a tab and selecting a new one. Therefore we do handle and squelch the DOMWindowClose event in browser.js and then send a request to close the tab to Java via a message.

In Java we merely call Tabs.closeTab(aTab) which then fires more messages and cleans up the tab for us. Tested using the testcase in bug 504352 and it works fine.
Attachment #584922 - Flags: review?(mbrubeck)
Attachment #584922 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 584922 [details] [diff] [review]
patch

Review of attachment 584922 [details] [diff] [review]:
-----------------------------------------------------------------

I'd prefer to put the listener in Tabs.java rather than GeckoApp.java, but otherwise looks good.
Attachment #584922 - Flags: feedback?(bugmail.mozilla) → feedback+
Comment on attachment 584922 [details] [diff] [review]
patch

Review of attachment 584922 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing the review. Looks fine. Why are we using DOMWindowClosed as the message here rather than something like Tab:Close? I see other DOM* messages, so maybe its on purpose?

I think we could probably move a few of these Tab:* messages into Tabs.java, but I'm fine doing that in a follow up.
Attachment #584922 - Flags: review?(mbrubeck) → review+
(In reply to Wesley Johnston (:wesj) from comment #2)
> Comment on attachment 584922 [details] [diff] [review]
> patch
> 
> Review of attachment 584922 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Stealing the review. Looks fine. Why are we using DOMWindowClosed as the
> message here rather than something like Tab:Close? I see other DOM*
> messages, so maybe its on purpose?

Tab:Close is only used to let Java know a Tab was destroyed. It is not used to "start" the Tab close process. The complete "Tab close" process requires selecting a fallback Tab, based on parent or index, then sending Tab:Select and Tab:Close messages to JS. JS then sends back acknowledgement messages to Java letting Java know that a new tab was selected and the desired tab was destroyed.

> I think we could probably move a few of these Tab:* messages into Tabs.java,
> but I'm fine doing that in a follow up.

I agree with this followup cleanup/refactor
https://hg.mozilla.org/integration/mozilla-inbound/rev/a579327de2c2
(Assignee)

Updated

5 years ago
status-firefox11: --- → affected
status-firefox12: --- → affected
Comment on attachment 584922 [details] [diff] [review]
patch

Needed for Aurora as a core feature (handling window.close) and needed because it blocks bug 712761.
Attachment #584922 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/a579327de2c2
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12

Comment 7

5 years ago
Comment on attachment 584922 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approving for Aurora.
Attachment #584922 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Landed on aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/58458dfc1779
status-firefox11: affected → fixed
status-firefox12: affected → fixed
Verified fixed on:

Mozilla/5.0 (Android;Linux armv7l;rv:11.0a2)Gecko/20120130
Firefox/11.0a2 Fennec/11.0a2
Device: Samsung Galaxy S
OS: Android 2.2

Mozilla/5.0 (Android;Linux armv7l;rv:12.0a1)Gecko/20120130
Firefox/12.0a1 Fennec/12.0a1
Device: Samsung Galaxy S
OS: Android 2.2
Status: RESOLVED → VERIFIED
Assignee: nobody → mark.finkle
You need to log in before you can comment on or make changes to this bug.