484442 - Fx crashes on on bringing up safe-mode with any/all of the Options Checked [@ g_object_set_data - setup_widget_prototype] at shutdown

Status: VERIFIED FIXED

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

Description

[Aakash Desai [:aakashd]]

Litmus test case: 6931
Build: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3) Gecko/20090305 
Firefox/3.1b3 ID:20090305134136

This occurs only on Linux only and the function works just fine on the latest 3.0 release.


Steps to Reproduce:
1. Start Firefox in safe-mode (happens with or without "-P")
2. Select any or all the Options and Click on the Activated "Makes Changes and Restart" button.

Actual Results:

Firefox crashes with the following crash report Id:
http://crash-stats.mozilla.com/report/index/f475db62-d5fd-4d2e-9408-19c922090320?p=1

Expected Results:

Firefox should not crash.

Comment 1

[Benjamin Smedberg]

0  	libgobject-2.0.so.0.1800.2  	libgobject-2.0.so.0.1800.2@0x295d2  	
1 	libxul.so 	setup_widget_prototype 	widget/src/gtk2/gtk2drawing.c:143
2 	libxul.so 	ensure_toggle_button_widget 	widget/src/gtk2/gtk2drawing.c:184
3 	libxul.so 	ensure_button_arrow_widget 	widget/src/gtk2/gtk2drawing.c:195
4 	libxul.so 	moz_gtk_get_downarrow_size 	widget/src/gtk2/gtk2drawing.c:2901
5 	libxul.so 	nsNativeThemeGTK::GetMinimumWidgetSize 	widget/src/gtk2/nsNativeThemeGTK.cpp:1054
6 	libxul.so 	nsIFrame::AddCSSMinSize 	layout/xul/base/src/nsBox.cpp:735
7 	libxul.so 	nsImageBoxFrame::GetMinSize 	layout/xul/base/src/nsImageBoxFrame.cpp:469
8 	libxul.so 	nsImageBoxFrame::GetPrefSize 	layout/xul/base/src/nsImageBoxFrame.cpp:456
9 	libxul.so 	nsImageBoxFrame::GetBoxAscent 	layout/xul/base/src/nsImageBoxFrame.cpp:476
10 	libxul.so 	nsSprocketLayout::GetAscent 	layout/xul/base/src/nsSprocketLayout.cpp:1525
...lots of layout
37 	libxul.so 	nsRootBoxFrame::Reflow 	layout/xul/base/src/nsRootBoxFrame.cpp:236
38 	libxul.so 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:821
39 	libxul.so 	ViewportFrame::Reflow 	layout/generic/nsViewportFrame.cpp:282
40 	libxul.so 	PresShell::DoReflow 	layout/base/nsPresShell.cpp:6408
41 	libxul.so 	PresShell::ProcessReflowCommands 	layout/base/nsPresShell.cpp:6514
42 	libxul.so 	PresShell::DoFlushPendingNotifications 	layout/base/nsPresShell.cpp:4581
43 	libxul.so 	PresShell::ReflowEvent::Run 	layout/base/nsPresShell.cpp:6295
44 	libxul.so 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:510
45 	libxul.so 	NS_ProcessPendingEvents_P 	nsThreadUtils.cpp:180
46 	libxul.so 	NS_ShutdownXPCOM_P 	xpcom/build/nsXPComInit.cpp:761
47 	libxul.so 	ScopedXPCOMStartup::~ScopedXPCOMStartup 	toolkit/xre/nsAppRunner.cpp:951
48 	libxul.so 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3320
49 	firefox-bin 	main 	browser/app/nsBrowserApp.cpp:156
50 	libc-2.8.90.so 	libc-2.8.90.so@0x16684 

This is likely that we've already called moz_gtk_shutdown() from nsNativeThemeGTK::Observe("xpcom-shutdown"). bz, do you know if there's some way to not do any layout after xpcom-shutdown (or better yet, a possible xpcom-before-shutdown notification to avoid ordering issues)?

Comment 2

[timeless]

surely nsNativeThemeGTK should protect itself from being used when it won't be happy...

Comment 3

[Boris Zbarsky [:bzbarsky]]

We could easily revoke all pending reflow events and not post any more, or even bail early from DoFlushPendingNotifications based on a shutdown notification, sure.  That would mean no dialogs of any sort after that point; I assume that's ok.

Most simply, we could put in place a script blocker at shutdown.  That might be desirable anyway.  Jonas, Olli, what do you think?

Comment 4

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

I wish XPCOM had some static method to check if we're in shutdown.
nsPresShell::IsSafeToFlush could use that.

