50990 - crash in GlobalWindowImpl::InsertTimeoutIntoList

Status: VERIFIED FIXED

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

Description

[sean echevarria]

will attach stack trace.

GlobalWindowImpl::InsertTimeoutIntoList takes an argument: nsTimeoutImpl
**aList.  aList is null and a crash occurs in this line in nsGlobalWindow.cpp:
  while ((to = *aList) != nsnull) {

GlobalWindowImpl::InsertTimeoutIntoList is called by
GlobalWindowImpl::RunTimeout and passes mTimeoutInsertionPoint as the aList
argument.

built from the tip 8/30.

Comment 1

[sean echevarria]

Attachment: win2k callstack

Comment 2

[sean echevarria]

adding crash keyword

I'm working out a way to get some Beatnik content available that demonstrates
the problem.  This content should also demonstrate the problem with bug 50705 (I
found this bug while getting a testcase together for 50705).

Comment 3

[Asa Dotzler [:asa]]

adding brendan since I see his hand in those lines recently. brendan, if this is
not yours just ignore. sean, when can you have a testcase?

Comment 4

[Brendan Eich [:brendan]]

I just unhorked all the crappy three-space "Fibonacci" indentation that Travis
inflicted on this file.  I didn't change anything substantive, as cvs diff -w
will confirm (I used GNU indent).

This must be a jst bug, by default -- although he's surely not to blame, he's
just the flak catcher.  Johnny, if you need help, holler.

/be

Comment 5

[sean echevarria]

Hopefully I'll have the testcase situation sorted out tomorrow.

Comment 6

[sean echevarria]

changing dependencies.  There's a pretty good testcase for bug 50705.  Trying 
to build one for this bug, but 50705 is making it difficult.

Comment 7

[Nisheeth Ranjan]

Marking nsbeta3+ and setting priority to P2...

Comment 8

[sean echevarria]

This is definitely timing related.  I had a testcase that demonstrated the 
crash about 70% of the time.  However, it only demonstrated the crash on 1 of 3 
servers I tried it from.  The one server it happened from is internal.  And now 
that one case doesn't crash with today's pull but the original content that I 
experienced this with still crashes.

The testcase that used to work can be found at 
http://www.mindspring.com/~sean_/mozilla/timerlist/ (one of the servers that I 
couldn't repro from).

I can set you up with the original content but it requires signing an 
evaluation agreement for an alpha version of our free xpcom plugin (don't get 
me started...).  The plugin is only required because the content won't start if 
it is not installed.

Comment 9

[sean echevarria]

The eval form is also required for the content itself.  From the form:
The "Evaluation Technology" shall mean: A modified version of an already 
publicly available currently released eMix... 

adding beatnik keyword

Comment 10

[Nisheeth Ranjan]

Marking nsbeta3- (ekrock, nisheeth).  Sean, can you try and fix this, please.  
We can't reproduce this in house.

Comment 11

[sean echevarria]

Well it's easy enough to add:

NS_ASSERTION(aList, "GlobalWindowImpl::InsertTimeoutIntoList null timeoutList");
if (aList == nsnull)
  return;

to the top of GlobalWindowImpl::InsertTimeoutIntoList() to prevent this 
particular crash.

But, the next time GlobalWindowImpl::RunTimeout() is called, there's a crash 
because 'this' has been deleted.  Looks like a timeout is firing on a non-
existent window.

nsGlobalWindow_RunTimeout() does the following:

timeout->window->RunTimeout(timeout)

where window is garbage (0xfeeefeee).

Comment 12

[sean echevarria]

Attachment: proposed fix

Comment 13

[sean echevarria]

I've attached a proposed fix.  I looked around in nsGlobalWindow.cpp and saw 
this being used:

        insertion_point = (mTimeoutInsertionPoint == NULL)
                          ? &mTimeouts : mTimeoutInsertionPoint;

So I modified the call that was passing in a null pointer so that it doesn't 
blindly pass in mTimeoutInsertionPoint.

This patch addresses the crash and the followup crash I mentioned in my last 
post.  Now I'm only seeing bug 50705 in regards to timeout/intervals.

Johnny or Brendan, care to review?

Comment 14

[Brendan Eich [:brendan]]

You know, I always wondered why null was a valid state for the insertion point. 
Typically you can avoid that special case, which seems to cause the insertion
point to be reset to &mTimeouts, simply by initializing mTimeoutInsertionPoint
to &mTimeouts and resetting it directly to that address.

Sean, care to try that state reduction?  Or am I missing something else that a
null value means?

/be

Comment 15

[sean echevarria]

Attachment: another patch

Comment 16

[sean echevarria]

added another patch that clears up the crash.  I didn't limit changes to only 
the initialization of mTimeoutInsertionPoint since it gets modified in several 
places (and was explicitly set to nsnull in one more place than the class ctor).

Comment 17

[Brendan Eich [:brendan]]

I'm confused, why did you make changes of this sort:

-  mTimeoutInsertionPoint = last_insertion_point;
+  if (last_insertion_point)
+    mTimeoutInsertionPoint = last_insertion_point;
+  else
+    mTimeoutInsertionPoint = &mTimeouts;

last_insertion_point is never set to null, it is only set (once) to
mTimeoutInsertionPoint.  So there is no need for these changes to its uses.

/be

Comment 18

[sean echevarria]

Attachment: patch 'o the hour

Comment 19

[sean echevarria]

My fault - missed that initialization of last insertion point.

Comment 20

[Brendan Eich [:brendan]]

Looks good to me -- jst, can you give an r=?  If so, a=[e-mail].

/be

Comment 21

[Johnny Stenback (:jst)]

Looks good to me. Sean, I assume you've given this a fair ammount of testing
with timeouts in JS, right? If so, then r=jst, thanks for fixing this!

Comment 22

[sean echevarria]

thanks guys - checked in.

Comment 23

[Keyser Sose]

Works for Me
Platform: PC
OS: Windows 98
Mozilla Version: 2000100508

Marking as Verified