612523 - array comprehension hangs if JSOPTION_JIT is enabled

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Franck]

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.

Comment 1

[Franck]

Attachment: testcase

This testcase hangs on when run in debug mode in msvc9.

Comment 2

[Brendan Eich [:brendan]]

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

Comment 3

[Brendan Eich [:brendan]]

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

Comment 4

[Brendan Eich [:brendan]]

(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

Comment 5

[Brendan Eich [:brendan]]

Attachment: 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

Comment 6

[Luke Wagner [:luke]]

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.

Comment 7

[Brendan Eich [:brendan]]

Luke, can you take this one? My mq is overfull.

/be

Comment 8

[Luke Wagner [:luke]]

You bet.

Comment 9

[Luke Wagner [:luke]]

Attachment: unfuse

+11 lines, -40 lines.  Still needs a JSAPI test.

Comment 10

[Luke Wagner [:luke]]

Attachment: patch

Last patch was for a different bug.

Comment 11

[Luke Wagner [:luke]]

Attachment: 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.

Comment 12

[Luke Wagner [:luke]]

Pretend there was also a
  CHECK(count == 101);
after the
  CHECK(JSVAL_TO_INT(result) == 100);

Comment 13

[Robert Sayre]

Andreas, do you have time for this review?

Comment 14

[Brendan Eich [:brendan]]

Great to see this fixed.

/be

Comment 15

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/9acf849c97b4

Comment 16

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/9acf849c97b4