Last Comment Bug 320982 - Crash when browse this Site over top menu [@ nsGlobalWindow::RunTimeout]
: Crash when browse this Site over top menu [@ nsGlobalWindow::RunTimeout]
Status: VERIFIED FIXED
: crash, fixed1.8.1, regression, verified1.8.0.5
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- critical with 1 vote (vote)
: ---
Assigned To: Olli Pettay [:smaug] (TPAC)
: Hixie (not reading bugmail)
Mentors:
http://www.scriptinternals.de/new/ger...
Depends on:
Blocks: sa19873
  Show dependency treegraph
 
Reported: 2005-12-20 10:30 PST by 1106-29
Modified: 2006-06-26 09:55 PDT (History)
16 users (show)
dveditz: blocking1.8.0.5+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
a patch for 1.8 (5.01 KB, patch)
2006-05-29 14:53 PDT, Olli Pettay [:smaug] (TPAC)
mrbkap: review+
Details | Diff | Splinter Review
Alternative patch (3.17 KB, patch)
2006-06-06 15:50 PDT, Blake Kaplan (:mrbkap)
bryner: superreview+
Details | Diff | Splinter Review
a test case (1.49 KB, text/html)
2006-06-07 04:06 PDT, Olli Pettay [:smaug] (TPAC)
no flags Details
Something like this (4.57 KB, patch)
2006-06-07 14:07 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
test case for online/offline events in iframes (1.07 KB, text/html)
2006-06-08 00:37 PDT, Olli Pettay [:smaug] (TPAC)
no flags Details
Freeze/thaw on inner window and fixes to delayed dom events (7.99 KB, patch)
2006-06-08 00:53 PDT, Olli Pettay [:smaug] (TPAC)
mrbkap: review+
bryner: superreview+
Details | Diff | Splinter Review
for 1.8.1 (11.20 KB, patch)
2006-06-15 17:27 PDT, Olli Pettay [:smaug] (TPAC)
no flags Details | Diff | Splinter Review
this one is for 1.8.1 (11.22 KB, patch)
2006-06-15 17:44 PDT, Olli Pettay [:smaug] (TPAC)
bryner: review+
bryner: superreview+
bryner: approval‑branch‑1.8.1+
dveditz: approval1.8.0.5+
Details | Diff | Splinter Review
patch for 1.8.0 (without storage event fixes) (4.27 KB, patch)
2006-06-24 06:22 PDT, Olli Pettay [:smaug] (TPAC)
no flags Details | Diff | Splinter Review

Description 1106-29 2005-12-20 10:30:12 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8) Gecko/20051111 Firefox/1.5

Browser Crash when you browse the site over the top menu
or when you go to other site in this tab.
i test on 1.5 see Build Identifier
and last 1.6a2 from 20.12.05 with clear profile


Reproducible: Always

Steps to Reproduce:
1.www.scriptinternals.de
2.over top menu goto download SystemScripter 6.0
3.wait or to next page or new site in same tap
Comment 1 Ria Klaassen (not reading all bugmail) 2005-12-20 11:12:39 PST
Confirmed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051220 Firefox/1.6a1 ID:2005122005

TB13161792E (trunk)
TB13161826W (branch)
Comment 3 Ria Klaassen (not reading all bugmail) 2005-12-21 00:13:34 PST
Without flash no crash.
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-12-21 06:48:18 PST
Likely a regression from bug 297561.
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-12-21 06:51:27 PST
I don't understand step 3 of the steps to reproduce, could somebody clarify?
Comment 6 Ria Klaassen (not reading all bugmail) 2005-12-21 07:06:11 PST
On top of http://www.scriptinternals.de/new/ger/home/default.htm there's a dropdown menu (Download). In that menu go to System Scripter 0.6. The next page crashes.
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-12-21 07:37:14 PST
Ah, ok, seems like you need to move the mouse before the new page loads. I got it to crash about half of the time when using these steps:

