Last Comment Bug 342352 - Bug 338897 may have caused other crash
: Bug 338897 may have caused other crash
Status: VERIFIED FIXED
: regression, verified1.8.0.5, verified1.8.1
Product: Core
Classification: Components
Component: Event Handling (show other bugs)
: 1.8 Branch
: x86 Linux
: -- blocker (vote)
: ---
Assigned To: Olli Pettay [:smaug] (high review load, please consider other reviewers)
: Hixie (not reading bugmail)
Mentors:
Depends on: 342414
Blocks:
  Show dependency treegraph
 
Reported: 2006-06-21 14:49 PDT by Olli Pettay [:smaug] (high review load, please consider other reviewers)
Modified: 2006-08-22 09:32 PDT (History)
5 users (show)
mtschrep: blocking1.8.1+
dveditz: blocking1.8.0.5+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
possible patch (1.84 KB, patch)
2006-06-21 15:54 PDT, Olli Pettay [:smaug] (high review load, please consider other reviewers)
no flags Details | Diff | Review
possible patch, even simpler (985 bytes, patch)
2006-06-21 17:20 PDT, Olli Pettay [:smaug] (high review load, please consider other reviewers)
roc: review+
roc: superreview+
dveditz: approval1.8.0.5+
mtschrep: approval1.8.1+
Details | Diff | Review

Description Olli Pettay [:smaug] (high review load, please consider other reviewers) 2006-06-21 14:49:26 PDT
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:] 2006-06-21 14:59:04 PDT
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] (high review load, please consider other reviewers) 2006-06-21 15:54:31 PDT
Created attachment 226554 [details] [diff] [review]
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] (high review load, please consider other reviewers) 2006-06-21 17:20:05 PDT
Created attachment 226566 [details] [diff] [review]
possible patch, even simpler

Now someone should just tell why "quit-application" is called twice with
the test case.
Comment 4 Jay Patel [:jay] 2006-06-21 18:22:29 PDT
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:] 2006-06-21 18:53:42 PDT
(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:] 2006-06-21 19:24:16 PDT
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] (high review load, please consider other reviewers) 2006-06-22 01:31:55 PDT
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] (high review load, please consider other reviewers) 2006-06-22 01:45:02 PDT
(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:] 2006-06-22 02:16:58 PDT
(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] (high review load, please consider other reviewers) 2006-06-22 05:47:42 PDT
toolkit is using eAttemptQuit, spider eForceQuit.
Comment 11 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2006-06-22 06:41:10 PDT
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] (high review load, please consider other reviewers) 2006-06-22 06:42:30 PDT
Comment on attachment 226566 [details] [diff] [review]
possible patch, even simpler

But still asking approval for 1.8.0
Comment 13 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2006-06-22 07:37:18 PDT
Checked in to trunk
Comment 14 Bob Clary [:bc:] 2006-06-22 09:44:53 PDT
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] (high review load, please consider other reviewers) 2006-06-22 10:57:47 PDT
(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] 2006-06-22 14:23:07 PDT
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] 2006-06-22 14:24:08 PDT
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] (high review load, please consider other reviewers) 2006-06-22 15:15:12 PDT
Checked in to branches.
bc, I hope you can now run the spider with better success.
Comment 19 Tracy Walker [:tracy] 2006-08-22 09:13:38 PDT
bc, or smaug please verify, thanks
Comment 20 Bob Clary [:bc:] 2006-08-22 09:32:33 PDT
wfm with ff2b2 1.8.0.7, 1.8, 1.9 windows
verified fixed.

Note You need to log in before you can comment on or make changes to this bug.