TM: "Assertion failure: JSVAL_IS_DOUBLE(val), at ../jsopcode.cpp"

RESOLVED FIXED in mozilla1.9.2

Status

()

P2
critical
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: gkw, Assigned: brendan)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Trunk
mozilla1.9.2
assertion, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta1-fixed, status1.9.1 wanted)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

9 years ago
function foo(f) {
    f()
}
foo((eval("\
    (function () {\
        for each(l in [0, 0xB504F332, 0]) {\
            for (d in Error()) {}\
        }\
    })\
")))

asserts js debug shell on TM branch with -j at Assertion failure: JSVAL_IS_DOUBLE(val), at ../jsopcode.cpp:3964. It previously asserted at Assertion failure: (((val) & (((JSUint32)1 << (3)) - 1)) == 0x2), at ../jsopcode.cpp:3959 - no idea when the assertion morphed though.

autoBisect shows it is probably related to bug 487134, where it just began asserting:

The first bad revision is:
changeset:   27516:b8cf788763a0
user:        jorendorff
date:        Tue May 05 14:26:06 2009 -0700
summary:     Record all calls to native functions (487134, r=gal, brendan).
Flags: blocking1.9.2?
(Reporter)

Updated

9 years ago
status1.9.1: --- → ?
(Reporter)

Comment 1

9 years ago
This asserts on 1.9.1 branch too.
Once again, a pre-existing bug revealed by my innocent patch...

GET_ATOM_FROM_BYTECODE behaves strangely when the decompiler tries to decompile the currently-executing function while cx->fp->imacpc is non-null.

The patch below works for *this* test case, but it's probably not correct generally. It's not clear to me which atom-loading macros are supposed to be used in which cases. I need to look at it more carefully before patching.

diff --git a/js/src/jsopcode.h b/js/src/jsopcode.h
--- a/js/src/jsopcode.h
+++ b/js/src/jsopcode.h
@@ -312,7 +312,7 @@ js_GetIndexFromBytecode(JSContext *cx, J
 #define GET_ATOM_FROM_BYTECODE(script, pc, pcoff, atom)                       \
     JS_BEGIN_MACRO                                                            \
         uintN index_ = js_GetIndexFromBytecode(cx, (script), (pc), (pcoff));  \
-        JS_GET_SCRIPT_ATOM((script), index_, atom);                           \
+        (atom) = (script)->getAtom(index_);                                   \
     JS_END_MACRO
 
 #define GET_OBJECT_FROM_BYTECODE(script, pc, pcoff, obj)                      \
diff --git a/js/src/jsscript.h b/js/src/jsscript.h
--- a/js/src/jsscript.h
+++ b/js/src/jsscript.h
@@ -130,6 +130,11 @@ struct JSScript {
 #ifdef CHECK_SCRIPT_OWNER
     JSThread        *owner;     /* for thread-safe life-cycle assertions */
 #endif
+
+    JSAtom * getAtom(size_t index) const {
+        JS_ASSERT(index < atomMap.length);
+        return atomMap.vector[index];
+    }
 };
 
 #define JSSF_NO_SCRIPT_RVAL     0x01    /* no need for result value of last
Depends on: 513119
(Assignee)

Comment 3

9 years ago
JS_GET_SCRIPT_ATOM does try to handle imacros, and succeeds for this bug's testcase when run in the interpreter. Why does it go wrong with -j? Because we call imacros only when recording, duh (to self ;-).

This is easy: imacros do not use common atoms (all of which are string-keyed, none is double-keyed) for ops such as JSOP_DOUBLE. Only JSOP_STRING and other string-atom consumers (property name ops, etc.) should test imacpc and switch to the common atoms if non-null and the script being decompiled matches the one that is running.

/be
(Assignee)

Comment 4

9 years ago
Sorry for the latent bug -- I will try to fix quickly.

/be
Assignee: general → brendan
Status: NEW → ASSIGNED
(Assignee)

Comment 5

9 years ago
Created attachment 398509 [details] [diff] [review]
proposed fix

This is hard to type-check due to multi-typed immediates for JSOP_LOOKUPSWITCH*. Also we cannot use JSOP_DOUBLE in imacros. For now I've injected assertions (some redundant but given this mess that seems better). Suggestions for a better way are welcome, but this does fix the bug.

/be
Attachment #398509 - Flags: review?(jorendorff)
(Assignee)

Updated

9 years ago
Priority: -- → P2
Target Milestone: --- → mozilla1.9.2
(Assignee)

Comment 6

9 years ago
Created attachment 398721 [details] [diff] [review]
fix up decompiler's JSOP_LOOKUPSWITCH* assertion
Attachment #398509 - Attachment is obsolete: true
Attachment #398721 - Flags: review?(jorendorff)
Attachment #398509 - Flags: review?(jorendorff)
Attachment #398721 - Flags: review?(jorendorff)
Comment on attachment 398721 [details] [diff] [review]
fix up decompiler's JSOP_LOOKUPSWITCH* assertion

Here's the test case that breaks this for string atoms.

function f() {
    for (var i=0; i<9; i++)
        assertEq("" + f, expected);
}
var expected = "" + f;
f();
(Assignee)

Comment 8

9 years ago
Created attachment 398771 [details] [diff] [review]
proposed fix, v3
Attachment #398721 - Attachment is obsolete: true
Attachment #398771 - Flags: review?(jorendorff)
>               case JSOP_LOOKUPSWITCH:
>               case JSOP_LOOKUPSWITCHX:
>               {
>+#ifdef DEBUG
>[...]
>+                JSStackFrame *fp = js_GetTopStackFrame(cx);
>+                JS_ASSERT(!fp || !fp->imacpc || fp->script != jp->script);
>+#endif

This seems too strict. We might be decompiling the current function, which contains a switch, and we might be in an imacro too. That would trip this assertion, I think, even though the LOOKUPSWITCH isn't in the imacro.

You can test by adding a LOOKUPSWITCH to function f in comment 7.

>+        if (pc_ && size_t((pc_) - (script_)->code) >= (script_)->length) {  \

Can't tell you how happy I was to hear this doesn't compile if you pass pc_ to the macro. :) Cc-ing Andreas and Shaver.

Don't forget the trace-tests!

r=me with those fixes.
(In reply to comment #9)
> >+        if (pc_ && size_t((pc_) - (script_)->code) >= (script_)->length) {  \
> 
> Can't tell you how happy I was to hear this doesn't compile if you pass pc_ to
> the macro. :) Cc-ing Andreas and Shaver.

"if you pass NULL to the macro." Sorry, got a little carried away.
Attachment #398771 - Flags: review?(jorendorff) → review+
Comment on attachment 398771 [details] [diff] [review]
proposed fix, v3

It'll be nice to get rid of this bug. I have seen bizarre results from js_Disassemble() during debugging, almost certainly due to this.
(Assignee)

Comment 12

9 years ago
http://hg.mozilla.org/tracemonkey/rev/6a7fc22fd61d

/be
Whiteboard: fixed-in-tracemonkey
(Assignee)

Updated

9 years ago
Depends on: 514790
(Assignee)

Updated

9 years ago
Depends on: 514943
http://hg.mozilla.org/mozilla-central/rev/6a7fc22fd61d
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Blocking 1.9.2+.  P2.
Flags: blocking1.9.2? → blocking1.9.2+
(Reporter)

Updated

9 years ago
blocking1.9.1: --- → ?
blocking1.9.1: ? → ---
status1.9.1: ? → wanted
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.