77578 - Whole app hangs www.mrshowbiz.com (wrong eval scope for JS)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[David Hyatt]

Go to www.mrshowbiz.com.  Click on the "News" image in the upper left hand corner.

The entire app hangs.  Breaking into the debugger always shows us in DOM
property fetching code.

My hope is that this bug will be fixed by the new XPConnectified DOM.

Comment 1

[Fabian Guisset]

Worksforme win2k build 2001042513 and linux cvs build from 4/27 8AM

Comment 2

[David Hyatt]

Current build as of 4/28 still exhibits the problem.  This is not worksforme.

Comment 3

[Johnny Stenback (:jst)]

Clicking on the News link once doesn't trigger this hang for me but if I click
on News, and the once that loads, click on celebreties I see the hang. The hang
is in the onclick handler of the clicked link, the code looks something like
this (this is not the onclick handler itself, it's called by the onclick
handler, _k is 'this', i.e. the link element)...

function _ri(_k,_w) {
  var _ilc=0;

  while(document.links[_ilc]!=_k) {
    _ilc++;
  }

  ...
}

mozilla ends up being stuck in the while loop since the link that's clicked on
isn't found in document.links[], for some reason, I don't know why yet tho...

Any debugging help would be appreciated...

Comment 4

[Johnny Stenback (:jst)]

This is really weird, sometimes it hangs, sometimes it doesn't...

Comment 5

[Fabian Guisset]

Ok I got it to hang twice, but not after clicking on those links once or twice:
i had to click ten times or so. Quite hard to reproduce now. It can hang either
when clicking on news or when clicking on celebrities. I haven't got the other
buttons to hang us (yet?). Also, Johnny, I see no onclick handler anywhere, nor
the function you copy-pasted. Point me to the js file that has it?
(On a side note: page contains evil vbscript :P)
Adding hang keyword. 

Comment 6

[Johnny Stenback (:jst)]

http://a1604.g.akamai.net/f/1604/2007/1h/stats.hitbox.com/js/hbe-v65-no10-dig.js

