305452 - Clicking on link crashed web browser [@ nsGlobalWindow::RunTimeout]

Status: RESOLVED FIXED

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

Description

[Justin Cherniak]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050820 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050820 Firefox/1.6a1

When visiting www.snapstream.com if you click on the products link the browser
crashes every time.

Reproducible: Always

Steps to Reproduce:
1. Visit http://snapstream.com
2. Click on the Products link
3.

Actual Results:  
Browser crashed

Comment 1

[Jesse Ruderman]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4) Gecko/20050821
Firefox/1.0+

Crashes on Mac/branch too when I follow the steps in comment 0 and then scroll
using spacebar.  My stack (TB8588585K) sucks because of bug 304842.

Comment 2

[Ria Klaassen (not reading all bugmail)]

Branch: TB8596158Z @nsGlobalWindow::RunTimeout
Trunk:  TB8596209E @nsGlobalWindow::RunTimeout 

Comment 3

[Ria Klaassen (not reading all bugmail)]

This build WFM: 1.8b4_2005081608
And this one fails: 1.8b4_2005081615
 
In trunk it failed a little bit earlier: 1.9a1_2005081607

Comment 4

[Martijn Wargers (dead)]

Stack:
nsGlobalWindow::RunTimeout 
[c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/dom/src/base/nsGlobalWindow.cpp,
line 6043]
nsGlobalWindow::ClearAllTimeouts 
[c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/dom/src/base/nsGlobalWindow.cpp,
line 6363]
nsAppStartup::DestroyExitEvent 
[c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/toolkit/components/startup/src/nsAppStartup.cpp,
line 387]
main 
[c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/browser/app/nsBrowserApp.cpp,
line 61]
kernel32.dll + 0x16d4f (0x7c816d4f)

Comment 5

[Martijn Wargers (dead)]

I can confirm this with current trunk build.
Bonsai link based on comment 3:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-08-16+07%3A00%3A00&maxdate=2005-08-16+16%3A00%3A00&cvsroot=%2Fcvsroot

Comment 6

[Jay Patel [:jay]]

This regressed on 8/17 according to the Firefox15 branch Talkback topcrash table:
http://talkback-public.mozilla.org/reports/firefox/FF15x/FF15x-topcrashers.html

Here is a query to those crashes:
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=1&searchby=stacksig&match=contains&searchfor=nsGlobalWindow::RunTimeout&vendor=MozillaOrg&product=Firefox15&platform=All&buildid=&sdate=&stime=&edate=&etime=&sortby=bbid

It looks like a recent checkin by mrbkap or jst might have caused this somewhere
around the crash point:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/dom/src/base/nsGlobalWindow.cpp&mark=848&rev=#848

It is currently the #1 topcrash on the branch (since the other 2-3 higher ranked
crashes have been fixed).  We should get this into beta.

Comment 7

[Jay Patel [:jay]]

Assigning to jst and cc'ing mrbkap

Comment 8

[Martijn Wargers (dead)]

Attachment: testcase (not minimised)

After opening this testcase (based on the site) and then visiting another site,
I crash with the latest nightly trunk build.
If a minimised testcase is really needed, let me know, and I'll try to minimise
further.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: easy fix

This is the easy "null-check" version of this fix. The current question is
whether we should do this or stop event handlers from running altogether (see
below).

The issue here is that we null out each of our timeouts' mTimer member in
nsGlobalWindow::SuspendTimeouts. The page then has a mousemove event listener
that creates a timeout. When you click on the link, we store the page in the
bfcache (calling SuspendTimeouts). Then, before we show the new document, if
the mouse moves, the event handler is called (even though the document is in
the bfcache!) and creates the timeout, which (surprise, surprise) runs. We then
end up in nsGlobalWindow::RunTimeout, which iterates through the timeout list,
assuming non-null mTimer members and crash.

Comment 10

[Scott Gifford]

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

Comment 11

[Scott Gifford]

I see the same bug on Linux build 20050821 when visiting:

    http://www.rft.com/appcommercial/pinpoint.htm

TalkBack ID TB8714783G

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: don't dispatch events if we're in the bfcache

This also fixes the bug, and it doesn't put the page script in a potentially
weird state.

Comment 13

[Brian Ryner (not reading)]

Comment on attachment 193882 [details] [diff] [review]
don't dispatch events if we're in the bfcache

This seems to be missing the implementation of nsGlobalWindow::IsFrozen...

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: updated to comments

IsFrozen was already implemented in nsGlobalWindow.h as an inline function. jst
suggested that I pull mIsFrozen (and thus the IsFrozen implementation) into
nsPIDOMWindow, so that's what I've done.

Comment 15

[Brian Ryner (not reading)]

Comment on attachment 194243 [details] [diff] [review]
updated to comments

