714234 - Handle window.close() via DOMWindowClose

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: 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.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 2

[Wesley Johnston (:wesj)]

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.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

(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 4

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a579327de2c2

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a579327de2c2

Comment 7

[Alex Keybl [:akeybl]]

Comment on attachment 584922 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approving for Aurora.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Landed on aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/58458dfc1779

Comment 9

[Cristian Nicolae (:xti)]

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