Last Comment Bug 714234 - Handle window.close() via DOMWindowClose
: Handle window.close() via DOMWindowClose
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: Firefox 12
Assigned To: Mark Finkle (:mfinkle) (use needinfo?)
:
Mentors:
Depends on:
Blocks: 712761
  Show dependency treegraph
 
Reported: 2011-12-29 23:16 PST by Mark Finkle (:mfinkle) (use needinfo?)
Modified: 2012-02-06 07:13 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
patch (7.52 KB, patch)
2011-12-29 23:16 PST, Mark Finkle (:mfinkle) (use needinfo?)
wjohnston2000: review+
bugmail: feedback+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Mark Finkle (:mfinkle) (use needinfo?) 2011-12-29 23:16:17 PST
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.
Comment 1 Kartikaya Gupta (email:kats@mozilla.com) 2011-12-29 23:18:16 PST
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) 2011-12-30 11:38:49 PST
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?) 2011-12-30 11:45:16 PST
(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?) 2011-12-30 12:24:10 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/a579327de2c2
Comment 5 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-30 12:25:45 PST
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, back in August) 2011-12-31 19:47:42 PST
https://hg.mozilla.org/mozilla-central/rev/a579327de2c2
Comment 7 Alex Keybl [:akeybl] 2012-01-03 13:32:44 PST
Comment on attachment 584922 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approving for Aurora.
Comment 8 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-03 14:05:52 PST
Landed on aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/58458dfc1779
Comment 9 Cristian Nicolae (:xti) 2012-01-31 04:31:50 PST
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

Note You need to log in before you can comment on or make changes to this bug.