404531 - Clear private data shutdown confirmation hangs

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Dave Townsend [:mossop]]

STR:

1. Enable clear private data on shutdown
2. Enable asking before clearing private data.
3. Shutdown Firefox.

From there you get the clear private data confirmation dialog, but its beachballing and you have to kill firefox to get rid of it.

Comment 1

[Dave Townsend [:mossop]]

This is a regression from Firefox 2, I'd guess from the cocoa widget impl

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

Why do you suspect this is cocoa widgets? If we have good reason, we should throw it over to that component.

Comment 3

[Stephen Donner [:stephend] Not actively reading bugmail]

Attachment: Sample during the hang

Comment 4

[Justin Dolske [:Dolske]]

Attachment: Sample during the hang

The other sample looks to be from a optimized build, and the stack is bogus. This is a sample with a debug build.

Looks like it's going crazy trying to process some event or notify some observer?

Comment 5

[(not reading, please use [e-mail] instead)]

here's a stack from a debug build, maybe it will help.

#0  0x34f40093 in nsTArray<JSContextAndFrame>::Elements (this=0x2944c74) at ../../../../dist/include/xpcom/nsTArray.h:302
#1  0x34f40217 in nsTArray<JSContextAndFrame>::AppendElements<JSContext*> (this=0x2944c74, array=0xbfffd7b4, arrayLen=1) at ../../../../dist/include/xpcom/nsTArray.h:524
#2  0x34f40266 in nsTArray<JSContextAndFrame>::AppendElement<JSContext*> (this=0x2944c74, item=@0xbfffd7b4) at ../../../../dist/include/xpcom/nsTArray.h:538
#3  0x34ef8b97 in XPCJSContextStack::Push (this=0x2944c70, cx=0x0) at /Users/sspitzer/Desktop/trunk/mozilla/js/src/xpconnect/src/xpcthreadcontext.cpp:124
#4  0x34ef9498 in nsXPCThreadJSContextStackImpl::Push (this=0x2945d20, cx=0x0) at /Users/sspitzer/Desktop/trunk/mozilla/js/src/xpconnect/src/xpcthreadcontext.cpp:392
#5  0x34ecc9e9 in nsXPConnect::OnProcessNextEvent (this=0x2945f10, aThread=0x2922a00, aMayWait=1, aRecursionDepth=0) at /Users/sspitzer/Desktop/trunk/mozilla/js/src/xpconnect/src/nsXPConnect.cpp:2222
#6  0x0136cd48 in nsThread::ProcessNextEvent (this=0x2922a00, mayWait=1, result=0xbfffd8ec) at /Users/sspitzer/Desktop/trunk/mozilla/xpcom/threads/nsThread.cpp:493
#7  0x013123e5 in NS_ProcessNextEvent_P (thread=0x2922a00, mayWait=1) at nsThreadUtils.cpp:227
#8  0x304e420b in nsXULWindow::ShowModal (this=0x3663ac00) at /Users/sspitzer/Desktop/trunk/mozilla/xpfe/appshell/src/nsXULWindow.cpp:398
#9  0x304dd95b in nsContentTreeOwner::ShowAsModal (this=0x3ded6ce0) at /Users/sspitzer/Desktop/trunk/mozilla/xpfe/appshell/src/nsContentTreeOwner.cpp:524
#10 0x14d3ef46 in nsWindowWatcher::OpenWindowJSInternal (this=0x36618d60, aParent=0x0, aUrl=0x3c58bd70 "chrome://browser/content/sanitize.xul", aName=0x3c57e3f0 "Sanitize", aFeatures=0x3c58afd0 "chrome,titlebar,centerscreen,modal", aDialog=0, argv=0x0, aCalledFromJS=0, _retval=0xbfffe134) at /Users/sspitzer/Desktop/trunk/mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp:945
#11 0x14d3f615 in nsWindowWatcher::OpenWindow (this=0x36618d60, aParent=0x0, aUrl=0x3c58bd70 "chrome://browser/content/sanitize.xul", aName=0x3c57e3f0 "Sanitize", aFeatures=0x3c58afd0 "chrome,titlebar,centerscreen,modal", aArguments=0x0, _retval=0xbfffe134) at /Users/sspitzer/Desktop/trunk/mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp:419
#12 0x0137ec5c in NS_InvokeByIndex_P (that=0x36618d60, methodIndex=3, paramCount=6, params=0xbfffe0e4) at /Users/sspitzer/Desktop/trunk/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp:179
#13 0x34f086ee in XPCWrappedNative::CallMethod (ccx=@0xbfffe334, mode=CALL_METHOD) at /Users/sspitzer/Desktop/trunk/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:2342
#14 0x34f10176 in XPC_WN_CallMethod (cx=0x294c630, obj=0x28ede60, argc=5, argv=0x25d711c, vp=0xbfffe474) at /Users/sspitzer/Desktop/trunk/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1480
#15 0x010632ca in js_Invoke (cx=0x294c630, argc=5, vp=0x25d7114, flags=0) at /Users/sspitzer/Desktop/trunk/mozilla/js/src/jsinterp.c:1023
#16 0x01075318 in js_Interpret (cx=0x294c630, pc=0x366fbb65 ":", result=0xbfffeafc) at /Users/sspitzer/Desktop/trunk/mozilla/js/src/jsinterp.c:3863
#17 0x01063355 in js_Invoke (cx=0x294c630, argc=3, vp=0x25d6e18, flags=2) at /Users/sspitzer/Desktop/trunk/mozilla/js/src/jsinterp.c:1040
#18 0x34f03181 in nsXPCWrappedJSClass::CallMethod (this=0x366198f0, wrapper=0x3661ffd0, methodIndex=3, info=0x2032270, nativeParams=0xbfffef54) at /Users/sspitzer/Desktop/trunk/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp:1443
#19 0x34efb889 in nsXPCWrappedJS::CallMethod (this=0x3661ffd0, methodIndex=3, info=0x2032270, params=0xbfffef54) at /Users/sspitzer/Desktop/trunk/mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp:567
#20 0x0137ef16 in PrepareAndDispatch (self=0x36619b10, methodIndex=3, args=0xbffff074) at /Users/sspitzer/Desktop/trunk/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_unixish_x86.cpp:93
#21 0x0137ef74 in nsXPTCStubBase::Stub3 (this=0x36619b10) at ../../../../../../dist/include/xpcom/xptcstubsdef.inc:5
#22 0x013240bb in nsObserverList::NotifyObservers (this=0x24208cc, aSubject=0x3bfd9af0, aTopic=0x246334 "profile-change-teardown", someData=0x25a720) at /Users/sspitzer/Desktop/trunk/mozilla/xpcom/ds/nsObserverList.cpp:128
#23 0x01324765 in nsObserverService::NotifyObservers (this=0x29266d0, aSubject=0x3bfd9af0, aTopic=0x246334 "profile-change-teardown", someData=0x25a720) at /Users/sspitzer/Desktop/trunk/mozilla/xpcom/ds/nsObserverService.cpp:181
#24 0x0021852d in nsXREDirProvider::DoShutdown (this=0xbffff3d4) at /Users/sspitzer/Desktop/trunk/mozilla/toolkit/xre/nsXREDirProvider.cpp:833
#25 0x00207f7c in ScopedXPCOMStartup::~ScopedXPCOMStartup (this=0xbffff47c) at /Users/sspitzer/Desktop/trunk/mozilla/toolkit/xre/nsAppRunner.cpp:857
#26 0x0021017f in XRE_main (argc=1, argv=0xbffff7a0, aAppData=0x290b160) at /Users/sspitzer/Desktop/trunk/mozilla/toolkit/xre/nsAppRunner.cpp:3213
#27 0x00002798 in main (argc=1, argv=0xbffff7a0) at /Users/sspitzer/Desktop/trunk/mozilla/browser/app/nsBrowserApp.cpp:153

