632778 - "Assertion failure: !((attrs ^ shape->attrs) & 0x40) || !(attrs & 0x40),"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

--
function f() {
    "use strict";
}
g = wrap(f);
Object.defineProperty(g, "arguments", {set: function(){}});
--
Asserts in debug builds (interpreter/JM/TM):

Assertion failure: !((attrs ^ shape->attrs) & JSPROP_SHARED) || !(attrs & JSPROP_SHARED), at ../jsscope.cpp:1075

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Stacktrace

Comment 2

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   51090:8acc48c670d5
user:        Jeff Walden
date:        Mon Aug 02 23:52:12 2010 -0700
summary:     Bug 514581 - ES5: fun.caller and fun.arguments must throw when fun is strict-mode code.  r=jimb

Comment 3

[Christian Holler (:decoder)]

Found this as well, I have a test case without "use strict" if that is relevant :)

Comment 4

[Jeff Walden [:Waldo]]

Whether or not it's relevant, please post it.  :-)  If it is relevant, it's more coverage.  If it's not, it's a separate bug we should be sure to fix, although if so perhaps the fix might be better in a new bug.

Comment 5

[Christian Holler (:decoder)]

I totally forgot this bug :) Here is the requested test case (tested on mozilla-inbound revision ec66137aed05 with options -j -m):

o5 = Namespace;
function f1(o) o6 = o5;
function f4(o) {
    _var_ = o
    var prop = Object.getOwnPropertyNames(f4)[4];
    Object.defineProperty(_var_, prop, {
        set: function () {},
    })
}
function f5(o) f1 = o.bind();
(function () {
    f1()
    f5(o6)
    o5 = wrap(f1)
})();
f4(o5)

Comment 6

[Jason Orendorff [:jorendorff]]

Comment 5 reduced:

obj = wrap(Number.bind());
Object.defineProperty(obj, "caller", {set: function () {}});

Comment 7

[Jason Orendorff [:jorendorff]]

I can't take this, but I can see what's happening.

Native objects are implemented in two "layers", a low-level physical layer (mostly in jsscope.cpp) and a sort of user-y API layer (mostly in jsobj.cpp). The former asserts things that the latter is supposed to enforce with a runtime check.

Object.defineProperty on a transparent proxy calls JS_DefinePropertyById on the wrapped object. We end up in js::DefineNativeProperty, here:

        /*
         * If we are defining a getter whose setter was already defined, or
         * vice versa, finish the job via obj->changeProperty, and refresh the
         * property cache line for (obj, id) to map shape.
         */
        if (!js_LookupProperty(cx, obj, id, &pobj, &prop))
            return NULL;
        if (prop && pobj == obj) {
            shape = (const Shape *) prop;
-->         if (shape->isAccessorDescriptor()) {
                shape = obj->changeProperty(cx, shape, attrs,
                                            JSPROP_GETTER | JSPROP_SETTER,
                                            (attrs & JSPROP_GETTER)
                                            ? getter
                                            : shape->getter(),
                                            (attrs & JSPROP_SETTER)
                                            ? setter
                                            : shape->setter());
                if (!shape)
                    return NULL;
            } else {
                shape = NULL;
            }
        }

The attrs of the function.caller property include JSPROP_GETTER and JSPROP_SETTER but not JSPROP_SHARED. So isAccessorDescriptor() returns true, but when we get down to the physical layer in jsscope.cpp, we assert that we aren't trying to turn a slotful property into a slotless one.

It could be fixed by making this code check for more than just isAccessorDescriptor(), I guess. Maybe Jeff's willing to take this?

Comment 8

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

(In reply to Jason Orendorff [:jorendorff] from comment #6)
> Comment 5 reduced:
> 
> obj = wrap(Number.bind());
> Object.defineProperty(obj, "caller", {set: function () {}});

The assertion caused by this testcase as well as the one in comment 0 has mutated to:

Assertion failure: !((attrs ^ shape->attrs) & 0x40) || !(attrs & 0x40),

Comment 9

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

Bug 750307 probably fixed this. The testcases in comment 0 and comment 6 no longer assert.

autoBisect shows this is probably related to the following changeset:

The first good revision is:
changeset:   96576:2ddb6278d1de
user:        Jason Orendorff
date:        Wed Jun 13 03:11:18 2012 -0500
summary:     Bug 750307 - "Assertion failure: isBoolean()" in RegExpObject::ignoreCase after redefining nonconfigurable data property. r=Waldo. Second landing, test change rs=bholley on IRC.

Comment 10

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

Attachment: tests from testcases in comment 0 and comment 6

I'm not sure if tests need r+ but running them through a second pair of eyes may be a good idea...

Comment 11

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

Tests landed:

http://hg.mozilla.org/integration/mozilla-inbound/rev/df2e726ece1f

Comment 12

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

Test backed out, thanks Luke for pointing this out:

http://hg.mozilla.org/integration/mozilla-inbound/rev/627c49f46421

The testcase no longer gives an assert but according to Luke shows a TypeError instead (which I think is correct):

"TypeError: can't redefine non-configurable property 'arguments'"

Comment 13

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

Landing take two:

http://hg.mozilla.org/integration/mozilla-inbound/rev/172ffbd87b1b

In the jit-test directory, I ran:

python jit_test.py <path to js binary> bug632778-1.js bug632778-2.js

and they passed. \o/

Thanks jorendorff for helping me out on IRC.

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/172ffbd87b1b

Comment 15

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

VERIFIED based on landed test.

Comment 16

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

Updated tests to use test metalines instead, since they are in jit-test:

http://hg.mozilla.org/integration/mozilla-inbound/rev/3bca687c261c

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3bca687c261c