342352 - Bug 338897 may have caused other crash

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

According to bc, there has been a lot more crashes lately in 1.8.0 and 1.8 (Linux only).
I can't reproduce the crash (even with bc's exact instructions, which
required one to install an extension and run some specific chrome:// url...),
but without Bug 338897 I don't see a gtk assertion which is there otherwise

(Gecko:5685): Gtk-CRITICAL **: gtk_widget_destroy: assertion `GTK_IS_WIDGET (widget)' failed

Stack from bc:

Stack:
.L30
(/work/mozilla/builds/ff/1.5.0/mozilla/obj-dbg/toolkit/profile/src/nsProfileLock.cpp:210)
UNKNOWN  (/lib/tls/libpthread.so.0)
gtk_widget_destroy+0x00000055  (/usr/lib/libgtk-x11-2.0.so.0)
nsDragService::Observe(nsISupports*, char const*, unsigned short const*)
(/work/mozilla/builds/ff/1.5.0/mozilla/widget/src/gtk2/nsDragService.cpp:136)
nsObserverService::NotifyObservers(nsISupports*, char const*, unsigned
short const*)
(/work/mozilla/builds/ff/1.5.0/mozilla/xpcom/ds/nsObserverService.cpp:233)
nsAppStartup::Quit(unsigned int)
(/work/mozilla/builds/ff/1.5.0/mozilla/toolkit/components/startup/src/nsAppStartup.cpp:284)
nsAppStartup::Observe(nsISupports*, char const*, unsigned short const*)
(/work/mozilla/builds/ff/1.5.0/mozilla/toolkit/components/startup/src/nsAppStartup.cpp:522)
nsObserverService::NotifyObservers(nsISupports*, char const*, unsigned
short const*)
(/work/mozilla/builds/ff/1.5.0/mozilla/xpcom/ds/nsObserverService.cpp:233)
nsXULWindow::Destroy()
(/work/mozilla/builds/ff/1.5.0/mozilla/xpfe/appshell/src/nsXULWindow.cpp:551)
nsWebShellWindow::Destroy()
(/work/mozilla/builds/ff/1.5.0/mozilla/xpfe/appshell/src/nsWebShellWindow.cpp:850)
nsChromeTreeOwner::Destroy()
(/work/mozilla/builds/ff/1.5.0/mozilla/xpfe/appshell/src/nsChromeTreeOwner.cpp:353)
nsGlobalWindow::ReallyCloseWindow()
(/work/mozilla/builds/ff/1.5.0/mozilla/dom/src/base/nsGlobalWindow.cpp:4689)
nsCloseEvent::HandleEvent()
(/work/mozilla/builds/ff/1.5.0/mozilla/dom/src/base/nsGlobalWindow.cpp:4454)
HandleCloseEvent
(/work/mozilla/builds/ff/1.5.0/mozilla/dom/src/base/nsGlobalWindow.cpp:4464)
PL_HandleEvent
(/work/mozilla/builds/ff/1.5.0/mozilla/xpcom/threads/plevent.c:688)
PL_ProcessPendingEvents
(/work/mozilla/builds/ff/1.5.0/mozilla/xpcom/threads/plevent.c:623)
nsEventQueueImpl::ProcessPendingEvents()
(/work/mozilla/builds/ff/1.5.0/mozilla/xpcom/threads/nsEventQueue.cpp:417)
event_processor_callback
(/work/mozilla/builds/ff/1.5.0/mozilla/widget/src/gtk2/nsAppShell.cpp:67)
UNKNOWN  (/usr/lib/libglib-2.0.so.0)
g_main_context_dispatch+0x000001CB  (/usr/lib/libglib-2.0.so.0)
UNKNOWN  (/usr/lib/libglib-2.0.so.0)
g_main_loop_run+0x000001A9  (/usr/lib/libglib-2.0.so.0)
gtk_main+0x000000B2  (/usr/lib/libgtk-x11-2.0.so.0)
nsAppShell::Run()
(/work/mozilla/builds/ff/1.5.0/mozilla/widget/src/gtk2/nsAppShell.cpp:141)
nsAppStartup::Run()
(/work/mozilla/builds/ff/1.5.0/mozilla/toolkit/components/startup/src/nsAppStartup.cpp:150)
XRE_main
(/work/mozilla/builds/ff/1.5.0/mozilla/toolkit/xre/nsAppRunner.cpp:2374)
main (/work/mozilla/builds/ff/1.5.0/mozilla/browser/app/nsBrowserApp.cpp:61)
__libc_start_main+0x000000D3  (/lib/tls/libc.so.6)

Comment 1

[Bob Clary [:bc] (inactive)]

steps to reproduce:

1. Install Spider 
<http://bclary.com/projects/spider/spider/spider.xpi>

2.
<./firefox -chrome "chrome://spider/content/spider.xul?url%3Dhttp%253A%252F%252Ftest.bclary.com%252Ftests%252Fmozilla.org%252Fjs%252Fjs-test-driver-standards.html%253Ftest%253Decma%252FArray%252F15.4-2.js%253Blanguage%253Dtype%253Btext%252Fjavascript%26depth%3D0%26timeout%3D120%26waittime%3D5%26hooksignal%3Don%26autostart%3Don%26autoquit%3Don%26javascripterrors%3Don%26javascriptwarnings%3Don%26csserrors%3Don%26scripturl%3Dhttp%3A%2F%2Ftest.bclary.com%2Ftests%2Fmozilla.org%2Fjs%2Fuserhookeach-js.js">

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible patch

When I use --g-fatal-warnings I can see the crash.
This patch helps for that, though I'm not at all familiar with gtk code.
(But right now way too tired to decide what I'd like to do with this crash -
 is the patch safe enough for branch or should Bug 338897 be backed out...)

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible patch, even simpler

Now someone should just tell why "quit-application" is called twice with
the test case.

Comment 4

[Jay Patel [:jay]]

bc:  Can you answer smaug's question about why we call quit twice?  Anyway to test this fix with your test automation to see if the crashes go away?  If not, we will need to approve this patch for the 1.8.0 branch and get it checked in so you can retest.

Comment 5

[Bob Clary [:bc] (inactive)]

(In reply to comment #4)
> bc:  Can you answer smaug's question about why we call quit twice?  

No. It isn't obvious to me why quit-application is sent to the observers twice unless the xpfe code is notifying as well as the toolkit code.

> Anyway to test this fix with your test automation to see if the crashes go away? 

I can try the particular test mentioned in this bug. I don't know if that would be a sufficient test but I guess it would be better than nothing. 

Comment 6

[Bob Clary [:bc] (inactive)]

I did not crash in linux after applying the second patch, doing make in obj-dbg/widgets and running the example test case.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

This gets called after appService.quit has been called already once.
chrome/content/spider/spider.js:919:    appService.quit(forceQuit);

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #7)
> This gets called after appService.quit has been called already once.
> chrome/content/spider/spider.js:919:    appService.quit(forceQuit);
> 
er, or otherway round.

Comment 9

[Bob Clary [:bc] (inactive)]

(In reply to comment #8)

This was modeled after <http://lxr.mozilla.org/mozilla1.8.0/source/toolkit/content/globalOverlay.js#47>. Maybe the cause is a name collision with Spider's version of goQuitApplication and the toolkit version. 

Can you tell who is calling whom?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

toolkit is using eAttemptQuit, spider eForceQuit.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

And there is this comment:
http://lxr.mozilla.org/seamonkey/source/toolkit/components/startup/src/nsAppStartup.cpp#194

I tried spider with eAttemptQuit and that way it seemed to work ok even without the crash fix patch.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 226566 [details] [diff] [review]
possible patch, even simpler

But still asking approval for 1.8.0

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Checked in to trunk

Comment 14

[Bob Clary [:bc] (inactive)]

I've seen that comment and it is BS. I have to use eForceQuit or Firefox will not terminate on Mac OS X. So, toolkit is calling its goQuitApplication when Spider completes iterating over and closing all windows?

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #14)
> So, toolkit is calling its goQuitApplication when
> Spider completes iterating over and closing all windows?
> 
First Spider calls appService.quit(eForceQuit) ( which doesn't close any windows). Then the last window is closed which causes "xul-window-destroyed" message, which is handled in nsAppStartup::Observe by calling Quit(eConsiderQuit).

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 226566 [details] [diff] [review]
possible patch, even simpler

approved for 1.8.0 branch, a=dveditz for drivers

Comment 17

[Daniel Veditz [:dveditz]]

Need a non-nobody owner to get this in, and it's Olli's patch: You're the lucky guy! :-)

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Checked in to branches.
bc, I hope you can now run the spider with better success.

Comment 19

[Tracy Walker [:tracy]]

bc, or smaug please verify, thanks

Comment 20

[Bob Clary [:bc] (inactive)]

wfm with ff2b2 1.8.0.7, 1.8, 1.9 windows
verified fixed.