(In reply to comment #3)
> Most simply, we could put in place a script blocker at shutdown.  That might be
> desirable anyway.
Wouldn't that break some cases when scripts are also handling xpcom-shutdown?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Would a script blocker prevent those from running?

I'd be ok with presshell just observing shutdown directly too, but it seems like we also want to prevent frame construction, DOM mutations, etc on shutdown...

Comment 6

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

(In reply to comment #5)
> Would a script blocker prevent those from running?
Only if some script modifies DOM and something is listening
mutation events - or at least we'd get "killing mutation events" assertions.

> I'd be ok with presshell just observing shutdown directly too, but it seems
> like we also want to prevent frame construction, 
Sure

>DOM mutations
I wonder how.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Moving this to layout per the discussion; a fix in the native theme code would be very painful (would need to touch lots of functions).

Comment 8

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

Attachment: stack

I get very different stack. That happens because wrapper->mThread is null.

Comment 9

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

(In reply to comment #8)
On a debug build. Haven't yet tried opt.

Comment 10

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

And I don't get the crash at all on 1.9.1.

I'll try on 32bit linux tomorrow.

Comment 11

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

On 32bit linux I get yet another stack.
I guess safe mode does something strange.
Regression range would be *great*.


#0  0x00323416 in __kernel_vsyscall ()
#1  0x00f28456 in nanosleep () from /lib/libc.so.6
#2  0x00f2826e in sleep () from /lib/libc.so.6
#3  0x00e6fc50 in ah_crap_handler (signum=11)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/toolkit/xre/nsSigHandlers.cpp:149
#4  0x00e708fd in nsProfileLock::FatalSignalHandler (signo=11) at nsProfileLock.cpp:216
#5  <signal handler called>
#6  0x0534278f in nsWebShellWindow::SetPersistenceTimer (this=0xb2f131a0, aDirtyFlags=5)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/xpfe/appshell/src/nsWebShellWindow.cpp:591
#7  0x05342e87 in nsWebShellWindow::HandleEvent (aEvent=0xbfc7d0c0)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/xpfe/appshell/src/nsWebShellWindow.cpp:359
#8  0x08dcb07c in nsWindow::DispatchEvent (this=0xb4749250, aEvent=0xbfc7d0c0, aStatus=@0xbfc7d0fc)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/widget/src/gtk2/nsWindow.cpp:580
#9  0x08dc04df in nsWindow::DispatchResizeEvent (this=0xb4749250, aRect=@0xbfc7d12c, aStatus=@0xbfc7d14c)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/widget/src/gtk2/nsWindow.cpp:540
#10 0x08dc0714 in nsWindow::Resize (this=0xb4749250, aWidth=347, aHeight=107, aRepaint=1)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/widget/src/gtk2/nsWindow.cpp:971
#11 0x053399ad in nsXULWindow::SetSize (this=0xb2f131a0, aCX=347, aCY=107, aRepaint=1)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/xpfe/appshell/src/nsXULWindow.cpp:598
#12 0x05337030 in nsXULWindow::SizeShellTo (this=0xb2f131a0, aShellItem=0xb7c31494, aCX=347, aCY=107)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/xpfe/appshell/src/nsXULWindow.cpp:1742
#13 0x0532edc6 in nsChromeTreeOwner::SizeShellTo (this=0xb4639670, aShellItem=0xb7c31494, aCX=347, aCY=107)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/xpfe/appshell/src/nsChromeTreeOwner.cpp:292
#14 0x02d6f9d2 in DocumentViewerImpl::SizeToContent (this=0xb30a6f80)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/layout/base/nsDocumentViewer.cpp:3229
#15 0x0533bcd4 in nsXULWindow::OnChromeLoaded (this=0xb2f131a0)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/xpfe/appshell/src/nsXULWindow.cpp:983
#16 0x05342729 in nsWebShellWindow::OnStateChange (this=0xb2f131a0, aProgress=0xb7c31414, aRequest=0xb47da2c0, 
    aStateFlags=<value optimized out>, aStatus=2152398850)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/xpfe/appshell/src/nsWebShellWindow.cpp:661
#17 0x0143a60c in nsDocLoader::FireOnStateChange (this=0xb7c31400, aProgress=0xb7c31414, aRequest=0xb47da2c0, 
    aStateFlags=786448, aStatus=2152398850)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/uriloader/base/nsDocLoader.cpp:1254
#18 0x0143acb9 in nsDocLoader::doStopDocumentLoad (this=0xb7c31400, request=0xb47da2c0, aStatus=2152398850)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/uriloader/base/nsDocLoader.cpp:888
#19 0x0143ae27 in nsDocLoader::DocLoaderIsEmpty (this=0xb7c31400, aFlushLayout=1)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/uriloader/base/nsDocLoader.cpp:782
#20 0x0143b308 in nsDocLoader::OnStopRequest (this=0xb7c31400, aRequest=0xb2e8b2e0, aCtxt=0x0, 
    aStatus=2152398850) at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/uriloader/base/nsDocLoader.cpp:678
