Closed Bug 372453 Opened 13 years ago Closed 13 years ago

XULRunner apps crash if you use Quit or Preferences after all the windows are dead

Categories

(Toolkit Graveyard :: XULRunner, defect, critical)

x86
macOS
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Gijs, Assigned: Gijs)

References

()

Details

(Keywords: regression)

Attachments

(2 files, 3 obsolete files)

Attached file Apple Crash report
Steps to reproduce:

1. Install trunk xulrunner (with cocoa widgets)
2. Follow steps 2 and 3 on http://cz.rdmsoft.com/xr/ to install ChatZilla for xulrunner.
3. Open ChatZilla
4. type /quit in the input box and hit enter/return.
5. Open the ChatZilla menu (the app one, so the leftmost one)
6. Click Quit.

Expected results:
Process quits

Actual results:
Process crashes

I'm not really sure what's causing this. Note though, that ChatZilla doesn't (yet) use the magic needed to hook up to the Mac about and quit menus. This probably has something to do with it.

Also, I vaguely recall using one of my own debug builds and hitting an assertion/warning when opening the App menu while there was still an app window left. Something about an event firing but not getting to the event handler? This is probably Bad :-)

Additionally, the Services and hide/show window entries are not in the app menu. This probably also has something to do with it.

Note that the magic to hook up to the app menu should be optional, as far as I can tell. It should certainly not crash :-)