Comment 6

[(not reading, please use [e-mail] instead)]

note, the clear private data dialog has a cancel button.

if I'm prompting on clear private data, and I'm quitting, should cancel stop the quit?

if so, should we be moving the call to this.Sanitizer.onShutdown(); out of _onProfileShutdown() in nsBrowserGlue.js and into the handler for the "quit-application-requested" topic?

Comment 7

[Sylvain Pasche]

Looks similar to bug 371398. It's not hanging on Linux, but consuming 100%. Might be worth testing the patch there on Mac.

Comment 8

[Justin Dolske [:Dolske]]

The "hang" is also 100% CPU on OS X. It's a hang in the sense that despite the event queue frantically spinning, it's not actually responsive to any input.

Comment 9

[Sylvain Pasche]

ok. I tried that patch (I had it applied on a debug build), but that doesn't fix the hang.

Comment 10

[Brendan Eich [:brendan]]

I had this bite big-time after accepting the beta3 update offer. It's bad. Let to a full update, still burned me, finally the full update "took" on next startup.

This bug needs an owner.

/be

Comment 11

[Mike Schroepfer]

--> Josh

Comment 12

[Steven Michaud [:smichaud] (Retired)]

A quick look at this leads me to believe that the Clear Private Data
dialog/window is running in a Gecko modal loop after the appshell has
been destroyed -- and that (as a result) native events are being
starved.

