581486 - TM: "Assertion failure: !IsFunctionObject(v),"

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

x = Proxy.create((function () {
    return {
        iterate: function () {
            return (function () {})
        }
    }
})(), function () {})
for each(let z in [null, new String('q'), new String('q'), x]) {
    for (var c = true in z) {}
}

asserts js debug shell on TM tip with -j at Assertion failure: !IsFunctionObject(v), at ../jstracer.cpp:2824

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   47546:9c869e64ee26
user:        Luke Wagner
date:        Wed Jul 14 23:19:36 2010 -0700
summary:     Bug 549143 - fatvals

Comment 2

[Luke Wagner [:luke]]

It seems that fatvals added a missing assert to NativeToValue which catches a pre-existing bug.  If you add:

  JS_ASSERT(!JSVAL_TO_OBJECT(v)->isFunction());

to the TT_OBJECT case of NativeToValue, this asserts in the cset before fatval.

I think I can debug this though.

Comment 3

[Luke Wagner [:luke]]

The error is that js_ValueToIterator is returning a function-object which screws up the tracer's assumption that this only returns non-function objects.  The fix should be simple, but the question is "where does the check belong?".

Comment 4

[Luke Wagner [:luke]]

gal: if I just mimic existing behavior off trace, then the error naturally occurs in JSOP_MOREITER.  However, this requires guarding on trace that the returned object is or is not a function.  It would be much simpler to have js_ValueToIterator or, if nothing else, ObjectToIterator, detect the error so ObjectToIterator can just fail if a function is returned.  Is that ok or can you think of a way in which this somehow changes behavior.  JSOP_MOREITER follows JSOP_ITER, so I'm guessing 'no'.

Comment 5

[Andreas Gal :gal]

Yes, we should force proxies and __iterator__ to only return objects and throw a type error if not. ValueToIterator should assert that its an object.

Comment 6

[Luke Wagner [:luke]]

Oh wait, this is legal and works without -j:

x = Proxy.create((function () {
    return {
        iterate: function () {
            function f() {}
            f.i = 0;
            f.next = function() {
                       if (this.i++ > 4) throw StopIteration;
                       return this.i
                     };
            return f;
        }
    }
})(), function () {})
for each(let z in [null, new String('q'), new String('q'), x]) {
    for (var c = true in z) {}
}

So ObjectToIterator *can* return a function.

Comment 7

[Brendan Eich [:brendan]]

Luke, thanks for testing -- I was gonna point out the same thing yesterday but wanted to wait in case there was a special case for non-proxied, no-__iterator__ enumeration. We should be guarding on native-vs.-non-native (including proxies) already. Then if native, we'd need something like the old imacro scheme that used a shape guard to ensure there was no __iterator__ customization.

(We'll get rid of __iterator__ in favor of the proxy iterate/enumerate stuff.)

Then we'd know we have a built-in iterator, which definitely returns object not function. Getting this hoisted to JSOP_ITER (before the loop) may be tricky.

/be

Comment 8

[David Mandelin [:dmandelin]]

WFM in bdc3aa93dc26.

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #8)
> WFM in bdc3aa93dc26.

Not sure if this was the changeset that fixed it:

autoBisect shows this is probably related to the following changeset:

The first good revision is:
changeset:   52487:9e7fa574c491
user:        David Anderson
date:        Thu Aug 05 18:04:21 2010 -0700
summary:     [JAEGER] Increase HOTLOOP to 4; blacklist after 300 execs.