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

VERIFIED FIXED in mozilla0.9.3

Status

()

Core
JavaScript Engine
P1
critical
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: David Hyatt, Assigned: brendan)

Tracking

({dom0, hang})

Trunk
mozilla0.9.3
x86
Windows 2000
dom0, hang
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PDT+, URL)

Attachments

(3 attachments)

(Reporter)

Description

17 years ago
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

17 years ago
Worksforme win2k build 2001042513 and linux cvs build from 4/27 8AM
(Reporter)

Comment 2

17 years ago
Current build as of 4/28 still exhibits the problem.  This is not worksforme.
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...
This is really weird, sometimes it hangs, sometimes it doesn't...

Comment 5

17 years ago
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. 
Keywords: hang
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.
Created attachment 32576 [details]
More readable version of the file with the function where we hang.
Severity: normal → critical

Comment 8

17 years ago
Updating QA contact to Shivakiran Tummala.
QA Contact: desale → stummala
I'm unable to reproduce this hang any more. WORKSFORME.
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → WORKSFORME

Comment 10

17 years ago
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....
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---

Comment 11

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

Updated

17 years ago
Target Milestone: --- → mozilla1.0

Comment 12

17 years ago
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. 
 
Keywords: dom0, mozilla0.9.3
Summary: Whole app hangs on www.mrshowbiz.com → Whole app hangs www.mrshowbiz.com (wrong eval scope for JS)

Comment 13

17 years ago
Created attachment 41910 [details]
simple testcase for showing eval scope is incompatible with Nav4.x/IE

Updated

17 years ago
Assignee: jst → rogerl
Status: REOPENED → NEW
Component: DOM Core → Javascript Engine
QA Contact: stummala → pschwartau
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?
Unsetting target milestone.
Target Milestone: mozilla1.0 → ---
(Assignee)

Comment 16

17 years ago
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
(Assignee)

Comment 17

17 years ago
Created attachment 41920 [details] [diff] [review]
proposed fix

Comment 18

17 years ago
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
sr=shaver.  (Don't suppose there's an assertion you can add to detect this sort
of mixup in the future?)

Comment 20

17 years ago
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.
(Assignee)

Comment 21

17 years ago
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
Assignee: rogerl → brendan
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla0.9.3

Comment 22

17 years ago
shaver did give sr=, above.
(Assignee)

Comment 23

17 years ago
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
(Assignee)

Comment 24

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

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED

Comment 25

17 years ago
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

17 years ago
PDT+, please check this in on the branch as soon as practical.
Whiteboard: PDT+
(Assignee)

Comment 27

17 years ago
Checked into the MOZILLA_0_9_2_BRANCH.

/be

Comment 28

17 years ago
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

17 years ago
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 -

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.