1) Go to http://www.scriptinternals.de/new/ger/home/default.htm
2) Click "SystemScripter 6.0" in the "Download" drop down menu at the top
3) Before the new page loads, start moving the mouse across the flash banner
Comment 8 Ria Klaassen (not reading all bugmail) 2005-12-21 07:42:25 PST
Here it crashes immediately after leaving the flash animation on top of the second page. If I stay with my mouse inside the flash animation it doesn't crash. 
Comment 9 Adam Guthrie 2005-12-21 12:16:43 PST
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20051220 Firefox/1.6a1

#6  0xb5332a04 in nsGlobalWindow::RunTimeout (this=0x8960628, aTimeout=0x8ce2fc8) at /moz/trunk/mozilla/dom/src/base/nsGlobalWindow.cpp:6306
#7  0xb5332c90 in nsGlobalWindow::TimerCallback (aTimer=0x8ce7b60, aClosure=0x8ce2fc8) at /moz/trunk/mozilla/dom/src/base/nsGlobalWindow.cpp:6602
#8  0xb7d8ca3b in nsTimerImpl::Fire (this=0x8ce7b60) at /moz/trunk/mozilla/xpcom/threads/nsTimerImpl.cpp:400
#9  0xb7d8cccd in handleTimerEvent (aEvent=0xb1700588) at /moz/trunk/mozilla/xpcom/threads/nsTimerImpl.cpp:467
#10 0xb7d847ae in PL_HandleEvent (self=0xb1700588) at /moz/trunk/mozilla/xpcom/threads/plevent.c:688
#11 0xb7d8463a in PL_ProcessPendingEvents (self=0x80e1d90) at /moz/trunk/mozilla/xpcom/threads/plevent.c:623
#12 0xb7d876bd in nsEventQueueImpl::ProcessPendingEvents (this=0x80e1d48) at /moz/trunk/mozilla/xpcom/threads/nsEventQueue.cpp:417
#13 0xb5d00230 in event_processor_callback (source=0x82ee438, condition=G_IO_IN, data=0x80e1d48) at /moz/trunk/mozilla/widget/src/gtk2/nsAppShell.cpp:67
#14 0xb770531c in g_vasprintf () from /usr/lib/libglib-2.0.so.0
#15 0xb76de4ee in g_main_context_dispatch () from /usr/lib/libglib-2.0.so.0
#16 0xb76e14f6 in g_main_context_check () from /usr/lib/libglib-2.0.so.0
#17 0xb76e19d8 in g_main_context_iteration () from /usr/lib/libglib-2.0.so.0
#18 0xb7ae26a8 in gtk_main_iteration () from /usr/lib/libgtk-x11-2.0.so.0
#19 0xb1ae1a72 in gtkTimerCallback () from /home/aguthrie/.mozilla/plugins/libflashplayer.so
#20 0xb76e0006 in g_main_context_wakeup () from /usr/lib/libglib-2.0.so.0
#21 0xb76de4ee in g_main_context_dispatch () from /usr/lib/libglib-2.0.so.0
#22 0xb76e14f6 in g_main_context_check () from /usr/lib/libglib-2.0.so.0
#23 0xb76e17e3 in g_main_loop_run () from /usr/lib/libglib-2.0.so.0
#24 0xb7ae2e65 in gtk_main () from /usr/lib/libgtk-x11-2.0.so.0
#25 0xb5d00c26 in nsAppShell::Run (this=0x816f400) at /moz/trunk/mozilla/widget/src/gtk2/nsAppShell.cpp:139
#26 0xb708086f in nsAppStartup::Run (this=0x816f3b8) at /moz/trunk/mozilla/toolkit/components/startup/src/nsAppStartup.cpp:161
#27 0xb7ed80ed in XRE_main (argc=2, argv=0xbfd2aa04, aAppData=0x8049820) at /moz/trunk/mozilla/toolkit/xre/nsAppRunner.cpp:2298
#28 0x08048676 in main (argc=2, argv=0xbfd2aa04) at /moz/trunk/mozilla/browser/app/nsBrowserApp.cpp:61
Comment 10 Adam Guthrie 2005-12-21 12:16:57 PST