#21 0x0118205a in nsLoadGroup::RemoveRequest (this=0xb2f126d0, request=0xb2e8b2e0, ctxt=0x0, aStatus=2152398850)
---Type <return> to continue, or q <return> to quit---
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/netwerk/base/src/nsLoadGroup.cpp:680
#22 0x011829b5 in nsLoadGroup::Cancel (this=0xb2f126d0, status=2152398850)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/netwerk/base/src/nsLoadGroup.cpp:331
#23 0x0143b179 in nsDocLoader::Stop (this=0xb7c31400)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/uriloader/base/nsDocLoader.cpp:313
#24 0x0143b14c in nsDocLoader::Stop (this=0xb55eb040)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/uriloader/base/nsDocLoader.cpp:308
#25 0x01439a9b in nsDocLoader::Destroy (this=0xb55eb040)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/uriloader/base/nsDocLoader.cpp:413
#26 0x01439eb4 in ~nsDocLoader (this=0xb55eb040)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/uriloader/base/nsDocLoader.cpp:210
#27 0x01439d5f in nsDocLoader::Release (this=0xb55eb040)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/uriloader/base/nsDocLoader.cpp:225
#28 0x007f81c3 in nsCOMPtr_base::assign_assuming_AddRef (this=0xb7b3b744, newPtr=0x0)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/xpcom/build/../glue/nsCOMPtr.h:456
#29 0x007f7f1e in nsCOMPtr_base::assign_with_AddRef (this=0xb7b3b744, rawPtr=0x0) at nsCOMPtr.cpp:89
#30 0x0083fdd9 in nsCOMPtr<nsISupports>::operator= () at ../../dist/include/xpcom/nsCOMPtr.h:975
#31 FreeServiceContractIDEntryEnumerate (aTable=0xb7c49268, aHdr=0xb7caab80, aNumber=197, aData=0x0)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/xpcom/components/nsComponentManager.cpp:1736
#32 0x007f597b in PL_DHashTableEnumerate (table=0xb7c49268, etor=0x83fdb3 <FreeServiceContractIDEntryEnumerate>, 
    arg=0x0) at pldhash.c:754
#33 0x0083d88f in nsComponentManagerImpl::FreeServices (this=0xb7c49220)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/xpcom/components/nsComponentManager.cpp:1749
#34 0x008032bb in NS_ShutdownXPCOM_P (servMgr=0xb7c49224)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/xpcom/build/nsXPComInit.cpp:807
#35 0x00e617fc in ~ScopedXPCOMStartup (this=0xbfc7df7c)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/toolkit/xre/nsAppRunner.cpp:887
#36 0x00e65594 in XRE_main (argc=4, argv=0xbfc7e0e4, aAppData=0xb7c05540)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/toolkit/xre/nsAppRunner.cpp:3273
#37 0x08049573 in main (argc=4, argv=0xbfc7e0e4)
    at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/browser/app/nsBrowserApp.cpp:156

Comment 12

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

XPCOM might be better Component for this bug.

Comment 13

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

As far as I see, this is a regression from bug 212316.
At least I couldn't reproduce after backing that out.

Ere or bsmedberg, want to take this? I'm really not familiar with shutdow.

Comment 14

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

Benjamin: can you handle this for us?

Comment 15

[Benjamin Smedberg]

Attachment: Don't run commandline handlers if we're just going to immediately shut down, rev. 1

After the user clicked "restart" (or "cancel") in the safe-mode dialog, we were still running the normal commandline handlers, which ended up opening a browser window. This obviously sucks. Future rearchitecting of bootstrap would make this easy to fix, but in the meantime we just keep asking appstartup "are we shutting down now" before performing intermediate startup actions.

Comment 16

[Benjamin Smedberg]

