Closed Bug 612523 Opened 14 years ago Closed 14 years ago

array comprehension hangs if JSOPTION_JIT is enabled


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
blocking2.0 --- betaN+


(Reporter: soubok, Assigned: luke)



(Whiteboard: fixed-in-tracemonkey)


(2 files, 3 obsolete files)

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.
Attached file 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.

Blocks: 592069
blocking2.0: --- → ?
OS: Windows XP → All
Hardware: x86 → All
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.

(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.

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.

Attachment #490881 - Flags: feedback?(lw)
blocking2.0: ? → beta9+
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.

blocking2.0: beta9+ → ?
You bet.
Assignee: general → lw
Attached patch unfuse (obsolete) — Splinter Review
+11 lines, -40 lines.  Still needs a JSAPI test.
Attached patch patch (obsolete) — Splinter Review
Last patch was for a different bug.
Attachment #490881 - Attachment is obsolete: true
Attachment #491278 - Attachment is obsolete: true
Attachment #490881 - Flags: feedback?(lw)
Attached patch patchSplinter Review
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.
Attachment #491279 - Attachment is obsolete: true
Attachment #491357 - Flags: review?(gal)
Attachment #491357 - Flags: review?(dvander)
Pretend there was also a
  CHECK(count == 101);
after the
  CHECK(JSVAL_TO_INT(result) == 100);
Attachment #491357 - Flags: review?(dvander) → review+
blocking2.0: ? → betaN+
Andreas, do you have time for this review?
Attachment #491357 - Flags: review?(gal) → review+
Great to see this fixed.

Whiteboard: fixed-in-tracemonkey
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.