167658 - iteration through arguments hangs in endless loop

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Georg Maaß]

Mozilla 1.0rc2 and also Mozilla 1.1 (nightly build 20020802008) each on Linux
but not Mozilla 1.1 Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.1)
Gecko/20020826

hang in an endless loop, if I try to do this inside a function:

for(var i in arguments) alert(i + ': '+arguments[i]);

If I try this:

for(var i = 0; i < arguments.length; ++i) alert(i + ': '+arguments[i]);

They work well the first time of calling the function, but the second time this
also loops endless with each arguments[i] being undefined for i > 0.


The test code:
--------------

'Apfelkiste, Apfelschale'.replace('Apfel', function()
{
    for(var i in arguments)alert(i+': '+arguments[i]);
    return 'Bananen';
});


'Apfelkiste, Apfelschale'.replace('Apfel', function()
{
    for(var i = 0; i < arguments.length; i++)alert(i+': '+arguments[i]);
    return 'Bananen';
});

May be that this bug is fixed silent, because it does not occure on Mozilla 1.1
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.1) Gecko/20020826. But I
did not find a bug entry for this. So I post this now to give you a chance to
look inside the code to find out whether it is fixed or does not occure on this
browser for other reasons.

Comment 1

[Phil Schwartau]

Attachment: HTML testcase from reporter

Comment 2

[Phil Schwartau]

I should note that an up-to-date trunk binary 20020910xx on either
WinNT or Linux doesn't hang on the testcase.

Confirming report with older Mozilla trunk builds from August on Linux:

                         20020826xx
                         20020828xx

If I load the HTML testcase with either of these builds, Mozilla hangs
and I have to kill the process to get out.

BUT: if I bring up Tools > Web Development > JavaScript Debugger
before I load the testcase, I do NOT hang, and the testcase runs
fine! Note I am not debugging the testcase; I just have the debugger
window open in the background before I load the testcase in the browser.

That is enough to stop the hang for me in these older builds. 
Georg, does that work for you, too?

Comment 3

[Phil Schwartau]

> I should note that an up-to-date trunk binary 20020910xx on either
> WinNT or Linux doesn't hang on the testcase.

CORRECTION: I AM able to hang on this with up-to-date trunk binary 20020910xx.
Method: load the testcase, reload, reload, reload...eventually I hang.

I have also been able to hang even if the JS Debugger window is open,
so scratch that other remark, too -

Comment 4

[Phil Schwartau]

Reassigning to Kenton; cc'ing Brendan

Note the behavior of the testcase is all over the map for me.

1. Sometimes it runs fine (you get three alertboxes)
2. Sometimes you get an infinite number of alertboxes
3. Sometimes the browser just hangs

Comment 5

[Brendan Eich [:brendan]]

Any luck reproducing this in the js shell (or the xpcshell)?  I don't see it in
my shell testing, nor do I expect it.

Note that per ECMA-262 Edition 3 10.1.8, arguments properties are DontEnum, so
the for..in loop should iterate zero times.  The for (var i=0; i <
arguments.length; i++) loop is the way to iterate over arguments elements.  I
don't like this ECMA misfeature, as it breaks symmetry between arguments and
Array objects, but it's spec-gospel.

/be

Comment 6

[Georg Maaß]

Should we request for an erratum on this spec (262-3 10.1.8) or should we
request for an change of the spec in 262-4?

This actual spec breaks compatiblity with JavaScript prior to JavaScript 1.5.
Because of this, I suggest to change the spec.

Greetings, Georg

Comment 7

[Phil Schwartau]

cc'ing Waldemar for the spec question above -

Comment 8

[Phil Schwartau]

QA Note: 

I have finally been able to reproduce this in the JS shell,
both on Linux and WinNT. I have to use a loop to load the testcase
several times in a row before I hang.  Here is the testcase,
which I've adapted from Georg's:


-----------------------------------------------------------
'Apfelkiste, Apfelschale'.replace('Apfel', function(){
    for(var i in arguments)print(i+': '+arguments[i]);
    return 'Bananen';
});

print('Done');
-----------------------------------------------------------


Then in the JS shell, I load the test in a loop like this:

js> for (var i=0; i<100; i++) {load('167658.js')}

After several iterations, I hang! 

Comment 9

[Phil Schwartau]

Attachment: Linux gdb stack traces from interrrupts of JS hang: testcase #1

Comment 10

[Phil Schwartau]

I can also hang if I put the loop in the testcase itself:

