636818 - Crash [@ js::CallJSPropertyOpSetter] or [@ js_SetProperty]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack from crashed js debug shell

a = evalcx('');
a.__proto__ = ''.__proto__;
a.length = 3;

is a testcase which crashes js debug (@ js::CallJSPropertyOpSetter) and opt shells (@ js_SetProperty) on TM changeset f007657da01e without -m nor -j.

Hoping for at least .x+, this is flooding a newly-improved fuzzer, locking s-s to err on the side of caution.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   56817:9ec91c8f9b8e
user:        Blake Kaplan
date:        Fri Oct 29 10:42:35 2010 -0700
summary:     Bug 596031 - 'this' is wrong in getters and setters when a proxy object is on the prototype chain. r=brendan/jorendorff/gal

Comment 1

[Jason Orendorff [:jorendorff]]

Unhiding and taking.

Comment 2

[Jason Orendorff [:jorendorff]]

The first thing js_SetPropertyHelper does is:

    protoIndex = js_LookupPropertyWithFlags(cx, obj, id, cx->resolveFlags,
                                            &pobj, &prop);
    if (protoIndex < 0)
        return JS_FALSE;
    if (prop) {
        if (!pobj->isNative()) {
            if (pobj->isProxy()) {
                AutoPropertyDescriptorRooter pd(cx);
                if (!JSProxy::getPropertyDescriptor(cx, pobj, id, true, &pd))
                    return false;

                if (pd.attrs & JSPROP_SHARED)
                    return CallSetter(cx, obj, id, pd.setter, pd.attrs, pd.shortid, strict, vp);

                if (pd.attrs & JSPROP_READONLY) {
                    if (strict)
                        return obj->reportReadOnly(cx, id);
                    if (cx->hasStrictOption())
                        return obj->reportReadOnly(cx, id, JSREPORT_STRICT | JSREPORT_WARNING);
                    return true;
                }
            }
            ...

In this case, obj is a sandbox and pobj is its prototype, which is a proxy, a cross-compartment wrapper of String.prototype. The property being set is "length", which is both SHARED and READONLY. It is a SHARED data property.

Ideally we would not have such crazy properties, but since we have them, this code needs to cope.

I have a tiny patch, but it needs a bit of testing and bug 636879 is more pressing right now. Will post tomorrow.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 4

[Andreas Gal :gal]

backed out, breaks the build, removing approval

http://hg.mozilla.org/mozilla-central/rev/7536e407b706

Comment 5

[:Gavin Sharp [email: [e-mail]]]

This caused bustage because the landed patch removed security/manager/boot/public/nsISSLStatusProvider.idl (not included in the patch here).

Comment 6

[Jason Orendorff [:jorendorff]]

Sigh. I have no idea how that could possibly have happened. hg rebase craziness, I guess. Sorry.

Comment 7

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/121e1af64e31

Comment 8

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

Original landing: http://hg.mozilla.org/mozilla-central/rev/25027d672f50
Backout: http://hg.mozilla.org/mozilla-central/rev/7536e407b706
Relanding: http://hg.mozilla.org/mozilla-central/rev/121e1af64e31

Comment 9

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/tests/js1_8_5/extensions/regress-636818.js.

Comment 10

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

Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.