479490 - ###!!! ASSERTION: Mismatched calls to ResumeTimeouts!: 'mTimeoutsSuspendDepth', file //mozilla/dom/src/base/nsGlobalWindow.cpp, line 8436

Status: VERIFIED FIXED

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

Description

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

I get the assertion if using "back for an iframe.

http://mozilla.pettay.fi/xhr_upload/xhr_upload_demo_sync.html
Click the buttons for iframe to add something to shistory, then start (sync)XHR
and press back.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

Here's a first stab... What do you guys think? I think it's hideous, but the other places I tried to hook were unreliable.

Comment 2

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

I wonder why you want top, not parent.
What if there are nested iframes and back-forward is used for the most inner one, 
does the approach still work?

Comment 3

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

Hrm, there is also the case when something is loaded from history (not bfcache). It should have its timeouts suspended if it is loaded to a iframe
inside a window which has timeouts suspended.

(Trying to think of something which combines event suppression handling and
timer suspension here...)

Comment 4

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

FYI, I added a timer test to that test page.

Comment 5

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

For testing browser.sessionhistory.cache_subframes=true is useful.

Comment 6

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

Comment on attachment 363451 [details] [diff] [review]
Patch, v1

we need to do something else.

Comment 7

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

Patch coming, I hope.

Comment 8

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

Attachment: patch

This patch isn't as bad as it looks like.
nsGlobalWindow::SuspendTimeouts/ResumeTimeouts changes are mainly because
of one extra "if". Sub windows' SuspendTimeouts should be called even if
window is already suspended!

The patch applies over bug 333198, and it does fix the crash you Bent saw.
(nsRefPtr in nsGlobalWindow::SetDocShell)

I tested this with and without  browser.sessionhistory.cache_subframes

(Trying to get hg to create a -w patch)

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 363695 [details] [diff] [review]
patch

This patch tests fine for me!

>+  nsresult ResumeTimeoutsNoThaw() { return ResumeTimeouts(PR_FALSE); }

I get why you made this but I think it either doesn't belong on the interface or there should also be a SuspendNoFreeze or something.

>+  virtual PRUint32 TimeoutsSuspended() = 0;

Maybe call this TimeoutSuspendCount? Currently it's not obvious that it returns an int.

>+    for (nsRefPtr<nsGlobalWindow> inner = (nsGlobalWindow *)PR_LIST_HEAD(this);

Nice catch!!!

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 363695 [details] [diff] [review]
patch

- In nsXMLHttpRequest::Send():

           resumeTimeoutRunnable = NS_NEW_RUNNABLE_METHOD(nsPIDOMWindow,
                                                          suspendedWindow.get(),
-                                                         ResumeTimeouts);
+                                                         ResumeTimeoutsNoThaw);

I'd love to see this done w/o having to add this ResumeTimeoutsNoThaw() to the API. I.e. create a new runnable that just calles ResumeTimeouts() with the appropriate arguments, or something. A bit more code, but I think that's a good tradeoff here.

- In nsGlobalWindow::SetNewDocument():

+      nsCOMPtr<nsIDOMWindow> parentWindow;
+      GetParent(getter_AddRefs(parentWindow));
+      nsCOMPtr<nsPIDOMWindow> pWindow = do_QueryInterface(parentWindow);
+      if (pWindow && pWindow->TimeoutsSuspended()) {
+        SuspendTimeouts(pWindow->TimeoutsSuspended());
+      }

This doesn't look right. GetParent() returns |this| if there is no parent (yeah, silly, but that's what it does), and that's probably not what you want here. Use GetParentInternal(), or explicitly check for that case here.

sr=jst with that fixed.

Comment 11

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

I'll update the patch.
Note, this requires bug 333198.

Comment 12

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

Attachment: patch

Comment 13

[Daniel Holbert [:dholbert]]

See also bug 480158 -- looks related. (may be fixed by this bug's patch)

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: patch

Updated patch, interdiff in a sec.

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: interdiff

This simply makes sure that we use the top's inner window rather than the outer in case the inner changes during the sync xhr.

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 364167 [details] [diff] [review]
patch

sr=jst

Comment 18

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

Thanks bent
http://hg.mozilla.org/mozilla-central/rev/fee69bd1bfa7

I'll push this to 1.9.1

Comment 19

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/18e895e6c4fc

Comment 20

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

Attachment: backout patch for 1.9.1

Comment 21

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

Backed out.

Comment 22

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

http://hg.mozilla.org/mozilla-central/rev/9f0feb0c922a

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

and on branch:

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/923cc0596b81

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2]]

So the patch was backed-out on trunk and 1.9.1. Means the assertion has been fixed by a patch on another bug?

Comment 25

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

The patch was backed out and the it relanded.

Comment 26

[Aakash Desai [:aakashd]]

verified FIXED on debug builds:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090706 Minefield/3.6a1pre ID:20090706143743

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.1pre) Gecko/20090706 Shiretoko/3.5.1pre ID:20090706143721