Closed Bug 485022 Opened 15 years ago Closed 15 years ago

TM: "Assertion failure: JS_ON_TRACE(cx), at ../jsarray.cpp"

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: gkw, Assigned: gal)

References

Details

(4 keywords, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 3 obsolete files)

x = (0 for each (y in []))
for (var a = 0; a < 4; ++a) { [0 for each (z in x)]}

Assertion failure: JS_ON_TRACE(cx), at ../jsarray.cpp:3227

(debug js shell with -j)

No time yet to autoBisect, but I think this is a regression.
Flags: blocking1.9.1?
autoBisect shows this is probably related to bug 463238 or http://hg.mozilla.org/tracemonkey/rev/1c6be1c210b9 : 

The first bad revision is:
changeset:   26344:1c6be1c210b9
user:        Andreas Gal
date:        Fri Mar 20 18:52:11 2009 -0700
summary:     Support calling arbitrary JSFastNatives from trace (463238, r=brendan).
Blocks: 463238
5019	                    ok = ((JSFastNative) fun->u.n.native)(cx, argc, vp);
(gdb) 
5028	                    regs.sp = vp + 1;
(gdb) 
5029	                    if (!ok)
(gdb) 
6950	    if (fp->imacpc && cx->throwing) {
(gdb) p ok
$15 = 0
(gdb) n
6952	        if (*fp->imacpc == JSOP_NEXTITER) {
(gdb) n
6954	            JS_ASSERT(*regs.pc == JSOP_CALL || *regs.pc == JSOP_DUP);
(gdb) n
6955	            if (js_ValueIsStopIteration(cx->exception)) {
(gdb) n
6956	                cx->throwing = JS_FALSE;
(gdb) n
6957	                cx->exception = JSVAL_VOID;
(gdb) n
6958	                regs.sp[-1] = JSVAL_HOLE;
(gdb) n
6959	                PUSH(JSVAL_FALSE);
(gdb) n
6960	                goto end_imacro;

We call the iterator from trace and it returns with an error which we swallow. FastNativeCallComplete is not invoked, so the code to unpack the return value and check for the error condition is never emitted onto trace, which eventually causes the crash.
Attached patch patch (obsolete) — Splinter Review
Assignee: general → gal
Attachment #369223 - Flags: review?(brendan)
Correctness bug. I support blocking request.
Priority: -- → P1
Blocks: 485131
Follow-up bug filed to make this case traceable: https://bugzilla.mozilla.org/show_bug.cgi?id=485131
Attachment #369223 - Flags: review?(brendan) → review+
Comment on attachment 369223 [details] [diff] [review]
patch

>+#define IN_IMACRO(m) (*cx->fp->imacpc && (uintptr_t(cx->fp->regs->pc - m) < sizeof(m)))

The * before cx->fp->imacpc is wrong.

>@@ -6853,16 +6855,21 @@ TraceRecorder::callNative(JSFunction* fu
>     }
> 
>     if (!(fun->flags & JSFUN_FAST_NATIVE))
>         ABORT_TRACE("untraceable slow native");
> 
>     if (constructing)
>         ABORT_TRACE("untraceable fast native constructor");
> 
>+    // hack for bug 485022 since record_FastNativeCallComplete is not called in the
>+    // exception path

Use "FIXME: bug 485131" at front of comment convention, citing the followup bug.

r=me with these fixes.

/be
Comment on attachment 369223 [details] [diff] [review]
patch

This slows down SS.
Attachment #369223 - Flags: review-
Looking for a volunteer for this bug. Basically we need a specialized variant of FastNativeCallComplete that covers the exception path (collapse the follow-up bug into this one).
Note: fix can go in as P1, but due to the significant perf regression follow-up should be P2.
Never regress, never surrender!

/be
(In reply to comment #10)
> Note: fix can go in as P1, but due to the significant perf regression follow-up
> should be P2.

(In reply to comment #11)
> Never regress, never surrender!

Fix doesn't go in until the perf is fixed. I am not making the rule here, just reiterating.
Flags: blocking1.9.1? → blocking1.9.1+
Attached patch v3 (obsolete) — Splinter Review
Turns out we have only 2 native iterator classes: js_IteratorClass and js_GeneratorClass. The latter we were handling through the getprop/call path instead of the callbuiltin path, which catches the StopIteration exception and turns it into a hole. The simple fix here is to handle js_GeneratorClass through the builtin as well. This is still buggy in case of API-implemented iterators. The DOM doesn't do that. This also never worked right for scripted iterators I think but I haven't been able to come up with a test case. Follow-up bugs might be needed.
Attachment #369226 - Attachment is obsolete: true
Attachment #369550 - Flags: review?(brendan)
Andreas, do I need to do anything to the wrapper iterator hook (see XPC_XOW_Interator and XPCWrapper::CreateIteratorObj)?
Not sure. The idea is to catch the exception and return JSVAL_HOLE in the Iterator/Generator path. Scripted iterators we should currently abort on since they throw an exception which we do not trace atm. Native iterators other than js_IteratorClass/js_GeneratorClass that are STATUS_FAIL would explode. Does XP connect do that?
Attachment #369550 - Flags: review?(brendan) → review-
Comment on attachment 369550 [details] [diff] [review]
v3

XPConnect has a native implementation of the iterator/generator API that I think would also need to be special-cased here.
Attached patch v4Splinter Review
Catch StopIteration in the custom iterator case of JSOP_NEXTITER and transform it into a hole (in the interpreter and on trace). Testcase needed. mrbkap?
Attachment #369550 - Attachment is obsolete: true
Comment on attachment 369607 [details] [diff] [review]
v4

Note: we still abort trace on scripted iterator next implementations since we don't trace throw.
Attachment #369607 - Flags: review?(mrbkap)
Attachment #369607 - Flags: review?(mrbkap) → review+
Whiteboard: checkin-needed
http://hg.mozilla.org/tracemonkey/rev/82437f1743a1
Whiteboard: checkin-needed → fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/82437f1743a1
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Verified fixed with testcase given in comment 0 on trunk and 1.9.1 with the
following debug builds:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre)
Gecko/20090422 Minefield/3.6a1pre ID:20090422224452

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre)
Gecko/20090422 Shiretoko/3.5b4pre ID:20090422122043
Status: RESOLVED → VERIFIED
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.2a1
Flags: in-testsuite?
Bug in removed tracer code, setting in-testsuite- flag.
Flags: in-testsuite? → in-testsuite-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: