285219 - RangeError: reserved slot index out of range

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[timeless]

analysis from brendan:
eval(__s, __o) ... same as __o.eval(__s), more or less
that's the bug
http://lxr.mozilla.org/mozilla/source/extensions/venkman/resources/content/venkman-eval.js#50

Comment 1

[Brendan Eich [:brendan]]

Attachment: proposed fix

Yeah, the push-a-mostly-zeros-compilation-frame code is copied and specialized
in four places.  I don't want to mess with coalescing it now; it hates me, and
I'll probably bust something.

/be

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 176696 [details] [diff] [review]
proposed fix

Oops, that wasn't the patch I wanted to attach.

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: proposed fix

Rediff, then attach, duh.  Anyway, all good here.  Testcase is trivial -- this
should not crash you in the js shell:

var o = {hi: 'there'};
eval("var r = /re(1)(2)(3)/g", o);

/be

Comment 4

[Mike Shaver (:shaver emeritus)]

Comment on attachment 176698 [details] [diff] [review]
proposed fix

Aw, it doesn't hate you, it just has a bit of a temper. r=shaver

Comment 5

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 6

[Bob Clary [:bc] (inactive)]

Brendan, your test case definitely crashes in the js shell, but when I put it in
the test library framework it doesn't crash. ?

Comment 7

[Brendan Eich [:brendan]]

Bob, did you wrap the test in a function?  Any other changes?

/be

Comment 8

[Boris Zbarsky [:bzbarsky]]

Could this have caused bug 285244?

Comment 9

[tabmix.onemen]

(In reply to comment #8)
> Could this have caused bug 285244?
if it mess with file js3250.dll so it seem that this "fix" caused bug 285244
i just replaced today js3250.dll with the same file from 2005-03-07 trunk and
bug 285244 disappear

Comment 10

[Bob Clary [:bc] (inactive)]

(In reply to comment #7)
> Bob, did you wrap the test in a function?  Any other changes?

Nope. I need to revert my build to test this again. More news at 11.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yep, this caused bug 285244.  Tested by locally applying to a build that did not
have the bug.

Comment 12

[Brendan Eich [:brendan]]

Attachment: one-line fix to last patch, should fix the regression

bz is kindly offering to test this.

/be

Comment 13

[Brendan Eich [:brendan]]

The attachment 176789 [details] [diff] [review] cures the bug in the previous patch: JSFRAME_EVAL should
not be propagated from a caller into a compiler frame for a function (body). 
Function bodies are separately scoped and activated scripts, so the
deoptimizations that accompany eval should not afflict them.

Here, the deoptimization from JSOP_SETVAR to JSOP_SETCONST was not accompanied
by the usual logic to set JSFUN_HEAVYWEIGHT, so we end up with a crash in
js_Interpret, case JSOP_SETCONST:, expecting fp->varobj to be non-null when it
wasn't set up as it would have been were the function flagged heavyweight.

Many thanks to bz for testing and debugging.  Selected IRC log:

<bz> aha
<bz> here we go
<bz> bingo
<bz> (gdb) p fp->flags & 0x30
<bz> $2 = 32
<bz> You want the stack here, right?
<brendan> y
<bz> http://web.mit.edu/bzbarsky/www/test.txt
<brendan> frame 26
<brendan> er, 27
<brendan> p *script
<bz> (gdb) p *script
<bz> $3 = {code = 0x84d2c18 ";", length = 525, main = 0x84d2c18 ";", version = 0,
<bz>   numGlobalVars = 0, atomMap = {vector = 0x84d3cc0, length = 59},
<bz>   filename = 0x84ccb7d "chrome://minit_drag/content/tablib.js", lineno = 1,
depth = 7,
<bz>   trynotes = 0x84d2f14, principals = 0x80a7784, object = 0x0}
<brendan> oh, an extension
<brendan> it calls eval
<brendan> there's the missing call
<brendan> ok, thanks
[. . .]
<bz> Looking
* bz is trying to find the file
<bz> note that all steps to reproduce need an extension
<brendan> y
<bz> though apparently a whole bunch trigger this
<brendan> copy/paste, the story of js code creation
<brendan> ;-)
<bz> yeah
<bz> ok, have file open
<bz> one eval call
<bz> if(gBrowser.mTabContainer._appendChild) //flowing tabs
<bz>   eval("miniT_drag.getNewIndex ="+miniT_drag.getNewIndex.toString().replace(
<bz>   'return i;',
<bz>   'if(event.clientY < gBrowser.mTabs[i].boxObject.y +
gBrowser.mTabs[i].boxObject.height)\
<bz>  return i;'));
<brendan> insanity
<brendan> source rewriting and recompiling
<bz> yeah
<brendan> self-modifying code
<bz> uh-huh

/be

Comment 14

[Mike Shaver (:shaver emeritus)]

Comment on attachment 176789 [details] [diff] [review]
one-line fix to last patch, should fix the regression

r=shaver, best one ever.

Comment 15

[Brendan Eich [:brendan]]

Fixed, for real.

/be

Comment 16

[Bob Clary [:bc] (inactive)]

Checking in regress-285219.js;
/cvsroot/mozilla/js/tests/ecma_3/RegExp/regress-285219.js,v  <--  regress-285219.js
initial revision: 1.1

Comment 17

[Bob Clary [:bc] (inactive)]

verified fixed.