261633 - Freeze/hang on this site, because of setInterval('stay()'); (recursive setInterval/setTimeout)

Status: RESOLVED FIXED

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

Description

[Martijn Wargers (dead)]

Mozilla is currently freezing/hanging on this site. 
It happens because of this script:
http://www.sogi.com.tw/js/bannerR.js
The function 'stay' has this line of code:
setInterval('stay()'); 
That is basically the same as: setInterval('stay()',0);

The code from that site has gotten 'live' for Mozilla since the undetected
document.all support bug was fixed.

A simple testcase with this code will show the bug:
function stay(){setInterval('stay()',0);}
stay(); 

This doesn't cause a freeze/hang in:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.1a) Gecko/20020611
But it does cause a freeze/hang in:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.1b) Gecko/20020722

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase (causes freeze/hang)

Comment 2

[Boris Zbarsky [:bzbarsky]]

That code (correctly) leads to exponential growth in the number of intervals set
(doubling every 10ms).  Not freezing would be nice, though.... any bright ideas
on how to do th at?

Comment 3

[Brendan Eich [:brendan]]

Put a generous but sufficient quota on the number of outstanding timeouts and
intervals?

In Netscape 2-4 we slapped quotas on various things, including number of top
level windows (50, IIRC).  No one cried, but that was then.

/be

Comment 4

[Hung-Te Lin]

(In reply to comment #3)
> In Netscape 2-4 we slapped quotas on various things, including number of top
> level windows (50, IIRC).  No one cried, but that was then.

 Even 10 can still prevent Firefox from freezing.
 So I think this is a feasible solution.
 Or, set a boundary value (e.g, 10, or 50) for the argument of setInterval.

Comment 5

[Jan Darmochwal]

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

Comment 6

[Martijn Wargers (dead)]

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

Comment 7

[Jon Henry]

Is it too late to develop a fix for this for Firefox 1.5? It was pointed out
today on the Mozillazine forums that this bug still occurs in branch nightlies.

http://forums.mozillazine.org/viewtopic.php?t=330510&postdays=0&postorder=asc&postsperpage=15&start=15

Comment 8

[Asa Dotzler [:asa]]

Too late for this to block the release.

Comment 9

[u221686]

this crash also occurrs on: http://ero.gameflier.com.my/default.htm

again, due to the following javascript:
function i(){ setInterval('stay()'); }
stay();

Comment 10

[Jesse Ruderman]

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

Comment 11

[:Mook]

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

Comment 12

[Daniel Veditz [:dveditz]]

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

Comment 13

[Jesse Ruderman]

Bug 331658 has some discussion of this issue.

Comment 14

[dolphinling]

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

Comment 15

[Martijn Wargers (dead)]

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

Comment 16

[Arphen Lin]

Mozilla seems to treat setInterval('test()') as setInterval('test()', 0), it's the worst solution.

IE 6.0 handles setInterval('test()') like setTimeout('test()', 0), it's acceptable.

Opera 9.0 only allows setInterval('test()') to execute test() one time, after that, it stopped. I think this is the best solution. 

Comment 17

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

(In reply to comment #16)
> Mozilla seems to treat setInterval('test()') as setInterval('test()', 0), it's
> the worst solution.
> 
> IE 6.0 handles setInterval('test()') like setTimeout('test()', 0), it's
> acceptable.
> 
> Opera 9.0 only allows setInterval('test()') to execute test() one time, after
> that, it stopped. I think this is the best solution. 

I don't understand. How is Opera's behavior different than IE's? I don't see what how "setTimeout('test()', 0)" is different from "execute test() one time, after that, it stopped".

Comment 18

[Arphen Lin]

(In reply to comment #16)
> Mozilla seems to treat setInterval('test()') as setInterval('test()', 0), it's
> the worst solution.
> 
> IE 6.0 handles setInterval('test()') like setTimeout('test()', 0), it's
> acceptable.
> 
> Opera 9.0 only allows setInterval('test()') to execute test() one time, after
> that, it stopped. I think this is the best solution. 

I found there are some errors in my previous post, so I test setInterval() again.
Here is my testcase, and I found that Opera's solution is the best.
--------------------------------------------------------------------------------
<html>
<head>
  <script>
  function test(){
    var d=document.getElementById('myd');
    if(d) d.innerHTML = Math.random();
  }
  </script>
</head>

<body>

  <div id="myd">

  <script>
  // IE 6: execute just one time
  // Opera 9: don't execute
  // Firefox 1.5: execute as fast as possible repeatedly
  setInterval('test()');

  // IE 6: execute just one time
  // Opera 9: execute as fast as possible repeatedly
  // Firefox 1.5: same with opera
  setInterval('test()', 0);

  // IE 6: execute every 1ms repeatedly
  // Opera 9: same with IE
  // Firefox 1.5: same with IE
  setInterval('test()', 1);
  </script>

</body>
</html>

Comment 19

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

Attachment: treat setInterval('foo()') as setTimeout('foo()', 0)

According to Arphen's testing, this would make us match IE in the single-argument setInterval() case. I wonder if Opera's behavior is best - treat the single-argument setInterval as an error?

Comment 20

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

Mapping setInterval(<func>) to setTimeout(<func>, 0) seems kind of arbitrary, but I'm not sure what people expect setInterval to do without an "interval length" param anyways. Treating "undefined" as 0 (our current behavior) seems most logical, but apparently some people have started assuming it does something else. 

Arphen: does Opera throw an exception when you call setInterval("test()"), or does it just do nothing?

Comment 21

[Arphen Lin]

(In reply to comment #20)
> Arphen: does Opera throw an exception when you call setInterval("test()"), or
> does it just do nothing?

No, I didn't see any exception in Opera's javascript console. 

Comment 22

[Johnny Stenback (:jst)]

Taking bug.

Comment 23

[Jesse Ruderman]

See also bug 352750, "Exponential growth in number of timeouts hangs browser in InsertTimeoutIntoList".  I think a fix for that should make abuses of setInterval that *don't* go away with the IE-compatibility fixes in the bug hang less.

Comment 24

[Johnny Stenback (:jst)]

Comment on attachment 227920 [details] [diff] [review]
treat setInterval('foo()') as setTimeout('foo()', 0)

r+sr=jst for this fix. Gavin, can you land this one, or you want me to?

Comment 25

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

Wow, I'd forgotten about this bug. I can land this.

Comment 26

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

Attachment: alternate testcase (fixed by patch)

Oops, I forgot that my fix only fixed one of the specific cases where this happens. Here's a testcase for that case.

Comment 27

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

I still haven't had a chance to land this patch, but I should be able to tomorrow. Handing this back to jst, since my fix isn't complete.

Comment 28

[Mike Connor [:mconnor]]

punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.

Comment 29

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

Landed my patch, which fixes the second testcase and the cause of the original report. The first testcase still isn't fixed, but jst thinks that should be dealt with in another bug.

mozilla/dom/src/base/nsGlobalWindow.cpp 	1.946
mozilla/dom/src/base/nsGlobalWindow.h 	1.309
mozilla/dom/src/base/nsJSTimeoutHandler.cpp 	1.9

Comment 30

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

Bug 335256 will cover the more general "recursive setTimeout DoS" issue.