If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

nested imacro abort not cleanly handled (botches assertions)

VERIFIED FIXED in mozilla1.9.1

Status

()

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

People

(Reporter: brendan, Assigned: brendan)

Tracking

({testcase, verified1.9.1})

Trunk
mozilla1.9.1
testcase, verified1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

9 years ago
Created attachment 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

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
Flags: blocking1.9.1?
(Assignee)

Comment 1

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

Comment 2

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

Comment 3

9 years ago
Created attachment 348478 [details] [diff] [review]
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
Attachment #348478 - Flags: review?(mrbkap)
(Assignee)

Comment 4

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

/be
(Assignee)

Updated

9 years ago
Priority: -- → P1

Updated

9 years ago
Flags: blocking1.9.1? → blocking1.9.1+
(Assignee)

Updated

9 years ago
Attachment #348478 - Flags: approval1.9.1b2?
(Assignee)

Comment 5

9 years ago
Comment on attachment 348478 [details] [diff] [review]
proposed fix

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

/be

Updated

9 years ago
Attachment #348478 - Flags: review?(mrbkap) → review+
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
(Assignee)

Comment 7

9 years ago
(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
Attachment #348478 - Flags: approval1.9.1b2? → approval1.9.1b2+
Comment on attachment 348478 [details] [diff] [review]
proposed fix

a1.9.1b2=beltzner
(Assignee)

Comment 9

9 years ago
Fixed on tm:

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

/be
Fixed on m-c:

http://hg.mozilla.org/mozilla-central/rev/e8ed5d4bf531
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Keywords: testcase

Comment 11

9 years ago
test landed http://hg.mozilla.org/mozilla-central/rev/9c143f56d841 and cvs
Flags: in-testsuite+
Flags: in-litmus-
Keywords: fixed1.9.1

Comment 12

9 years ago
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
You need to log in before you can comment on or make changes to this bug.