*** This bug has been marked as a duplicate of 315286 ***
Comment 11 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-04-22 23:58:28 PDT
Yeah, I still crash with current trunk build on this one.
Talkback ID: TB17867994E
nsGlobalWindow::ClearTimeoutOrInterval   nsGlobalWindow::ClearTimeoutOrInterval   nsGlobalWindow::ClearInterval   XPCWrappedNative::CallMethod  
Comment 12 Olli Pettay [:smaug] (TPAC) 2006-05-29 14:53:49 PDT
Created attachment 223727 [details] [diff] [review]
a patch for 1.8

This is a possible patch (for 1.8 for now since the dhtml menu on the page 
doesn't work too well in 1.9 - flash is painted above it).
The change to t->Release(nsnull) in nsGlobalWindow::SuspendTimeouts should fix 
nsGlobalWindow::ClearAllTimeouts crashes, at least those ones I saw during debugging this.
mTimeoutsSuspended makes sure that timeouts aren't used in child docshells of
a frozen window.
Asking review to get comments whether this is good enough, or should IsFrozen/Freeze() work in a bit different way, somehow recursively.
Comment 13 Olli Pettay [:smaug] (TPAC) 2006-06-06 02:26:36 PDT
I can see this crash also in 1.8.0, and even without Flash.
Just before the crash there is
###!!! ASSERTION: How'd our timer end up null if we're not frozen?: 'IsFrozen()', file /home/smaug/mozilla/mozilla_cvs/1.8.0_branch/mozilla/dom/src/base/nsGlobalWindow.cpp, line 6465

Patch fixes the crash, but as I asked, would it be better to make Freeze() to
work in a bit different way?
Comment 14 Blake Kaplan (:mrbkap) 2006-06-06 15:18:59 PDT
Moving the Release in SuspendTimeouts is a good fix and I'm sorry that I was so wrong in the first place.

I think that the right fix for the bug is to call Freeze() before suspending the timeouts, though. This avoids the extra member. I've looked, and there isn't anything that should be depending on the inner window's mFrozen member.

I'll attach a patch once my build finishes and I can test it.
Comment 15 Blake Kaplan (:mrbkap) 2006-06-06 15:50:33 PDT
Created attachment 224621 [details] [diff] [review]
Alternative patch
Comment 16 Brian Ryner (not reading) 2006-06-06 15:59:04 PDT
Comment on attachment 224621 [details] [diff] [review]
Alternative patch

Do you think you could expand the comment next to Freeze a bit, so that if I'm reading this code I'll have an idea what sort of things _could_ happen to the inner window, and why we don't want them to?

Looks good otherwise.
Comment 17 Blake Kaplan (:mrbkap) 2006-06-06 16:53:54 PDT
(In reply to comment #12)
> mTimeoutsSuspended makes sure that timeouts aren't used in child docshells of
> a frozen window.

My patch addresses this, and I believe that it is possible, but could you attach a stack or explain how a timeout managed to fire between the entrance to nsGlobalWindow::SaveWindowState and the call to nsGlobalWindow::SuspendTimeouts?
Comment 18 Olli Pettay [:smaug] (TPAC) 2006-06-07 01:06:33 PDT
(In reply to comment #17)
> My patch addresses this, and I believe that it is possible, but could you
> attach a stack or explain how a timeout managed to fire between the entrance to
> nsGlobalWindow::SaveWindowState and the call to
> nsGlobalWindow::SuspendTimeouts?
> 

It is not at that time, I think.
attachment 224621 [details] [diff] [review] does *not* fix the crash, at least not in 1.8
(Can't test on trunk since it is not working at all in 64bit Linux, Bug 340452)

This is one stack I got (with attachment 224621 [details] [diff] [review])
#0  0x00000038c520cf6d in raise () from /lib64/libpthread.so.0
#1  0x000000000041892b in nsProfileLock::FatalSignalHandler (signo=11) at nsProfileLock.cpp:206
#2  <signal handler called>
#3  0x00002aaab137062c in nsGlobalWindow::RunTimeout (this=0x1f9b960, aTimeout=0x124c680)
    at /home/smaug/mozilla/mozilla_cvs/18_branch/mozilla/dom/src/base/nsGlobalWindow.cpp:6460
#4  0x00002aaab1370b9f in nsGlobalWindow::TimerCallback (aTimer=Variable "aTimer" is not available.
)
    at /home/smaug/mozilla/mozilla_cvs/18_branch/mozilla/dom/src/base/nsGlobalWindow.cpp:6914
#5  0x00002aaaaae15f1c in nsTimerImpl::Fire (this=0x61f350) at /home/smaug/mozilla/mozilla_cvs/18_branch/mozilla/xpcom/threads/nsTimerImpl.cpp:394
#6  0x00002aaaaae16138 in handleTimerEvent (event=0x2aaab3a01e80) at /home/smaug/mozilla/mozilla_cvs/18_branch/mozilla/xpcom/threads/nsTimerImpl.cpp:459
#7  0x00002aaaaae0fe49 in PL_HandleEvent (self=0x2aaab3a01e80) at /home/smaug/mozilla/mozilla_cvs/18_branch/mozilla/xpcom/threads/plevent.c:688
#8  0x00002aaaaae101af in PL_ProcessPendingEvents (self=0x5bdfa0) at /home/smaug/mozilla/mozilla_cvs/18_branch/mozilla/xpcom/threads/plevent.c:623
#9  0x00002aaaaae127af in nsEventQueueImpl::ProcessPendingEvents (this=0x5bdda0)
    at /home/smaug/mozilla/mozilla_cvs/18_branch/mozilla/xpcom/threads/nsEventQueue.cpp:417
#10 0x00002aaaaf8b3812 in event_processor_callback (source=0x7fffff895460, condition=0, data=0x4159dbf04813a)
    at /home/smaug/mozilla/mozilla_cvs/18_branch/mozilla/widget/src/gtk2/nsAppShell.cpp:67
#11 0x00000038c6a2703a in g_main_context_dispatch () from /usr/lib64/libglib-2.0.so.0
#12 0x00000038c6a2a1b5 in g_main_context_check () from /usr/lib64/libglib-2.0.so.0
#13 0x00000038c6a2a4dd in g_main_loop_run () from /usr/lib64/libglib-2.0.so.0
#14 0x00000038c8a1ed03 in gtk_main () from /usr/lib64/libgtk-x11-2.0.so.0
#15 0x00002aaaaf8b3dd2 in nsAppShell::Run (this=0x69b860) at /home/smaug/mozilla/mozilla_cvs/18_branch/mozilla/widget/src/gtk2/nsAppShell.cpp:139
#16 0x00002aaaafc37caa in nsAppStartup::Run (this=0x69b810)
    at /home/smaug/mozilla/mozilla_cvs/18_branch/mozilla/toolkit/components/startup/src/nsAppStartup.cpp:150
#17 0x0000000000409575 in XRE_main (argc=Variable "argc" is not available.
) at /home/smaug/mozilla/mozilla_cvs/18_branch/mozilla/toolkit/xre/nsAppRunner.cpp:2376
#18 0x00000000004041a8 in main (argc=Variable "argc" is not available.
) at /home/smaug/mozilla/mozilla_cvs/18_branch/mozilla/browser/app/nsBrowserApp.cpp:61
Comment 19 Olli Pettay [:smaug] (TPAC) 2006-06-07 04:06:17 PDT
Created attachment 224680 [details]
a test case

This doesn't crash, but shows that timers aren't always suspended in frames.
Comment 20 Olli Pettay [:smaug] (TPAC) 2006-06-07 06:33:39 PDT
(In reply to comment #19)
> Created an attachment (id=224680) [edit]
> a test case
> 
> This doesn't crash, but shows that timers aren't always suspended in frames.
> 
You may have to shift-reload before seeing the problem.
And I got trunk working, and there I see a crash, but apparently it is
not the same timer related bug.

Comment 21 Olli Pettay [:smaug] (TPAC) 2006-06-07 07:04:45 PDT
Trunk crash is a regression from Bug 336674
Comment 22 Blake Kaplan (:mrbkap) 2006-06-07 09:17:37 PDT
Comment on attachment 223727 [details] [diff] [review]
a patch for 1.8

Sigh, this is easiest, I suppose.
Comment 23 Olli Pettay [:smaug] (TPAC) 2006-06-07 09:26:48 PDT
Comment on attachment 223727 [details] [diff] [review]
a patch for 1.8

And what would you think, Brian?
Comment 24 Brian Ryner (not reading) 2006-06-07 09:35:51 PDT
Comment on attachment 223727 [details] [diff] [review]
a patch for 1.8

>--- dom/src/base/nsGlobalWindow.cpp	19 May 2006 18:23:11 -0000	1.761.2.37
>+++ dom/src/base/nsGlobalWindow.cpp	29 May 2006 21:13:51 -0000
>@@ -6321,17 +6322,17 @@ nsGlobalWindow::SetTimeoutOrInterval(PRB
> 
>     rv = sSecMan->CheckSameOriginPrincipal(subjectPrincipal, ourPrincipal);
>     timeout->mPrincipal = NS_SUCCEEDED(rv) ? subjectPrincipal : ourPrincipal;
>     rv = NS_OK;
>   }
> 
>   PRTime delta = (PRTime)interval * PR_USEC_PER_MSEC;
> 
>-  if (!IsFrozen()) {
>+  if (!mTimeoutsSuspended) {
>     // If we're not currently frozen, then we set timeout->mWhen to be the
>     // actual firing time of the timer (i.e., now + delta). We also actually
>     // create a timer and fire it off.

The comment here no longer matches the |if| check.

Just to make sure I understand (I probably knew the answer at one time), we suspend timeouts on child windows, but we don't call Freeze() on them, and that's why this is necessary?
Comment 25 Olli Pettay [:smaug] (TPAC) 2006-06-07 11:41:18 PDT
> The comment here no longer matches the |if| check.

The comments should be then "If timeouts are not suspended, then..."
> 
> Just to make sure I understand (I probably knew the answer at one time), we
> suspend timeouts on child windows, but we don't call Freeze() on them, and
> that's why this is necessary?

Yes. The question I had/have is that should we freeze also child windows?
Although, with this patch maybe that is not needed. 

Comment 26 Blake Kaplan (:mrbkap) 2006-06-07 11:42:30 PDT
(In reply to comment #25)
> Yes. The question I had/have is that should we freeze also child windows?
> Although, with this patch maybe that is not needed. 

I've been considering this too. We could call Freeze on the sub-windows in SuspendTimeouts, and Thaw them in ResumeTimeouts.
Comment 27 Blake Kaplan (:mrbkap) 2006-06-07 14:07:11 PDT
Created attachment 224757 [details] [diff] [review]
Something like this

Smaug does this patch help?
Comment 28 Olli Pettay [:smaug] (TPAC) 2006-06-08 00:37:15 PDT
Created attachment 224836 [details]
test case for online/offline events in iframes
Comment 29 Olli Pettay [:smaug] (TPAC) 2006-06-08 00:53:32 PDT
Created attachment 224838 [details] [diff] [review]
Freeze/thaw on inner window and fixes to delayed dom events

This is based on attachment 224757 [details] [diff] [review], but calling Freeze/Thaw on inner window and
also fixing problems with offline/online and storage events when windows
are restored from bfcache.
Comment 30 Blake Kaplan (:mrbkap) 2006-06-08 09:26:54 PDT
Comment on attachment 224838 [details] [diff] [review]
Freeze/thaw on inner window and fixes to delayed dom events

Oops. I obviously need to test my patches more! This looks great.
Comment 31 Olli Pettay [:smaug] (TPAC) 2006-06-15 13:27:35 PDT
Comment on attachment 224838 [details] [diff] [review]
Freeze/thaw on inner window and fixes to delayed dom events

oops, this doesn't apply to 1.8.1, but checked in to trunk. Will do the 1.8.1 version of the patch.
Comment 32 Olli Pettay [:smaug] (TPAC) 2006-06-15 17:27:43 PDT
Created attachment 225802 [details] [diff] [review]
for 1.8.1

Otherwise the same patch, but no need to handle online/offline events (which are trunk only) and FireDelayedDOMEvents doesn't exists in branch, so I had to add it 
to nsPIDOMWindow_MOZILLA_1_8_BRANCH
Comment 33 Olli Pettay [:smaug] (TPAC) 2006-06-15 17:30:01 PDT
Comment on attachment 225802 [details] [diff] [review]
for 1.8.1

argh, too late here. wrong one
Comment 34 Olli Pettay [:smaug] (TPAC) 2006-06-15 17:44:15 PDT
Created attachment 225806 [details] [diff] [review]
this one is for 1.8.1
Comment 35 Brian Ryner (not reading) 2006-06-15 17:59:27 PDT
Comment on attachment 225806 [details] [diff] [review]
this one is for 1.8.1

>--- dom/src/base/nsGlobalWindow.cpp	15 Jun 2006 03:15:47 -0000	1.761.2.41
>+++ dom/src/base/nsGlobalWindow.cpp	16 Jun 2006 00:39:47 -0000
>@@ -7155,16 +7208,23 @@ nsGlobalWindow::SaveWindowState(nsISuppo
>   if (!mContext || !mJSObject) {
>     // The window may be getting torn down; don't bother saving state.
>     return NS_OK;
>   }
> 
>   nsGlobalWindow *inner = GetCurrentInnerWindowInternal();
>   NS_ASSERTION(inner, "No inner window to save");
> 
>+  // Don't do anything else to this inner window! After this point, all
>+  // calls to SetTimeoutOrInterval will create entries in the timeout
>+  // list that will only run after this window has come out of the bfcache.
>+  // Also, while we're frozen, we won't dispatch online/offline events
>+  // to the page.

Get rid of the reference to online/offline events since those aren't on the branch anyway

Looks ok otherwise
Comment 36 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-06-15 19:38:55 PDT
Could this have caused a Tp regression on btek?
Comment 37 Olli Pettay [:smaug] (TPAC) 2006-06-16 02:49:47 PDT
I backed out the patch, but couldn't really see any difference in tp, 
though the noise is quite bad.
Comment 38 Olli Pettay [:smaug] (TPAC) 2006-06-16 02:50:20 PDT
So now the patches are in trunk and branch
Comment 39 Daniel Veditz [:dveditz] 2006-06-23 20:35:23 PDT
Comment on attachment 225806 [details] [diff] [review]
this one is for 1.8.1

approved for 1.8.0 branch, a=dveditz
Comment 40 Daniel Veditz [:dveditz] 2006-06-24 00:09:24 PDT
*** Bug 342507 has been marked as a duplicate of this bug. ***
Comment 41 Daniel Veditz [:dveditz] 2006-06-24 00:10:55 PDT
Olli: can you land this on the 1.8.0 branch as well? Thanks.
Comment 42 Olli Pettay [:smaug] (TPAC) 2006-06-24 06:22:55 PDT
Created attachment 226916 [details] [diff] [review]
patch for 1.8.0 (without storage event fixes)
Comment 43 Adam Guthrie 2006-06-26 09:55:57 PDT
Verified FIXED using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.5) Gecko/20060626 Firefox/1.5.0.5.

No crash anymore when using Gavin's steps to reproduce in comment 7.

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