I have some strange behavior with my custom iterator object when JSOPTION_JIT is enabled. It is like if my JS_ThrowStopIteration has no more effect in an array comprehension statement.
Created attachment 490830 [details] testcase This testcase hangs on when run in debug mode in msvc9.
TraceRecorder::record_JSOP_MOREITER assumes js_IteratorMore is idempotent, which is unsound as Franck's embedding testcase shows: /* * The interpreter will call js_IteratorMore again, but that's legal. We have to * carefully protect ourselves against reentrancy. */ JSContext *localCx = cx; AutoValueRooter rooter(cx); if (!js_IteratorMore(cx, iterobj, rooter.addr())) RETURN_ERROR_A("error in js_IteratorMore"); Patch anon. /be
Patch tomorrow -- this bug is nasty. js_IteratorMore is not idempotent at all in the case of custom iterators. Not sure why the fix for bug 592069 assumed it was. /be
(In reply to comment #3) > Patch tomorrow -- this bug is nasty. js_IteratorMore is not idempotent at all > in the case of custom iterators. Not sure why the fix for bug 592069 assumed it > was. Sorry, that was vague. The precise problem is that js_IteratorMore is clearly not idempotent when the custom iterator's next method throws StopIteration. It stores the magic JS_NO_ITER_VALUE in cx->iterValue and false as its return value, and succeeds. The interpreter retries on the status exit, calls js_IteratorMore (indirectly) again, which sees the NO_ITER_VALUE magic and therefore invokes then next method, and Franck's next method blindly executes ++count and keeps on trucking. ++ ain't idempotent! We need a sticky JS_STOP_ITER_VALUE or some such, or equivalent state somewhere. /be
Created attachment 490881 [details] [diff] [review] this is ugly, uses DEBUG-only whyMagic Luke: this adds a new JSWhyMagic enumerator, used only by the tracer to restore the lost idempotency between the js_IteratorMore from record_JSOP_MOREITER and the one in the interpreter that follows directly. Any thoughts on a better way? I couldn't see how to do it without using whyMagic, so only this is DEBUG-only. I hacked Franck's test code into the shell, should go in a jsapi-test. /be
Bah, more gross hackage just for this one record_JSOP_MOREITER case! I think the real fix is to un-fuse JSOP_MOREITER. Then record_JSOP_MOREITER can use a normal STATUS_EXIT (we can remove the BUILTIN_NO_FIXUP_NEEDED hack) and everything is chill.
Luke, can you take this one? My mq is overfull. /be
blocking2.0: beta9+ → ?
Assignee: general → lw
Created attachment 491278 [details] [diff] [review] unfuse +11 lines, -40 lines. Still needs a JSAPI test.
Created attachment 491279 [details] [diff] [review] patch Last patch was for a different bug.
Created attachment 491357 [details] [diff] [review] patch The attached test case is a jsapi-test-ification of comment 0. It iloops before the patch and passes after. This also exposed a bug where is_boxed_true was returning a non-condition used as a condition.
Pretend there was also a CHECK(count == 101); after the CHECK(JSVAL_TO_INT(result) == 100);
Attachment #491357 - Flags: review?(dvander) → review+
Andreas, do you have time for this review?
Great to see this fixed. /be
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.