var UBOUND=100;
for (var j=0; j<UBOUND; j++){
  'Apfelkiste, Apfelschale'.replace('Apfel', function(){
      for(var i in arguments)print(i+': '+arguments[i]);
      return 'Bananen';
  });

  print(j);
}


Although in this case, whether I hang or not seems to depend on
strange quirks: e.g. whether I use |UBOUND| in the for-statement
or 100; e.g. whether I include |print(j)| or not... 

At any rate, I can never get the Perl test driver to fail on this,
even when I get the JS shell to fail. So I'll have to verify any
fix for this in the browser, or interactively in the JS shell -

Comment 11

[Phil Schwartau]

Here is the result of Georg's other test. This one involves iterating
through the arguments via |for (var i=0; i<arguments.length; i++)| 
instead of using for-in:

var UBOUND=100;
for (var j=0; j<UBOUND; j++)
{
  'Apfelkiste, Apfelschale'.replace('Apfel', function()
  {
      for(var i = 0; i < arguments.length; i++)print(i+': '+arguments[i]);
      return 'Bananen';
  });

  print(j);
}


-------------------------  OUTPUT IN JS SHELL  -------------------------  
0: Apfel
1: 0
2: Apfelkiste, Apfelschale
0
0: Apfel
1: 0
2: Apfelkiste, Apfelschale
1
0: Apfel
1: 0
2: Apfelkiste, Apfelschale
2
0: Apfel
1: 0
2: Apfelkiste, Apfelschale
3
0: Apfel
1: 0
2: Apfelkiste, Apfelschale
4
0: Apfel
1: 0
2: Apfelkiste, Apfelschale
5
0: Apfel
1: 0
2: Apfelkiste, Apfelschale
6
0: Apfel
1: 0
2: Apfelkiste, Apfelschale
7
0: Apfel
1: 0
2: Apfelkiste, Apfelschale
8
0: Apfel
1: 0
2: Apfelkiste, Apfelschale
9
0: Apfel
1: 0
2: Apfelkiste, Apfelschale
10
0: Apfel
1: 0
2: Apfelkiste, Apfelschale
11
0: Apfel
1: undefined
2: undefined
3: undefined
4: undefined
5: undefined
6: undefined
7: undefined
8: undefined
9: undefined
10: undefined
11: undefined
12: undefined
13: undefined
14: undefined
15: undefined
16: undefined
17: undefined
 
    etc.
    etc.


Notice that |j| got up to 12 before we got "stuck" -

Comment 12

[Phil Schwartau]

Attachment: Linux gdb stack traces from interrrupts of JS hang: testcase #2

Comment 13

[Brendan Eich [:brendan]]

js_NewRegExp fails to initialize almost all of the JSRegExp in the (flat) case,
leading to insanely large parenCount values depending on the uninitialized state
of memory recycled as rdata->base.regexp, in find_replen.  This bug should be
fixed ASAP.

/be

Comment 14

[rogerl (gone)]

I think js_NewRegExp, even in the flat case, exits through the bottom where all
the proper initialization occurs.

I believe this bug occurs because 'match_or_replace' is deleting the regexp
private data before it gets used in 'find_replen'. I'll attach a patch.

Comment 15

[rogerl (gone)]

Attachment: Don't delete regexp private data from GlobData structure.

A little icky since match_or_replace is called from three sites - now each has
to handle the delete. Also the delete is conditional on whether we gen'd up the
regexp from a string argument in the first place - hence a flag added to
GlobData.

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 100179 [details] [diff] [review]
Don't delete regexp private data from GlobData structure.

r=[e-mail] -- thanks, I was ctrl-C'ing and catching backtraces in
gdb, and rushed the diagnosis.

/be

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 100179 [details] [diff] [review]
Don't delete regexp private data from GlobData structure.

Hmm, I take it back -- early returns from str_replace will leak
rdata.base.regexp, and it'd save code to pass a "keep_regexp" flag *in* to
match_or_replace -- then only the one case (str_replace) that sets that flag
would need to duplicate the js_DestroyRegExp call at the bottom of m_or_r.

It seems like match_or_replace sprouted too many args, while still passing a
pointer to a local struct along.  If we can put forceFlat in that struct,
that's one fewer arg reg spill.

Then there's the avoidable js_AddRoot/js_RemoveRoot in str_match, and the
unnecessary array object creation under str_search due to its use of
match_glob.  I'll attempt a patch in a bit.

/be

Comment 18

[Brendan Eich [:brendan]]