I can give you a patch -w if that would help reviewing... lots of code just changed indent level. And boy, XRE_main needs to be separated into more subfunctions yet again :-(

Comment 17

[Dave Townsend [:mossop]]

Comment on attachment 372968 [details] [diff] [review]
Don't run commandline handlers if we're just going to immediately shut down, rev. 1

Bleh yeah this is getting crazy. The changes look pretty much ok as is

>+
>+          nsCOMPtr<nsIRemoteService> remoteService;
>+          if (!shuttingDown) {
> #ifdef MOZ_ENABLE_XREMOTE
>-          // if we have X remote support, start listening for requests on the
>-          // proxy window.
>-          nsCOMPtr<nsIRemoteService> remoteService;
>-          remoteService = do_GetService("@mozilla.org/toolkit/remote-service;1");
>-          if (remoteService)
>-            remoteService->Startup(gAppData->name,
>-                                   PromiseFlatCString(profileName).get());
>+            // if we have X remote support, start listening for requests on the
>+            // proxy window.
>+            remoteService = do_GetService("@mozilla.org/toolkit/remote-service;1");
>+            if (remoteService)
>+              remoteService->Startup(gAppData->name,
>+                                     PromiseFlatCString(profileName).get());
> #endif /* MOZ_ENABLE_XREMOTE */
> 
>-          // enable win32 DDE responses and Mac appleevents responses
>-          nativeApp->Enable();
>+            // enable win32 DDE responses and Mac appleevents responses
>+            nativeApp->Enable();
>+          }
> 
>           NS_TIMELINE_ENTER("appStartup->Run");
>           rv = appStartup->Run();
>           NS_TIMELINE_LEAVE("appStartup->Run");
>           if (NS_FAILED(rv)) {
>             NS_ERROR("failed to run appstartup");
>             gLogConsoleErrors = PR_TRUE;
>           }
> 
>           // Check for an application initiated restart.  This is one that
>           // corresponds to nsIAppStartup.quit(eRestart)
>           if (rv == NS_SUCCESS_RESTART_APP) {
>             needsRestart = PR_TRUE;
>             appInitiatedRestart = PR_TRUE;
>           }
> 
>+          if (!shuttingDown) {
> #ifdef MOZ_ENABLE_XREMOTE
>-          // shut down the x remote proxy window
>-          if (remoteService)
>-            remoteService->Shutdown();
>+            // shut down the x remote proxy window
>+            if (remoteService)
>+              remoteService->Shutdown();
> #endif /* MOZ_ENABLE_XREMOTE */
>+          }

Is there any reason to even call appStartup->Run() if we know we are shutting down already? If not you can just wrap this entire block in the if

Comment 18

[Benjamin Smedberg]

Hrm, I figured I still needed to run the event loop to process the quit message that we'd already posted... but perhaps I don't, since shutdown spins the event loop and processes pending events already. I'll try that.

Comment 19

[Benjamin Smedberg]

Actually, what am I thinking. I just had a baby and will be gone for two weeks. Dave, can you take this and finish it?

Comment 20

[Dave Townsend [:mossop]]

(In reply to comment #17)
> (From update of attachment 372968 [details] [diff] [review])
> > 
> >           NS_TIMELINE_ENTER("appStartup->Run");
> >           rv = appStartup->Run();
> >           NS_TIMELINE_LEAVE("appStartup->Run");
> >           if (NS_FAILED(rv)) {
> >             NS_ERROR("failed to run appstartup");
> >             gLogConsoleErrors = PR_TRUE;
> >           }
> > 
> >           // Check for an application initiated restart.  This is one that
> >           // corresponds to nsIAppStartup.quit(eRestart)
> >           if (rv == NS_SUCCESS_RESTART_APP) {
> >             needsRestart = PR_TRUE;
> >             appInitiatedRestart = PR_TRUE;
> >           }
> > 
> 
> Is there any reason to even call appStartup->Run() if we know we are shutting
> down already? If not you can just wrap this entire block in the if

To answer myself, yes we need to call appStartup->Run since that tells us that a restart has been requested. We could potentially add a getter for mRestart in nsIAppStartup2, but that seems like more hassle than it is worth for this, so I'll just land this patch as is.

Comment 21

[Dave Townsend [:mossop]]

Landed on trunk: http://hg.mozilla.org/mozilla-central/rev/22a5636b3f9d

Comment 22

[Dave Townsend [:mossop]]

Landed a bustage fix: http://hg.mozilla.org/mozilla-central/rev/4209e25766b7

Comment 23

[Dave Townsend [:mossop]]

Landed on the branch: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ccf51d9623b3

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Aaskash, is this fixed for you now?

Comment 25

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090522 Minefield/3.6a1pre ID:20090522031102

and

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1pre) Gecko/20090522 Shiretoko/3.5pre ID:20090522030833