(marking regression because this doesn't occur on XULrunner 1.8.0.4 which still had carbon widgets, I think?)
Version: unspecified → Trunk
bug 368911 will fix the quit issue, according to Mano. That still leaves the issue with preferences though.
Depends on: 368911
Do you still crash with the last patch on bug 368911?
Flags: blocking1.9?
I think the first question that needs to be answered here is why are we getting those alerts about the hidden window.
I can attempt to look into this again on thursday. If it needs to be done sooner, you will have to ask someone else, sorry.
That's great, thanks
Uh, so just to check, you are aware that ChatZilla does in fact not ship a hidden window (nor does XULRunner itself), which is why it doesn't find anything - right?
By default "browser.hiddenWindowChromeURL" is set to a chrome file you don't have, which is why the alerts. If that is set to "about:blank" they go away. Then we still have this crash :)
Actually, the pref isn't set at all in xulrunner. It doesn't even exist. The constant in the source file is set to a file xulrunner apps won't have unless they override it, though. So what should be done, imho, is change the constant in the source file. For which I filed bug 375861.

And you're right about the crashing part, but at least changing that thing fixes the behaviour of the Quit item while windows are available (which is broken still at the moment, most likely because of the alert box...).

I'll try and poke this code a bit more after I attach a patch to bug 375861.
Depends on: 375861
Mmk, so if we really fix this by just quitting xulrunner apps that don't ship their own hiddenWindow, then there's no sense for this being in widget. Moving...
Assignee: joshmoz → nobody
Component: Widget: Cocoa → XULRunner
Product: Core → Toolkit
QA Contact: cocoa → xulrunner
Attached patch Patch (obsolete) — Splinter Review
What it does:
- Add a readonly attribute to nsIAppshellService which corresponds to whether or not the app is using their own hiddenWindow.xul. I abstracted this out in favour of grabbing the hiddenWindow and checking its location URL separately - the latter seemed less reliable as well as relying on internal behaviour of the appshellservice from other parts of the code, which was something I thought would be undesirable. This could be an alternative approach, though.
- When a window goes away, check whether we:
  - only have one window left
  - check that we have a hiddenWindow still (this and the above should imply that we *only* have the hiddenWindow left. I'm saying 'should' because while looking for ways to fix this bug I came across things like
http://lxr.mozilla.org/seamonkey/source/mailnews/compose/src/nsMsgComposeService.cpp#337
which scared me somewhat.
  - check that the hiddenWindow was not provided by the app.

If all of the above is true, the app will quit.


I've tested this patch with the patch from bug 375861 applied (warning: they conflict. I reverted that patch in order to do get a patch for this one) using ChatZilla on xulrunner. I also tested the profilemanager for ChatZilla on xulrunner. I haven't tested it on Firefox yet. I will do so shortly - I don't expect any problems though: Firefox by default overrides the hiddenWindow URL (see http://lxr.mozilla.org/seamonkey/source/browser/app/profile/firefox.js#54), as do Calendar, SeaMonkey and Thunderbird.

Benjamin, could you take a look at this (perhaps after the patch for bug 375861)?
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attachment #260247 - Flags: first-review?(benjamin)
I've verified this doesn't break Firefox trunk.
Comment on attachment 260247 [details] [diff] [review]
Patch

Instead of putting all this logic here, can't we put it at startup where we increment considerquit?:
http://lxr.mozilla.org/mozilla/source/toolkit/components/startup/src/nsAppStartup.cpp#166
Once you have r+ from bsmedberg or someone else please request a review from me. I'd like to look over this before it lands.
Josh: sure, I'll get a review from you as well for the next patch I do. :-)

bsmedberg and I talked this over on IRC and thought it would also be possible to move the logic about the hiddenWindow into Quit(eConsiderQuit), and not increment mConsiderQuitStopper when we open the hiddenWindow. That's what I'll be doing next, as soon as I get time to poke at this again.
Attachment #260247 - Flags: first-review?(benjamin) → first-review+
Attached patch Patching Quit() instead (obsolete) — Splinter Review
This does what I told bsmedberg I'd make it do, I think... and it's been tested and doesn't break Minefield, and works on xulrunner with ChatZilla. I'll set an additional r? on Josh in a bit, seems bugzilla doesn't let me do that immediately.
Attachment #260247 - Attachment is obsolete: true
Attachment #264244 - Flags: review?(benjamin)
Attachment #264244 - Flags: review?(joshmoz)
Attachment #264244 - Flags: review?(benjamin) → review+
Attachment #264244 - Flags: review?(joshmoz) → review+
Checking in mozilla/toolkit/components/startup/src/nsAppStartup.cpp;
/cvsroot/mozilla/toolkit/components/startup/src/nsAppStartup.cpp,v  <--  nsAppStartup.cpp
new revision: 1.14; previous revision: 1.13
done
Checking in mozilla/xpfe/appshell/public/nsIAppShellService.idl;
/cvsroot/mozilla/xpfe/appshell/public/nsIAppShellService.idl,v  <--  nsIAppShellService.idl
new revision: 1.32; previous revision: 1.31
done
Checking in mozilla/xpfe/appshell/src/nsAppShellService.cpp;
/cvsroot/mozilla/xpfe/appshell/src/nsAppShellService.cpp,v  <--  nsAppShellService.cpp
new revision: 1.232; previous revision: 1.231
done
Checking in mozilla/xpfe/appshell/src/nsAppShellService.h;
/cvsroot/mozilla/xpfe/appshell/src/nsAppShellService.h,v  <--  nsAppShellService.h
new revision: 1.30; previous revision: 1.29
done
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
So I had to back this out because the test boxen went orange. I'm pretty sure there's a couple of things at work here. One of which is that I only set the "mUsefulHiddenWindow" property on mac - it should probably just be true on other platforms. Another is that I'm pretty sure either the open window tracking is getting lost (so mConsiderQuitStopper == 0 while there are open windows) or the hiddenWindow suddenly managed to start existing on other platforms (!hiddenWindow fails). On mac, only the mConsiderQuitStopper issue really explains what's going on, as far as I can tell... I will try to work on this again in a week's time when I've finished uni exams and have a windows box at my disposal again. If anyone figures this out before then, please do comment here.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Same patch with #ifdef magic (obsolete) — Splinter Review
This is exactly the same patch, but with ifdefs so nothing changes for those not on Mac OS. I #ifdef-ed both the code in Quit(), which shouldn't change for other callers, and the code in ExitLastWindowClosingSurvivalArea which shouldn't bother calling Quit() at all for every one-but-last window-close on non-mac, as that could get expensive.

I've tested this patch on both Windows and Mac.
Attachment #264244 - Attachment is obsolete: true
Attachment #269311 - Flags: review?(benjamin)
Attachment #269311 - Flags: review?(joshmoz)
Attachment #269311 - Flags: review?(joshmoz) → review+
Attachment #269311 - Flags: review?(benjamin) → review+
This is the same patch that has r+, but now actually applying to the *current* source code. Asking for approval as nobody has set blocking+ or wanted+.
Attachment #269311 - Attachment is obsolete: true
Attachment #277573 - Flags: review+
Attachment #277573 - Flags: approval1.9?
Comment on attachment 277573 [details] [diff] [review]
Same patch minus the bitrot in the context

a=bzbarsky, but in general a risk assessment would be very nice.

Also, why not make that member a PRPackedBool?  ;)
Attachment #277573 - Flags: approval1.9? → approval1.9+
With Boris' suggestion:

Checking in mozilla/toolkit/components/startup/src/nsAppStartup.cpp;
new revision: 1.18; previous revision: 1.17
Checking in mozilla/xpfe/appshell/public/nsIAppShellService.idl;
new revision: 1.34; previous revision: 1.33
Checking in mozilla/xpfe/appshell/src/nsAppShellService.cpp;
new revision: 1.235; previous revision: 1.234
Checking in mozilla/xpfe/appshell/src/nsAppShellService.h;
new revision: 1.32; previous revision: 1.31
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Depends on: 393652
Depends on: 395042
Depends on: 393813
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.