396234 - Crash when dialog comes up before automatic shutdown on Mac

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 384983 for steps to reproduce.  On Mac, those steps cause a crash because we're firing a pagehide event (presumably for this dialog that came up), but sXPConnect is already null when we hit the nsWindowSH code in nsDOMClassInfo.cpp that uses it, so we crash.

Comment 1

[Boris Zbarsky [:bzbarsky]]

The point is, this makes the EM restart thing suck that much worse on Mac, because I can't use -silent to work around it screwing with gdb.

Comment 2

[Boris Zbarsky [:bzbarsky]]

OK, I'm also getting this without -silent, just with the extension update check dialog.  It comes up, it closes, the app sits there unresponsive for a bit, the app crashes.  100% reproducible if I start current trunk after running Fx2 on the same profile.

Comment 3

[Phil Ringnalda (:philor)]

Fixed by and dupe of bug 395707, right?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Seems likely, yes.  At least in terms of reason for crash and symptoms.  I'll try to pull a tree from after that checkin and test.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I'm still seeing this crash with a build pulled on 2007-10-21.  So no, not fixed.

I think the difference is that the window being destroyed here is not the hidden window, as in bug 395707.

Comment 6

[Robert Sayre]

This might have been fixed by other checkins, please check again.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Checkins since comment 5?

I'll try to check tomorrow, I guess.

Comment 8

[Robert Sayre]

Not going to block beta2. Assigning to dtownsend for now.

Comment 9

[Dave Townsend [:mossop]]

It actually is the same problem with bug 395707 with the hidden window. However there I only covered the case of the window existing during an EM restart. In this case a window has appeared (thus creating a hidden window) but the app then never properly runs so the hidden window is not properly disposed of.

This particular case would actually be fixed by bug 384983 but there are potential other ways to hit this I believe, should be a fairly simple fix though.

Comment 10

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This is the way to go I think and a better fix for bug 395707. Now just always destroy the hidden window from the same place that we created it, named in nsAppRunner. Added the DestroyHiddenWindow on nsAppStartup mainly for symmetry and to save a few lines here and there.

There's still a couple of points where we miss it (the NS_ENSURE_SUCCESS etc. points) but since those are only in error circumstances anyway I left those for now.

Comment 11

[Benjamin Smedberg]

Comment on attachment 289302 [details] [diff] [review]
patch rev 1

>Index: toolkit/components/startup/public/nsIAppStartup.idl


> interface nsIAppStartup : nsISupports
> {
>     /**
>      * Create the hidden window.
>      */
>     void createHiddenWindow();
> 
>     /**
>+     * Destroys the hidden window.
>+     */
>+    void destroyHiddenWindow();

Please document what happens if you call this method without ever having called createHiddenWindow.

Need to rev iid.

>Index: toolkit/xre/nsAppRunner.cpp

>+            appStartup->DestroyHiddenWindow();

You have this sprinkled several different places. Can we unify them all in ScopedXPCOMStartup::~ScopedXPCOMStartup (right before NS_ShutdownXPCOM) or is there a reason we need to call it earlier in shutdown?

Comment 12

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Updated per review comments

Comment 13

[Dave Townsend [:mossop]]

Will check in when the tree reopens

Comment 14

[Dave Townsend [:mossop]]

Checking in toolkit/xre/nsAppRunner.cpp;
/cvsroot/mozilla/toolkit/xre/nsAppRunner.cpp,v  <--  nsAppRunner.cpp
new revision: 1.200; previous revision: 1.199
done
Checking in toolkit/components/startup/public/nsIAppStartup.idl;
/cvsroot/mozilla/toolkit/components/startup/public/nsIAppStartup.idl,v  <--  nsIAppStartup.idl
new revision: 1.5; previous revision: 1.4
done
Checking in toolkit/components/startup/src/nsAppStartup.cpp;
/cvsroot/mozilla/toolkit/components/startup/src/nsAppStartup.cpp,v  <--  nsAppStartup.cpp
new revision: 1.22; previous revision: 1.21
done