609287 - "Assertion failure: OperationInProgress(cx, proxy),"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

f = eval("\
  (function() {\
    __proto__ = \
    Proxy.createFunction((\
    function() {\
      return {\
        has: ArrayBuffer,\
      }\
    })\
    (\"\"), \
    JSON.parse\
    )\
  })\
")()

asserts js debug shell on TM changeset 7ec0a71652a6 without -m or -j at Assertion failure: OperationInProgress(cx, proxy),

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:   56613:9a16d6dfa3c4
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 2

[Andreas Gal :gal]

Attachment: patch

Comment 3

[Jason Orendorff [:jorendorff]]

I think there's still a bug here, because assigning to an inherited read-only data property ought to succeed, shadowing it.

"use strict";
function f(name) {
    return {enumerable: true, value: 3, configurable: true, writable: false };
}
var base = Proxy.create(
    {getOwnPropertyDescriptor: f, getPropertyDescriptor: f});
var obj = Object.create(base);
obj.x = 4;  // should shadow
assertEq(Object.getOwnPropertyDescriptor(obj, "x").value, 4);  // fails


If you replaced that var base with this one:
 var base = Object.create(null, {x: {enumerable: true, value: 3,
                                     configurable: true, writable: false }});
then it passes, as I expect.


I think the right fix for that is just this:
>                 if (pd.attrs & JSPROP_SHARED)
>                     return CallSetter(cx, obj, id, pd.setter, pd.attrs, pd.shortid, vp);
>-
>-                if (pd.attrs & JSPROP_READONLY) {
>-                    if (strict)
>-                        return obj->reportReadOnly(cx, id);
>-                    if (JS_HAS_STRICT_OPTION(cx))
>-                        return obj->reportReadOnly(cx, id, JSREPORT_STRICT | JSREPORT_WARNING);
>-                    return true;
>-                }
>             }

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 497323 [details] [diff] [review]
patch

Comment 3 is not relevant to this assertion, but feel free to check in the fix if you like it...

Comment 5

[Andreas Gal :gal]

Waldo, do you agree with #3? I didn't add it so I want some extra r+ for it.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #3)
> I think there's still a bug here, because assigning to an inherited read-only
> data property ought to succeed, shadowing it.

Really? I see something else:

js> obj = {}
({})
js> Object.defineProperty(obj, 'foo', { configurable: true, writable: false, value: 3 })
({})
js> var next = Object.create(obj)
js> next.foo = 4
4
js> next.foo
3

Comment 7

[Jeff Walden [:Waldo]]

Comment 3 is wrong.  8.12.5 [[Put]] silently returns or throws if [[CanPut]] returns false -- it never shadows.  8.12.4 [[CanPut]] returns desc.[[Writable]] if the relevant descriptor desc is a data descriptor.  So a non-writable descriptor prevents shadowing except by direct definition of a shadowing property.

(In reply to comment #3)
> If you replaced that var base with this one:
>  var base = Object.create(null, {x: {enumerable: true, value: 3,
>                                      configurable: true, writable: false }});
> then it passes, as I expect.

It shouldn't pass -- it should throw since the [[Put]] is called with Throw = true.  Is this a previously-unrecognized bug?

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 497323 [details] [diff] [review]
patch

Looks good as is, with no further additions for comment 3.

Comment 9

[Johnny Stenback (:jst)]

Fixed.

http://hg.mozilla.org/mozilla-central/rev/51573b855c1b

Comment 10

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929