Handle window.close() via DOMWindowClose

VERIFIED FIXED in Firefox 11

Status

()

defect
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: mfinkle, Assigned: mfinkle)

Tracking

unspecified
Firefox 12
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 fixed, firefox12 fixed)

Details

Attachments

(1 attachment)

Posted patch patchSplinter Review
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
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
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
Comment on attachment 584922 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approving for Aurora.
Attachment #584922 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.