The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla1.9alpha8

Status

()

Core
DOM: Core & HTML
--
critical
RESOLVED FIXED
13 years ago
8 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: Gavin)

Tracking

(Blocks: 1 bug, {hang, testcase})

Trunk
mozilla1.9alpha8
hang, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8rc1 -
blocking1.9 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

13 years ago
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
(Reporter)

Comment 1

13 years ago
Created attachment 160137 [details]
testcase (causes freeze/hang)
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?
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

13 years ago
(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

12 years ago
*** Bug 297522 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 6

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

Comment 7

12 years ago
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
Flags: blocking1.8rc1?

Comment 8

12 years ago
Too late for this to block the release.
Flags: blocking1.8rc1? → blocking1.8rc1-

Comment 9

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

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

Updated

12 years ago
Blocks: 159909

Comment 10

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

Updated

12 years ago
Severity: normal → critical
Keywords: hang

Updated

12 years ago
Keywords: testcase

Comment 11

11 years ago
*** Bug 321025 has been marked as a duplicate of this bug. ***
*** Bug 329404 has been marked as a duplicate of this bug. ***

Comment 13

11 years ago
Bug 331658 has some discussion of this issue.

Comment 14

11 years ago
*** Bug 303902 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 15

11 years ago
*** Bug 335256 has been marked as a duplicate of this bug. ***
Summary: Freeze/hang on this site, because of setInterval('stay()'); → Freeze/hang on this site, because of setInterval('stay()'); (recursive setInterval/setTimeout)

Comment 16

11 years ago
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. 
(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

11 years ago
(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>
Created attachment 227920 [details] [diff] [review]
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?
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

11 years ago
(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. 

Flags: blocking1.9a1?

Updated

11 years ago
Flags: blocking1.9a1? → blocking1.9+
Taking bug.
Assignee: general → jst

Comment 23

11 years ago
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.

Updated

10 years ago
Blocks: 384323
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?
Attachment #227920 - Flags: superreview+
Attachment #227920 - Flags: review+
Wow, I'd forgotten about this bug. I can land this.
Assignee: jst → gavin.sharp
Whiteboard: [checkin needed]
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha6
Created attachment 268970 [details]
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.
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.
Assignee: gavin.sharp → jst
punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
Attachment #160137 - Attachment is obsolete: true
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
Bug 335256 will cover the more general "recursive setTimeout DoS" issue.
Assignee: jst → gavin.sharp
Whiteboard: [checkin needed]
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Duplicate of this bug: 286905
You need to log in before you can comment on or make changes to this bug.