320982 - Crash when browse this Site over top menu [@ nsGlobalWindow::RunTimeout]

Status: VERIFIED FIXED

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

Description

[1106-29]

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)]

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 2

[Ria Klaassen (not reading all bugmail)]

1.9a1_2005081307 (no crash) - 1.9a1_2005081323 (crash)

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-08-13+06%3A00%3A00&maxdate=2005-08-13+23%3A00%3A00&cvsroot=%2Fcvsroot

Comment 3

[Ria Klaassen (not reading all bugmail)]

Without flash no crash.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Likely a regression from bug 297561.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

I don't understand step 3 of the steps to reproduce, could somebody clarify?

Comment 6

[Ria Klaassen (not reading all bugmail)]

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: [e-mail]]]

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)]

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]

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]

*** This bug has been marked as a duplicate of 315286 ***

Comment 11

[Martijn Wargers (dead)]

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][[e-mail]]]

Attachment: 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][[e-mail]]]

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) (inactive)]

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) (inactive)]

Attachment: Alternative patch

Comment 16

[Brian Ryner (not reading)]

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) (inactive)]

(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][[e-mail]]]

(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][[e-mail]]]

Attachment: a test case

This doesn't crash, but shows that timers aren't always suspended in frames.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

(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][[e-mail]]]

Trunk crash is a regression from Bug 336674

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 223727 [details] [diff] [review]
a patch for 1.8

Sigh, this is easiest, I suppose.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 223727 [details] [diff] [review]
a patch for 1.8

And what would you think, Brian?

Comment 24

[Brian Ryner (not reading)]

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][[e-mail]]]

> 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) (inactive)]

(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) (inactive)]

Attachment: Something like this

Smaug does this patch help?

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Attachment: test case for online/offline events in iframes

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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) (inactive)]

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][[e-mail]]]

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][[e-mail]]]

Attachment: 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][[e-mail]]]

Comment on attachment 225802 [details] [diff] [review]
for 1.8.1

argh, too late here. wrong one

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Attachment: this one is for 1.8.1

Comment 35

[Brian Ryner (not reading)]

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)]

Could this have caused a Tp regression on btek?

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

So now the patches are in trunk and branch

Comment 39

[Daniel Veditz [:dveditz]]

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]]

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

Comment 41

[Daniel Veditz [:dveditz]]

Olli: can you land this on the 1.8.0 branch as well? Thanks.

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch for 1.8.0 (without storage event fixes)

Comment 43

[Adam Guthrie]

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.