is the JS file that contains the function that makes mozilla hang, whoever wrote
that file made our life's much more fun by removing all whitespaces from the
file and using function and variable names that mean as little as possible to
the reader. The same file is the file that sets up the onclick handler on the
links (i.e. they're not in the HTML).

I'll attach a more readable version of that file.

Comment 7

[Johnny Stenback (:jst)]

Attachment: More readable version of the file with the function where we hang.

Comment 8

[Prashant Desale]

Updating QA contact to Shivakiran Tummala.

Comment 9

[Johnny Stenback (:jst)]

I'm unable to reproduce this hang any more. WORKSFORME.

Comment 10

[Sivakiran Tummala]

still hangs on linux build 06-20-2001-06.. i clicked on news for 3 times..

here is what top command shows


4109 stummala  11   0 31176  30M 12792 R    96.0 12.1   1:01 mozilla-bin
24153 stummala   3   0  1064 1064   816 R     1.1  0.4   0:00 top
23969 root       0   0 62428  60M  2620 R     0.9 24.2   0:04 X
24050 stummala   0   0  3196 3196  2492 S     0.5  1.2   0:00 magicdev
24137 stummala   0   0  4100 4100  3204 R     0.5  1.5   0:00 gnome-terminal
  682 root       0   0   164  132   112 S     0.1  0.0   6:03 gpm
    1 root       0   0   160  124   100 S     0.0  0.0   0:27 init
    2 root       0   0     0    0     0 SW    0.0  0.0   0:01 kflushd
    3 root       0   0     0    0     0 SW    0.0  0.0   0:46 kupdate
    4 root       0   0     0    0     0 SW    0.0  0.0   0:00 kpiod
    5 root       0   0     0    0     0 SW    0.0  0.0   0:56 kswapd
    6 root     -20 -20     0    0     0 SW<   0.0  0.0   0:00 mdrecoveryd
   61 root       0   0     0    0     0 SW    0.0  0.0   0:00 khubd
  311 root       0   0   376  316   272 S     0.0  0.1   0:05 pump
  361 root       0   0   220  168   136 S     0.0  0.0   0:04 syslogd
  386 rpc        0   0   276  240   204 S     0.0  0.0   0:01 portmap
  402 root       0   0     0    0     0 SW    0.0  0.0   0:00 lockd
~
~
96% of CPU 12.1 mem....

Comment 11

[Sivakiran Tummala]

still hangs on linux build 2001-06-20-06-trunk .., when I go to
http://www.mrshowbiz.com, and I clicked on news for 3 times. Here is what top
command shows


4109 stummala  11   0 31176  30M 12792 R    96.0 12.1   1:01 mozilla-bin
24153 stummala   3   0  1064 1064   816 R     1.1  0.4   0:00 top
23969 root       0   0 62428  60M  2620 R     0.9 24.2   0:04 X
24050 stummala   0   0  3196 3196  2492 S     0.5  1.2   0:00 magicdev
24137 stummala   0   0  4100 4100  3204 R     0.5  1.5   0:00 gnome-terminal
  682 root       0   0   164  132   112 S     0.1  0.0   6:03 gpm
    1 root       0   0   160  124   100 S     0.0  0.0   0:27 init
    2 root       0   0     0    0     0 SW    0.0  0.0   0:01 kflushd
    3 root       0   0     0    0     0 SW    0.0  0.0   0:46 kupdate
    4 root       0   0     0    0     0 SW    0.0  0.0   0:00 kpiod
    5 root       0   0     0    0     0 SW    0.0  0.0   0:56 kswapd
    6 root     -20 -20     0    0     0 SW<   0.0  0.0   0:00 mdrecoveryd
   61 root       0   0     0    0     0 SW    0.0  0.0   0:00 khubd
  311 root       0   0   376  316   272 S     0.0  0.1   0:05 pump
  361 root       0   0   220  168   136 S     0.0  0.0   0:04 syslogd
  386 rpc        0   0   276  240   204 S     0.0  0.0   0:01 portmap
  402 root       0   0     0    0     0 SW    0.0  0.0   0:00 lockd
~
~
96% of CPU is being used by mozilla-bin, and it kept growing.

Comment 12

[John Morrison]

Well, I ran into this hang myself and then found this bug report.  Here's
what's happening.

As jst noted, the hang happens in the synthesized onclick handler for the
link, inside a while loop that does this

    var _ilc = 0;
    while(document.links[_ilc]!=_k) {
        _ilc++;
    } 

where |_k| should be a member of |document.links|. So how could that while
loop hang? Long story ...

First, in the onload handler[1], some JS attaches an onclick handler to every
link on the page. For those links that already have an onclick handler, the
original onclick handler is cached in an array. When the "synthetic" onclick
handler is called, it does it's evil work, and then calls the original onclick
handler, if one exists for that link.

Okay, that's kinda clever. Where this goes wrong is in attaching the synthetic
onclick handler to each link. It does this with the following (simplified)
code:

    var _sf = new Array(document.links.length); // the cached onclick handlers
    for (var i = 0; i < document.links.length; i++) { 
	if (document.links[i].onclick) {
            _sf[i] = document.links[i].onclick;
        }
        eval("document.links[i].onclick = " +
             "  function () {             " +
             "    _ri(this,_sf);          " +
             "  };"
             );
    }

where |_ri()| is defined elsewhere in the file. |_ri()| is where the while
loop happens.

The problem is that inside the eval statement, when |i| is eval-ed, in
Mozilla, it doesn't resolve to the current value of |i| in the for
loop. Instead, it resolves to the value of |window.i|, which has been set
somewhere else in the file/script.

So, assume that |window.i| is 12. On each iteration of the for loop, the eval
will set an onclick handler on the 13th link of the page, instead of setting
an onclick handler for each link in the page.

When the for loop has |i| == 12, the script caches the previously attached
synthetic onclick handler, and attaches a new new synthetic handler. Oops!

When the unlucky user clicks on that link, the function |_ri()| is called with
|_k| being the current link and the while loop will terminate, and |_ri()|
continues to do it stuff. When it has done its stuff, |_ri()| then calls the
cached onclick handler, which then calls back into |_ri()| but now |_k| (i.e.,
|this|) is the array of cached onclick handlers. so, the while loop is entered
again, and since |_k| is not a member of |document.links|, the while loop runs
forever. Q.E.D. :-]


At any rate, the real bug here is a compatibility bug concerning the scope of 
an eval statement, depending on the value of |language| for the <script> block.

In Nav4.x and IE (on win32 at least), an eval inside a for-loop will resolve
|i| to the local value of |i| and not to |window.i| *regardless* of the value 
of the |language| attribute on the <script> block. e.g.,

<html>
<head>
<script>
    var idx = 99;
</script>
<!-- javascript1.1 or javascript1.2 will say '99' in mozilla -->
<script language="javascript1.1">
  function foo() {
      for(var idx = 42; idx < 43; idx++) {
	  eval("alert(idx)");
      }
  }
</script>
</head>
<body onload="foo()">
  This should do an alert with value '42'.
</body>
</html>