So I suspect that (on the Mac) the solution is some kind of hack to
nsAppShell.mm.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

> A quick look at this leads me to believe that the Clear Private Data
> dialog/window is running in a Gecko modal loop after the appshell
> has been destroyed -- and that (as a result) native events are being
> starved.

I've confirmed this ... mostly.  But I was wrong about the appshell
having already been destroyed by the time the Clear Private Data
dialog appears -- it's destructor hasn't yet been called, but it _has_
already been shut down (nsAppShell::Exit() has already been called).

I frankly think this is a bug in XRE -- one shouldn't still be
displaying dialogs after nsAppShell::Exit() has been called.  But time
is short before the Firefox 3 release.  And I understand that the XRE
shutdown code is very fragile and difficult to change (see bug 384284
comment #18).  So maybe it's best to hack out a temporary work-around
(or possibly several temporary OS-specific workarounds).

I'll post a proof-of-concept patch for OS X in my next comment.

Here I'll give a little more detail about what happens to trigger this
bug:

1) An nsAppExitEvent is dispatched from nsAppStartup::Quit().  When
   this event runs, it calls nsAppShell::Exit().  (Both the
   nsAppExitEvent class and nsAppStartup::Quit() are defined in
   toolkit/components/startup/src/nsAppStartup.cpp.)

2) A call to nsXREDirProvider::DoShutdown() (in
   toolkit/xre/nsXREDirProvider.cpp) is made from the destructor for
   the ScopedXPCOMStartup class (in nsAppStartup.cpp).  This call
   notifies a bunch of "observers".

3) One of these "observers" displays the Clear Private Data dialog.

4) Another "observer" (nsBaseAppShell::Observe() in
   widget/src/xpwidgets/nsBaseAppShell.cpp) calls nsAppShell::Exit() a
   second time.

Clearly it'd be nice to get rid of the first call to
nsAppShell::Exit() (the one made when the nsAppExitEvent runs).  But I
don't know how difficult this would be.

Comment 14

[Dave Townsend [:mossop]]

That seems an odd place to be displaying the dialog. Can we not use the quit-application notification sent direct from nsAppStartup to display the dialog. This is before the call to nsAppShell::Exit and would be cleaner I think.

I'm not positive if that'd solve the problem but it feels nicer.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

I think dtownsend has the right idea.

