array comprehension hangs if JSOPTION_JIT is enabled

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: soubok, Assigned: luke)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Comment 1

8 years ago
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
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.

/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
Attachment #490881 - Flags: feedback?(lw)

Updated

8 years ago
blocking2.0: ? → beta9+
(Assignee)

Comment 6

8 years ago
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)

Comment 8

8 years ago
You bet.
Assignee: general → lw
(Assignee)

Comment 9

8 years ago
Created attachment 491278 [details] [diff] [review]
unfuse

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

Comment 10

8 years ago
Created attachment 491279 [details] [diff] [review]
patch

Last patch was for a different bug.
Attachment #490881 - Attachment is obsolete: true
Attachment #491278 - Attachment is obsolete: true
Attachment #490881 - Flags: feedback?(lw)
(Assignee)

Comment 11

8 years ago
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.
Attachment #491279 - Attachment is obsolete: true
Attachment #491357 - Flags: review?(gal)
Attachment #491357 - Flags: review?(dvander)
(Assignee)

Comment 12

8 years ago
Pretend there was also a
  CHECK(count == 101);
after the
  CHECK(JSVAL_TO_INT(result) == 100);
Attachment #491357 - Flags: review?(dvander) → review+

Updated

8 years ago
blocking2.0: ? → betaN+

Comment 13

8 years ago
Andreas, do you have time for this review?

Updated

8 years ago
Attachment #491357 - Flags: review?(gal) → review+
Great to see this fixed.

/be
(Assignee)

Updated

8 years ago
Whiteboard: fixed-in-tracemonkey
Depends on: 622318
http://hg.mozilla.org/mozilla-central/rev/9acf849c97b4
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.