465220 - nested imacro abort not cleanly handled (botches assertions)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

Attachment: testcase

The attached test shows this. It works in the optimized js shell:

./Darwin_OPT.OBJ/js -j obad.js
obad.js:4: TypeError: can't convert ({toString:(function ()  > 2 ? this : "false")}) to primitive type

but a debug shell botches an anti-nesting assertion in TraceRecorder::call_imacro and (if that's replaced by runtime coping code) one in js_DecompileValueGenerator (or under it).

/be

Comment 1

[Brendan Eich [:brendan]]

(In reply to comment #0)
> Created an attachment (id=348471) [details]
> testcase
> 
> The attached test shows this. It works in the optimized js shell:
> 
> ./Darwin_OPT.OBJ/js -j obad.js
> obad.js:4: TypeError: can't convert ({toString:(function ()  > 2 ? this :
> "false")}) to primitive type

Yikes! What happened to the left operand of > (namely, i)? Investigating...

/be

Comment 2

[Brendan Eich [:brendan]]

Note also "foo" became "false" -- dead giveaway that fp->imacpc was set and we used the common atom pool instead of the script's pool. That explains the empty string instead of i in the ?: condition.

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: proposed fix

Note that js_DecompileValueGenerator is the only cx->fp->regs->pc-sensitive entry point to the decompiler (AFAIK -- double check me here please).

The jsscript.h fix avoids using the common atom pool when decompiling a function from a script in an imacro (say, one trying to format an error message).

/be

Comment 4

[Brendan Eich [:brendan]]

This is one to watch for b2. We may find conversions requiring imacros that throw in the wild.

/be

Comment 5

[Brendan Eich [:brendan]]

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

Goosing review, this is not something I'm happy shipping b2 without.

/be

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

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

I worry about the use of cx->fp->regs.pc in Decompile (checking if we're decompiling a get op)... r=mrbkap with that looked into

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> (From update of attachment 348478 [details] [diff] [review])
> I worry about the use of cx->fp->regs.pc in Decompile (checking if we're
> decompiling a get op)... r=mrbkap with that looked into

That's why js_DecompileValueGenerator swaps back fp->imacpc (which is in-script) for the imacro pc in fp->regs->pc, and restores after calling DecompileExpression.

Other Decompile callers do not depend on any active frame referring to the script being decompiled.

/be

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

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

a1.9.1b2=beltzner

Comment 9

[Brendan Eich [:brendan]]

Fixed on tm:

http://hg.mozilla.org/tracemonkey/rev/905234b7b9e4

/be

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Fixed on m-c:

http://hg.mozilla.org/mozilla-central/rev/e8ed5d4bf531

Comment 11

[Bob Clary [:bc] (inactive)]

test landed http://hg.mozilla.org/mozilla-central/rev/9c143f56d841 and cvs

Comment 12

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2