One thing I forgot to mention:  Even with my proof-of-concept fix
(which I'll now post in my _next_ comment), the Clear Private Data
dialog still eats about 100% of the cpu.

I suspect this is because it's polling in a very tight loop.  Ordinary
modal dialogs don't eat cpu like this, so I suspect this happens
because nsAppShell::Exit() has already been called.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

Attachment: Proof-of-concept fix for OS X

Here's my proof-of-concept "fix".  I haven't done much testing, and
I've already mentioned the 100% cpu problem.  So this "fix" is
definitely not in its final form.  Like dtownsend I hope this problem
can be fixed "upstream", so that my patch won't be necessary.

I've added an NSLog() statement to record when nsAppShell::Exit() is
called.

Comment 17

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build made using my "proof-of-concept" fix
(attachment 303395 [details] [diff] [review]):

https://build.mozilla.org/tryserver-builds/2008-02-14_15:[e-mail]-bugzilla404531-rev0/[e-mail]

Comment 18

[Steven Michaud [:smichaud] (Retired)]

Bsmedberg, do you think dtownsend's suggestion in comment #14 is
feasible?

Specifically, can we make the Clear Private Data dialog get displayed
as a result of the call (in nsAppStartup::Quit()) to NotifyObservers()
with aTopic set to "quit-application"?

And is so, how would you suggest doing this?

Comment 19

[Dave Townsend [:mossop]]

Attachment: Use quit-application

This switches to triggering the clear private data on quit-application which gives us a nice responsive window.

The calls to enter/exitLastWindowClosingSurvivalArea are no longer necessary. This was to prevent the close of the cpd window from calling nsAppStartup::Quit a second time and causing a second quit-application to be sent (bug 307840). Now that this call is being made directly from nsAppStartup::Quit we don't have to worry since that has it's own reentrency check.

I've also move the places shutdown call to here and there is no need for the profile started check, quit-application can only be sent after an app has started up with a profile.

I have yet to test on Linux and Windows but this feels like the right thing to do here.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

I wonder whether Dietrich had considered using quit-application while writing the patch for bug 376008, or whether Giorgio considered it for _onProfileShutdown when he wrote nsBrowserGlue, or whether they know anything that you and I don't about whether it is/isn't suitable :)

(I don't see any problems off-hand, but I'm not very familiar with these topics and their intended uses.)

Comment 22

[Dietrich Ayala (:dietrich)]

(In reply to comment #20)
> I wonder whether Dietrich had considered using quit-application while writing
> the patch for bug 376008, or whether Giorgio considered it for
> _onProfileShutdown when he wrote nsBrowserGlue, or whether they know anything
> that you and I don't about whether it is/isn't suitable :)

i do not remember why i used profile-before-change. i've moved to use quit-application-granted in bug 384370.

Comment 23

[Dave Townsend [:mossop]]

Just checked this on linux which is where most of the previous fixes to this kind of hang were and it seems fine there too.

Comment 24

[Giorgio Maone [:ma1]]

(In reply to comment #22)
> (In reply to comment #20)
> i do not remember why i used profile-before-change. i've moved to use
> quit-application-granted in bug 384370.
> 

I do not remember either, even if I guess it was a matter of consistency (performing cleanup as profile is going down).
Right now I couldn't back any objection, especially since we don't support warm profile change anymore.

Comment 25

[Dave Townsend [:mossop]]

Checking in browser/components/nsBrowserGlue.js;
/cvsroot/mozilla/browser/components/nsBrowserGlue.js,v  <--  nsBrowserGlue.js
new revision: 1.55; previous revision: 1.54
done

Comment 27

[Marcia Knous [:marcia]]

verified fixed using  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b4pre) Gecko/2008022704 Minefield/3.0b4pre. I verified using the STR in Comment 0.

Comment 28

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=7784 added to the Litmus FFT, with some modifications to cover the way the functionality exists today.