315286 - Crash [@ nsGlobalWindow::RunTimeout ]

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Bob Clary [:bc] (inactive)]

Crash in testcase for bug 37907 in Firefox 1.5/20051105/WinXP, haven't tested trunk yet. Marking sensitive, just in case.

	delay	0x00177408
	nsITimer::TYPE_ONE_SHOT	0x00000000
	rv	0x30025d10
+	this	0x03103278
+	timeout	0x035f1d68
-	timeout->mTimer	{...}
+           mRawPtr	0x00000000


nsGlobalWindow::RunTimeout(nsTimeout * 0x035f1d68) line 6357 + 41 bytes
nsGlobalWindow::TimerCallback(nsITimer * 0x035f1e78, void * 0x035f1d68) line 6656
nsTimerImpl::Fire() line 394 + 17 bytes
nsTimerManager::FireNextIdleTimer(nsTimerManager * const 0x00f96c78) line 628
nsAppShell::Run(nsAppShell * const 0x00f64938) line 142
nsAppStartup::Run(nsAppStartup * const 0x00f64898) line 150 + 26 bytes
XRE_main(int 0x00000002, char * * 0x003f6ea0, const nsXREAppData * 0x0042201c kAppData) line 2313 + 35 bytes
main(int 0x00000002, char * * 0x003f6ea0) line 61 + 18 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 7c816d4f()

Comment 1

[Bob Clary [:bc] (inactive)]

###!!! ASSERTION: You can't dereference a NULL nsCOMPtr with operator->().: 'mRawPtr != 0', file ../../dist/include/xpcom\nsCOMPtr.h
, line 849

Comment 2

[Peter van der Woude [:Peter6]]

Incident ID: 12503895
Stack Signature	nsGlobalWindow::RunTimeout 02670766
Product ID	Firefox15
Build ID	2005120103
Trigger Time	2005-12-02 02:43:53.0
Platform	Win32
Operating System	Windows NT 5.0 build 2195
Module	firefox.exe + (00151112)
URL visited	http://112brabant.isookleuk.nl/nieuws/chat/index_main.php?width=1280&height=1024
User Comments	
Since Last Crash	25811 sec
Total Uptime	25811 sec
Trigger Reason	Access violation
Source File, Line No.	c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/dom/src/base/nsGlobalWindow.cpp, line 6357
Stack Trace 	
nsGlobalWindow::RunTimeout  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/dom/src/base/nsGlobalWindow.cpp, line 6357]
nsGlobalWindow::TimerCallback  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/dom/src/base/nsGlobalWindow.cpp, line 6656]
nsAppStartup::Run  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/toolkit/components/startup/src/nsAppStartup.cpp, line 151]
main  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/browser/app/nsBrowserApp.cpp, line 61]
KERNEL32.dll + 0x28989 (0x7c598989)

Comment 3

[Ryan Flint [:rflint] (ping via IRC for reviews)]

*** Bug 319932 has been marked as a duplicate of this bug. ***

Comment 4

[Adam Guthrie]

*** Bug 320982 has been marked as a duplicate of this bug. ***

Comment 5

[Ere Maijala (slow)]

Attachment: Patch v1

Null check patch. This is kind of out of my domain but now that I stumbled on this I found out that timeout->mTimer can be null even though timeout->mInterval != 0.

Comment 6

[Johnny Stenback (:jst)]

(In reply to comment #5)
> Created an attachment (id=206617) [edit]
> Patch v1
> 
> Null check patch. This is kind of out of my domain but now that I stumbled on
> this I found out that timeout->mTimer can be null even though
> timeout->mInterval != 0.
> 

Do you know why this can happen? I haven't digged through the code to find out why yet, just wondering if you already did...

Comment 7

[Ere Maijala (slow)]

SuspendTimeouts perhaps. At least it's called right there when it would crash and it will delete mTimer without touching mInterval.

Comment 8

[Bob Clary [:bc] (inactive)]

also crashes on Mac OS X, at least with FF 1.5.0.1 RC1

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 206617 [details] [diff] [review]
Patch v1

This is close, but it won't reset the timer to start again (I'll explain when I attach my patch, which isn't quite complete yet)

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

What's happening here is that RunTimeout doesn't deal with the case where we call SuspendTimeouts while dealing with a timeout that's currently running. The patch is going to be somewhat messy because of bad factorizing in the timeout code.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

Note that this is a diff -w. I'll attach a full diff once I have reviews ;-). I think this leaves the timeout list in a sane state if we end up being frozen from executing one of the timers.

Note that setting up the timers correctly in this case is extra work, but this bug would also occur if the timeout had just set location.href (and really put the page into the bfcache).

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 210270 [details] [diff] [review]
Proposed fix

Looks good. r=jst

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 14

[Jesse Ruderman]

There was a topcrash in Firefox 1.5.0.1 (but not on trunk) with the same signature.  Would this patch fix it if applied to the branch?

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #14)
> There was a topcrash in Firefox 1.5.0.1 (but not on trunk) with the same
> signature.  Would this patch fix it if applied to the branch?

It's very possible that it would.

Comment 16

[Daniel Veditz [:dveditz]]

Tenatively making blocking, pending trunk testing to make sure there aren't regressions.

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 210270 [details] [diff] [review]
Proposed fix

Approving, dveditz let me know if you think I've done wrong here -- mrbkap was trying to avoid entangling with jst via CVS, and this patch seems baked, safe, and wanted.

/be

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.8 branches.

Comment 19

[Dave Liebreich [:davel]]

Marking [rft-dl] (ready for testing in Firefox 1.5.0.2 release candidates)

Comment 20

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060302 Firefox/1.5.0.1, no crash with testcase from bug 37907 (https://bugzilla.mozilla.org/attachment.cgi?id=8207).

Comment 21

[1106-29]

i tested rv:1.8.0.2 Gecko/20060308 Firefox/1.5.0.2
browser crash. See Bug 320982 and use URL for testing.