489812 - [FIX] Suspending timeouts doesn't clear mFiringDepth

Status: VERIFIED FIXED

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

Description

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

Attachment: proposed patch

Passes chrome/browser-chrome/mochitest

Ginn, could you test this with your testcase?

Comment 1

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

Doesn't regress https://bugzilla.mozilla.org/show_bug.cgi?id=37907, which bug 315286 fixed.

Comment 2

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

Attachment: Remove some debug leftovers from the test.

Comment 3

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

Comment on attachment 374283 [details] [diff] [review]
Remove some debug leftovers from the test.

Are you sure you want to move the IsFrozen check too? That was in place before we ever started messing with the sync xhr stuff so I'm worried it might cause some other hard-to-find regression. Having said that, though, I think that your change is correct. jst should weigh in.

Comment 4

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

(In reply to comment #3)
> (From update of attachment 374283 [details] [diff] [review])
> Are you sure you want to move the IsFrozen check too? 
As far as I see, that is the right thing to do... but sure, for 1.9.1 we can
make only the minimal change.

Comment 5

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

Attachment: Moving only mTimeoutsSuspendDepth check

Comment 6

[Ginn Chen]

It solved the problem of the website.

Thanks!

Comment 7

[Johnny Stenback (:jst)]

If I understand this right this can cause random timers to never fire, and that could break random sites. Blocking.

Comment 8

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

(In reply to comment #7)
> If I understand this right this can cause random timers to never fire, and that
> could break random sites.
Right.

Any chance for a sreview ;) The latter patch changes only sync-XHR case. The
first patch should fix also the case when page goes to bfcache and then back, but that is an *old* regression.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 374283 [details] [diff] [review]
Remove some debug leftovers from the test.

I think we do want this, and I'd even say we should take this for the branch as well.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 374304 [details] [diff] [review]
Moving only mTimeoutsSuspendDepth check

sr=jst for this one as well, but I still think we should take the other patch on the branch.

Comment 11

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

Comment on attachment 374304 [details] [diff] [review]
Moving only mTimeoutsSuspendDepth check

http://hg.mozilla.org/mozilla-central/rev/531f60d6cf88
I pushed this one. I think I need to write some tests before pushing the other one.

Comment 12

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

Ginn, could you verify (using a trunk build) that the bug is fixed.

Comment 13

[Ginn Chen]

I've confirmed it is fixed in this build.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090426 Minefield/3.6a1pre
Built from http://hg.mozilla.org/mozilla-central/rev/b552b1ef8aa0

Comment 14

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

Attachment: 191 patch

Comment 15

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c77b7d273dfb