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)
Core
JavaScript Engine
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)
8.98 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Comment 1•15 years ago
|
||
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
Assignee | ||
Comment 2•15 years ago
|
||
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.
Assignee | ||
Comment 3•15 years ago
|
||
Assignee: general → gal
Attachment #369223 -
Flags: review?(brendan)
Assignee | ||
Comment 5•15 years ago
|
||
Follow-up bug filed to make this case traceable: https://bugzilla.mozilla.org/show_bug.cgi?id=485131
Updated•15 years ago
|
Attachment #369223 -
Flags: review?(brendan) → review+
Comment 6•15 years ago
|
||
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
Assignee | ||
Comment 7•15 years ago
|
||
Comment on attachment 369223 [details] [diff] [review] patch This slows down SS.
Attachment #369223 -
Flags: review-
Assignee | ||
Comment 8•15 years ago
|
||
Attachment #369223 -
Attachment is obsolete: true
Assignee | ||
Comment 9•15 years ago
|
||
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).
Assignee | ||
Comment 10•15 years ago
|
||
Note: fix can go in as P1, but due to the significant perf regression follow-up should be P2.
Comment 11•15 years ago
|
||
Never regress, never surrender! /be
Comment 12•15 years ago
|
||
(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+
Assignee | ||
Comment 13•15 years ago
|
||
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)
Comment 15•15 years ago
|
||
Andreas, do I need to do anything to the wrapper iterator hook (see XPC_XOW_Interator and XPCWrapper::CreateIteratorObj)?
Assignee | ||
Comment 16•15 years ago
|
||
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?
Updated•15 years ago
|
Attachment #369550 -
Flags: review?(brendan) → review-
Comment 17•15 years ago
|
||
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.
Assignee | ||
Comment 18•15 years ago
|
||
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
Assignee | ||
Comment 19•15 years ago
|
||
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)
Updated•15 years ago
|
Attachment #369607 -
Flags: review?(mrbkap) → review+
Assignee | ||
Updated•15 years ago
|
Whiteboard: checkin-needed
Comment 20•15 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/82437f1743a1
Whiteboard: checkin-needed → fixed-in-tracemonkey
Comment 21•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/82437f1743a1
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 22•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/27cd16dfdb0e
Keywords: fixed1.9.1
Comment 23•15 years ago
|
||
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
Keywords: fixed1.9.1 → verified1.9.1
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.2a1
Reporter | ||
Updated•15 years ago
|
Flags: in-testsuite?
Comment 24•11 years ago
|
||
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.
Description
•