Attachment: proposed fix: GlobData.flags and all it entails

Test and review welcome.

/be

Comment 19

[Brendan Eich [:brendan]]

Attachment: oops, muffed str_match's tricky return value in last patch

str_match wants to let js_ExecuteRegExp create the resulting match array, which
it will do if !test.  But in the global regexp case, match_or_replace forces
test to false, because it wants to call js_ExecuteRegExp in a loop, matching
all occurrences.  In that case, it's up to the glob funarg to match_or_replace
to build up a match array to return.  The fix here is to use a local root (an
extra argv element) to protect that match array from the GC.

/be

Comment 20

[Brendan Eich [:brendan]]

Obviously(!), I meant "... forces test to true" -- or really, forces (!test) to
false.  The non-MODE_SEARCH, global regexp case in match_or_replace loops until
mismatch, and what I meant is that this code calls js_ExecuteRegExp in a way
(with a true test parameter) that relieves js_ExecuteRegExp from having to make
a useless match array -- but which requires match_or_replace or its caller or
the glob funarg passed from caller into m_or_r (the last, in fact) to make the
match array that str_match will return on success, overriding whatever
js_ExecuteRegExp left in *rval.

/be

Comment 21

[rogerl (gone)]

Comment on attachment 100211 [details] [diff] [review]
oops, muffed str_match's tricky return value in last patch

One question : at line 1590, after getting a bad return from match_or_replace
we return rather than jumping to 'out', but the bad return doesn't necessarily
mean the regexp wasn't succesfully built, so shouldn't it get deleted?

Comment 22

[rogerl (gone)]

[Oh, and the test suite ran fine.]

Comment 23

[Brendan Eich [:brendan]]

rogerl: if it were possible for match_or_replace to fail but to leave dangling
"out" parameters, that'd be a bug in m_or_r -- but in fact, because the only
return JS_FALSE occurrences there after the non-reobj-owned regexp has been
created are for cases where reobj is non-null (where the regexp wasn't created,
to wit: line 1146), there's no such bug in m_or_r.

/be

Comment 24

[Brendan Eich [:brendan]]

I hope that layering rule in the JS engine is clear: any fallible function is
resposible for cleaning up its out and inout params on error, even if a static
helper like match_or_replace.

Fix checked in (I tweaked the flags comments and order to document in/inout/out
and for aesthetic purity of in-to-out flag order).

/be

Comment 25

[Phil Schwartau]

Marking Verified FIXED.

Interactive testing in the JS shell as in Comment #8 now runs
successfully to completion every time.

Georg, this fix will be available in tomorrow's trunk builds.
Please reopen this bug if you still see the bug there - thanks.

Comment 26

[Georg Maaß]

Will there any build from 2002-09-24 in this directory contain the fix or should
I look for a minimum hour of the build of that date to be sure that the fix is
included?

Comment 27

[Phil Schwartau]

Georg: on my WinNT box, I can see the fix in trunk build 2002-09-24-08.

With this build, I cannot make the HTML testcase hang, no matter how
many times I reload it. Note it is important to get a trunk build,
not a branch build.

By contrast, with a trunk build from yesterday, 2002-09-23-08,
I do hang if I reload the testcase enough times.

Comment 28

[Georg Maaß]

Mozilla 1.2b Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2b) Gecko/20020926

does not hang. So it seams to be well done.

Comment 29

[fireweasel]

(In reply to comment #6)
> Should we request for an erratum on this spec (262-3 10.1.8) or should we
> request for an change of the spec in 262-4?
> 
> This actual spec breaks compatiblity with JavaScript prior to JavaScript 1.5.
> Because of this, I suggest to change the spec.
> 
> Greetings, Georg

Can I ask if this ever happened ? This "bug" just caught me out (in Firefox1.0).
Just because the bug is in the standard, doesn't mean it shouldn't be fixed.

Comment 30

[Georg Maaß]

(In reply to comment #29)
> Can I ask if this ever happened ? This "bug" just caught me out (in Firefox1.0).
> Just because the bug is in the standard, doesn't mean it shouldn't be fixed.

If you mean the malicious spec with »the "bug"«, then I must answer, that 262-4
still does not exist and
http://www.ecma-international.org/publications/standards/Ecma-262.htm does not
contain any link to an errata document. 262-3 it self is prehistoric now.

Comment 31

[Bob Clary [:bc] (inactive)]

Checking in regress-167658.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-167658.js,v  <--  regress-167658.js
initial revision: 1.1