811394 - Style Editor window close exits Firefox

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[cheesypoof]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:19.0) Gecko/19.0 Firefox/19.0
Build ID: 20121113030658

Steps to reproduce:

Click the close button on a 'Style Editor' window.


Actual results:

Firefox stops running.


Expected results:

Only the 'Style Editor' window should have closed.

Comment 1

[Loic]

Regression range:

m-c
good=2012-11-09
bad=2012-11-10
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=90cea19e27e2&tochange=a47525b93528

My guess goes to bug 806996.

Comment 2

[Alice0775 White]

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/983d136130c5
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121109074532
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/bd9729ca4de2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121109080131
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=983d136130c5&tochange=bd9729ca4de2

Regressed by Bug 807226

Comment 3

[Boris Zbarsky [:bzbarsky]]

WARNING: requested removal of nonexistent window: file ../../../../../mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp, line 1250

That's my first guess so far...

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK, so StyleEditor.jsm has an unload handler that calls window.close().  This is getting invoked when we click the close button on the window titlebar, and calls close() on that same window, which ends up reentering nsXULWindow::Destroy, like so:

#0  nsXULWindow::Destroy (this=0x10ed60570) at nsXULWindow.cpp:409
#1  0x000000010410ab67 in nsWebShellWindow::Destroy (this=0x10ed60570) at nsWebShellWindow.cpp:757
#2  0x00000001040ec259 in nsChromeTreeOwner::Destroy (this=0x10e6a3dd0) at nsChromeTreeOwner.cpp:348
#3  0x00000001040ec28c in non-virtual thunk to nsChromeTreeOwner::Destroy() () at nsChromeTreeOwner.cpp:349
#4  0x0000000103614261 in nsGlobalWindow::ReallyCloseWindow (this=0x121645000) at nsGlobalWindow.cpp:6663
#5  0x0000000103613de2 in nsGlobalWindow::FinalClose (this=0x121645000) at nsGlobalWindow.cpp:6610
#6  0x0000000103613be5 in nsGlobalWindow::Close (this=0x121645000) at nsGlobalWindow.cpp:6553
#7  0x000000010361395c in nsGlobalWindow::Close (this=0x121676c00) at nsGlobalWindow.cpp:6488

and then we presumably lose track of whatever magic stuff keeps track of number of open windows and quits the app when the last window closes...

The only reason this used to work is that the unload handler never got called before, because by the time we got there we no longer had an nsIScriptContet.

Paul, I'm not sure what this unload handler is doing, and whether it _really_ expects to be called, but figured you might want to know.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Actually, I'm wrong.  The handler _did_ get called before.  But we used to run the onunload on the JSContext of the window, which means that in nsGlobalWindow::FinalClose we discovered that we're running on the window's context and set a termination function instead of closing for real.  And by the time the termination function ran we'd finished the nsXULWindow::Destroy and hence things were ok.

Oh, and of course the caller is chrome, so nsGlobalWindow::FinalClose won't do async close in this case....

I guess I should just fix nsWebShellWindow::Destroy to handle recursion.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Actually, if the unload handler had dispatched an event to some other window, which had then turned around and called close() on the first window, possibly with a sync XHR in there to make sure there's a null on the JSContext stack, I bet this could have happened even in the old setup.  So yeah, just have to fix nsWebShellWindow.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: This seems to fix the issue for me

Try run pending at https://tbpl.mozilla.org/?tree=Try&rev=858c850cbaa2

Comment 8

[Boris Zbarsky [:bzbarsky]]

[e-mail], thank you for finding and reporting this!  And thank you Loic  and Alice0775 for hunting down what caused the problem.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Thanks for the quick review!

http://hg.mozilla.org/integration/mozilla-inbound/rev/6e9e10baf0dd

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6e9e10baf0dd

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Already in 19, no need to track.