So, the fix for this hang is just to fix this incompatibility with Nav4.x 
and IE.

Note: that evil "hitbox.com" script exists in a _lot_ of pages. That means
that this hang is lurking around in a lot of places. If the fix is not 
complicated (and perhaps it is not), then I think we should consider this 
for RTM, both to fix the hang, and also to avoid breaking other scripts
due to this eval incompatibility.


------------------------------
[1] actually, this is not the onload handler for the original document, 
but one that is attached to the document by an external script. It also
will cache any pre-existing onload handlers for the document and will 
call those when the synthetic onload handler is called. 
 

Comment 13

[John Morrison]

Attachment: simple testcase for showing eval scope is incompatible with Nav4.x/IE

Comment 14

[Johnny Stenback (:jst)]

John, thanks a *lot* for digging into this non-trivial-reproduce bug, this looks
like a JS engine bug, and not a DOM bug so over to JS Engine, Brendan, any clues
on this one?

Comment 15

[Johnny Stenback (:jst)]

Unsetting target milestone.

Comment 16

[Brendan Eich [:brendan]]

Not sure when this regressed, but I had something to do with it.  Here's a patch
that worksforme.  Phil, can you test the heck out of it with the js testsuite,
adding any tests as needed?  Rogerl, how about r=; shaver, sr=?

/be

Comment 17

[Brendan Eich [:brendan]]

Attachment: proposed fix

Comment 18

[rogerl (gone)]

1. I don't understand if the first part of the patch is related - it looks like 
you're just cleaning up the conditional?
2. The obj_eval code seems like it contains the necessary JSVERSION_IS_ECMA test 
for error'ing on indirect calls, so testing in the parser is not only redundant 
but messes up the eval scope. r=rogerl

Comment 19

[Mike Shaver (:shaver emeritus)]

sr=shaver.  (Don't suppose there's an assertion you can add to detect this sort
of mixup in the future?)

Comment 20

[Phil Schwartau]

Testcase added to JS test suite:

           js/tests/js1_5/Scope/regress-77578-001.js


This includes jrgm's test:

var i = 99;
test();

function test() {
  for(var i=42; i<43; i++) {
    eval('i');
  }
}


Note this also produces the error:

var i = 99;
test();

function test() {
  var i = 42;
  eval('i');
}


Without the patch applied, we get the erroneous result 99
under JS versions 1.0, 1.1, 1.2;  as jrgm has pointed out.

Comment 21

[Brendan Eich [:brendan]]

rogerl -- I hacked in the nearest trunk-based tree, which had a change from
if(a)if(b)c; to if(a&&b)c; -- sorry, I can take that out, but it matches style
elsewhere for such conditions.

shaver, can you sr=?

/be

Comment 22

[John Morrison]

shaver did give sr=, above.

Comment 23

[Brendan Eich [:brendan]]

Duh, there's shaver's comment.  I don't know what to assert, exactly, but my
mind is occupied by FASTLOAD issues.  For now, I'll check in the patch.  Shaver,
assertion suggestions welcome.

/be

Comment 24

[Brendan Eich [:brendan]]

Fix is in.  Closing, although assertion improvement followups could reopen.

/be

Comment 25

[John Morrison]

Just to confirm, with the correct scoping for the eval, the hang due to 
the hitbox.com infinite loop no longer occurs (tested in a current trunk
build with and without the patch with a local copy of a page where I knew
how to get this hang 100%).

As noted in email, I think this needs to be considered for the Netscape branch.

Comment 26

[selmer (gone)]

PDT+, please check this in on the branch as soon as practical.

Comment 27

[Brendan Eich [:brendan]]

Checked into the MOZILLA_0_9_2_BRANCH.

/be

Comment 28

[Phil Schwartau]

Verified on trunk builds 20010713xx on WinNT, Linux, and Mac:

1. http://www.mrshowbiz.com : I do not hang no matter what I do
2. jrgm's reduced testcase above : alerts '42' as it should

Verified in standalone JS shell: 

3. js/tests/js1_5/Scope/regress-77578-001.js passes on WinNT, Linux, and Mac


As of this writing, no 092-branch builds recent enough to contain the fix
are available on the ftp servers. I will have to wait to verify on the branch -

Comment 29

[Phil Schwartau]

OK, now I have 092-branch builds from 2001-07-14 on WinNT, Linux, and Mac.
These contain the fix and work perfectly, just like the trunk builds do:

1. http://www.mrshowbiz.com : I do not hang no matter what I do
2. jrgm's reduced testcase above : alerts '42' as it should


Marking VERIFIED FIXED on trunk and branch -