635888 - Meta refresh breaks slow script warning

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Test case - WARNING: may hang your browser!

When loading the attached file the following happens:

1) Slow script warning after x seconds
2) Meta refresh causes page to reload
3) Browser hangs - no slow script warning

This works as expected in 3.6.13

Comment 1

[Jan de Mooij [:jandem]]

Btw you have to click "Stop script" after step 1, of course.

Should this block? It's a regression, an easy way to (accidentally) hang the browser, and it's just annoying.

Comment 2

[Jan de Mooij [:jandem]]

FF4 beta 10: OK
FF4 beta 11: broken

Bisecting...

Comment 3

[Jan de Mooij [:jandem]]

Works: Feb 01 - http://hg.mozilla.org/mozilla-central/rev/8b5cb26bbb10
Fails: Feb 02 - http://hg.mozilla.org/mozilla-central/rev/1c2d53a2dcfb

Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8b5cb26bbb10&tochange=1c2d53a2dcfb

Comment 4

[Jan de Mooij [:jandem]]

Bug 621764 and bug 620615 look suspicious. CC'ing people.

Comment 5

[Andreas Gal :gal]

Yeah bug 620615 looks like a good candidate.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to comment #5)
> Yeah bug 620615 looks like a good candidate.

OK, blocking on that bug and moving to Toolkit bugs.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

It's almost certainly Bug 621764.

Comment 8

[:Paolo Amadini]

I can't verify this at present as my build is still in progress, but a quick
look at attachment 508997 [details] [diff] [review] makes me think that the new EnterModalState function
of the window could call EnterModalState on the current context twice:

http://mxr.mozilla.org/comm-central/source/mozilla/dom/base/nsGlobalWindow.cpp#6356

This is probably fine, but the problem might be that the LeaveModalState
function of the window will only call LeaveModalState on the current context
once if the return value of EnterModalState is not provided as an argument:

http://mxr.mozilla.org/comm-central/source/mozilla/dom/base/nsGlobalWindow.cpp#6469

Comment 9

[Robert Sayre]

Jan de Mooij, how did you find this?

Comment 10

[Johnathan Nightingale [:johnath]]

Won't hold the release, but dolske, can we get this investigated, and re-nom'd if we think it's likely to be widespread?

Comment 11

[Jan de Mooij [:jandem]]

(In reply to comment #9)
> Jan de Mooij, how did you find this?

I use meta refreshes to load random scripts in the browser. I also use window.location = ... but that fails if the script throws. Refreshing is cheaper than starting a new browser process for each test. I'm afraid this might hurt other test frameworks.

Comment 12

[:Paolo Amadini]

Attachment: Fix known callers to use the return value of EnterModalState

NOTE: At present, this patch is provided "as is" without testing of any kind!

It seems that comment 8 was in the right direction, and this patch fixes the
problem by changing all callers to pass the return value of EnterModalState
to LeaveModalState. I'm not sure if there is a convenient way to cover the
patch with regression tests.

I included the fix for bug 633891 (remove an unused line) because it is a
follow-up to bug 621764 too.

Thanks to Jan for finding the regression range!

Comment 13

[Justin Dolske [:Dolske]]

Comment on attachment 515773 [details] [diff] [review]
Fix known callers to use the return value of EnterModalState

Yay, was just getting to this today and I see you've got a patch. Laziness wins again! ;)

Anyway, looks fine to me but over to jst since I can't review this.

The other approach here would be to change EnterModalState to return the window COM-style -- EMS(nsDOMWindow **aWindow) -- and not have it twiddle the one thing when aWindow == nsnull. I don't think there's much advantage of one over the other.

Comment 14

[:Paolo Amadini]

I've now verified that the patch passes the existing regression tests (the few
failures I observed on the tryserver build, on some platforms, seem unrelated).

At this point in the cycle, I'm not sure if this patch qualifies for inclusion
in the release candidate and if so what is the procedure / which flags to use.

Comment 15

[Justin Dolske [:Dolske]]

Comment on attachment 515773 [details] [diff] [review]
Fix known callers to use the return value of EnterModalState

I think this is perfectly safe to take, so a+ing.

Comment 16

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

Comment on attachment 515773 [details] [diff] [review]
Fix known callers to use the return value of EnterModalState

Sadly, didn't land in time for RC1.

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/b92ca278fe21

Comment 18

[Vlad [QA]]

Still reproducible for me on Mozilla/5.0 (Windows NT 6.1; rv:6.0) Gecko/20100101 Firefox/6.0.
The dialog appear and I get a refresh even if I press continue or stop script.
The same behavior is present if I try  to close the dialog from the x button.
In all this time, Firefox is not responding.