Oh, oops.  In that case, r=me if you put mIsFrozen with the other
PRPackedBools.	Otherwise they don't actually pack.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Again, I think this patch is wrong.  We should NOT be blocking all events in the
document.  If we can detect events happening (which we can) then we should
remove the document from bfcache when events happen.  Otherwise we're
introducing a pretty significant incompatibility in the DOM behavior depending
on whether the page the user left was or was not stored in bfcache.  I don't
believe we want to be introducing such an incompatibility.

Comment 17

[Brian Ryner (not reading)]

(In reply to comment #16)
> Again, I think this patch is wrong.  We should NOT be blocking all events in 
the
> document.  If we can detect events happening (which we can) then we should
> remove the document from bfcache when events happen.

I think that would have the side-effect of removing documents from the cache 
just due to unprocessed native events when the swap the content viewers.  
That's the sort of thing that was causing us to his 
PresShell::RetargetEventToParent problems.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> I think that would have the side-effect of removing documents from the cache
> just due to unprocessed native events when the swap the content viewers.

You mean we dispatch mouse events to a DOM after we've removed that content
viewer from the window?  That sounds like a bug in itself.

As for thise bug, jst and I talked about it on #content a few days ago and were
basically agreed that the right fix here would be to add the new timeout but not
to fire it if the window is frozen (and then start up the timer if the window
gets unfrozen).

Comment 19

[Brian Ryner (not reading)]

(In reply to comment #18)
> You mean we dispatch mouse events to a DOM after we've removed that content
> viewer from the window?  That sounds like a bug in itself.

We get the events coming into PresShell::HandleEvent, at least.  I'd have to 
check what happens to them after that.

Comment 20

[Boris Zbarsky [:bzbarsky]]

I think it's reasonable to block events on the presentation level that are
coming in through the presentation if we're frozen.  Events being dispatched via
DOM should not be blocked, however.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 194243 [details] [diff] [review]
updated to comments

bz and I talked this over on IRC and he's convinced me to go the "hard" route
and respect the setTimeout call (i.e., stick it in the queue of timeouts, but
not run it until the page comes out of the bfcache).

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Save new timers on the list, but don't run them

This patch adds the new interval to the queue so that when we're pulled out of
the bfcache, we'll run them as well. I think the math here should work out
(both the timeout refcounting and the interval juggling).

I'll attach a diff -w for review in a minute.

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: diff -w

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: fix refcounting problem

This fixes a small refcounting botch (when one of the calls in
nsGlobalWindow::SetTimeoutOrInterval fails, calling timeout->Release(),
timeout->mRefCnt should be at least 1) that jst noticed. I'm carrying forward
bryner's r.

Comment 25

[Johnny Stenback (:jst)]

Comment on attachment 194387 [details] [diff] [review]
fix refcounting problem

+  if (IsFrozen()) {
+    // If we're frozen, then we didn't really put this timeout into a timer's
+    // closure. Therefore our AddRef above was bogus. We release it now so
that
+    // our refcount is correct, but timeout doesn't get deleted.
+    timeout->Release(scx);
+  }

I think this now does the right thing, but it's more fragile than it could be
(as manual refcounting commonly is). I think I'd rather see us release here
unconditionally and add an additional AddRef() call in the case where we *do*
create the timer, right after the creation of the timer happened and we know it
succeeded.

- In nsGlobalWindow::ResumeTimeouts():

   for (nsTimeout *t = mTimeouts; t; t = t->mNext) {
     PRInt32 interval = (PRInt32)PR_MAX(t->mWhen, DOM_MIN_TIMEOUT_VALUE);
     t->mWhen = now + nsInt64(t->mWhen);

+    // Add a reference for the new timer's closure.
+    t->AddRef();
+
     t->mTimer = do_CreateInstance("@mozilla.org/timer;1");
     NS_ENSURE_TRUE(t->mTimer, NS_ERROR_OUT_OF_MEMORY);

This will leak t if we fail to create the timer, only AddRef() when you know
you'll be releasing later on. The 

     rv = t->mTimer->InitWithFuncCallback(TimerCallback, t, interval,
					  nsITimer::TYPE_ONE_SHOT);
     NS_ENSURE_SUCCESS(rv, rv);

Seems like all other code that initializes a timer cancels and nulls out the
timer on failure to initialize it, and releases the timeout as well (or you
could simply move the above AddRef() call to after all this succeeded.

This code really needs nsRefPtr<>'s, wanna file a followup bug on that?

sr=jst with that fixed.

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk. I will check it into the branch tomorrow.

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

I filed bug 306598 on using nsRefPtr<>.

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 194387 [details] [diff] [review]
fix refcounting problem

I think we want this fix on the branch.

Comment 29

[Jay Patel [:jay]]

This is the #1 topcrash on the branch, so we need the a= to get this checked in
there.

Comment 30

[timeless]

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

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

I just checked this into MOZILLA_1_8_BRANCH.

Comment 32

[Tracy Walker [:tracy]]

verified Firefox 1.4 (Win, Lin, Mac) builds from 2005-09-08