Bug 169040 – Class Proxy can't be extended

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Dominic Szablewski]

A `Proxy` for a `class` can't be extended. When calling `SubClass extends ProxiedClass {}` JSC apparently tries to extend `Proxy` itself(?) instead of the underlying proxied class.

Test case:
http://phoboslab.org/files/bugs/safari-proxy-class.html

Tested in:
- Chrome (56.0.2924.87 (64-bit) macOS): success
- Firefox (51.0.1 (64-bit) macOS): success
- Safari TP (Release 24 (Safari 10.2, WebKit 12604.1.6)): fail

[GSkachkov]

Created attachment 307233 [details]
Patch

Fix with Workaround

[GSkachkov]

I upload the patch, but I think it can be fixed in better way.
```
    class SuperClass {
        constructor() {  }
    }

    let ProxiedSuperClass = new Proxy(SuperClass, {});

    class A extends ProxiedSuperClass {
        constructor() {
            super();
        }
    }
    const a = new A;
```
Issue happens because during initializing A class var ProxiedSuperClass stored to __proto__ variable of the A class, but during invoking super it takes __proto__ from the prototype the class A and it is the same as ProxiedSuperClass.__proto__ but it is not allowed to execute 'new ProxiedSuperClass.__proto__'
To fix issue I stored ProxiedSuperClass to private variable and load it before execute 'super'. 
Could you please suggest if there any better to fix this issue?

[Saam Barati]

What does the spec say here? Were we performing the Get() twice? This is observable behavior.

[Build Bot]

Comment on attachment 307233 [details]
Patch

Attachment 307233 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3547021

New failing tests:
webrtc/multi-video.html

[Build Bot]

Created attachment 307237 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[GSkachkov]

(In reply to Saam Barati from comment #3)
> What does the spec say here? Were we performing the Get() twice? This is
> observable behavior.

Ohh, I did not found something special about this in spec. 
But what I see, that there is unexpected behavior. For ordinary function:
```
function Foo() {}
class Bar extends Foo {}
Bar.__proto__ === Foo; // true
class A {}
class B extends A {}
B.__proto__ === A; // true
```
But for Proxy object:
```
class SuperClass {}
let ProxiedSuperClass = new Proxy(SuperClass, {});
class A extends ProxiedSuperClass {
    constructor() {
	super();
    }
}
A.__proto__ === ProxiedSuperClass; // Expected true, but false; true is A.__proto__ === ProxiedSuperClass.__proto__
```

[Saam Barati]

I suspect this is a bug in ProxyObject, but I'm not 100% sure.
It's weird, because this is the result:

```
class A extends (new Proxy(function(){}, {})) { };
A.__proto__ === Proxy.__proto__ === Function.prototype
```

[Saam Barati]

This function:

```
function foo(proto) {
    class A extends proto { };
    print(A.__proto__ === proto);
    return A.__proto__;
}
```

Produces this byte code:
```
foo#A3U9wp:[0x107d742d0->0x107d97d40, NoneFunctionCall, 186]: 186 m_instructions; 1488 bytes; 3 parameter(s); 16 callee register(s); 6 variable(s); scope at loc3
[   0] enter             
[   1] get_scope         loc3
[   3] mov               loc4, loc3
[   6] mov               loc6, <JSValue()>(const0)
[   9] mov               loc7, <JSValue()>(const0)
[  12] mov               loc8, arg1
[  15] eq_null           loc9, loc8
[  18] jtrue             loc9, 12(->30)
[  21] new_func_exp      loc6, loc3, f0
[  25] mov               loc9, True(const1)
[  28] jmp               9(->37)
[  30] new_func_exp      loc6, loc3, f1
[  34] mov               loc9, False(const2)
[  37] put_by_id         loc6, PrivateSymbol.isDerivedConstructor(@id0), loc9, IsDirect|Bottom
[  46] new_object        loc9, 1
[  50] mov               loc10, Null(const3)
[  53] is_undefined      loc11, loc8
[  56] jtrue             loc11, 15(->71)
[  59] eq_null           loc11, loc8
[  62] jtrue             loc11, 45(->107)
[  65] is_object         loc11, loc8
[  68] jtrue             loc11, 6(->74)
[  71] throw_static_error String (atomic) (identifier): The superclass is not an object., ID: 4(const4), TypeError
[  74] get_by_id         loc10, loc8, prototype(@id1)    predicting None
[  83] is_object_or_null loc11, loc10
[  86] jtrue             loc11, 12(->98)
[  89] is_function       loc11, loc10
[  92] jtrue             loc11, 6(->98)
[  95] throw_static_error String (atomic) (identifier): The value of the superclass's prototype property is not an object., ID: 4(const5), TypeError
[  98] put_by_id         loc6, __proto__(@id2), loc8, Bottom
[ 107] put_by_id         loc9, __proto__(@id2), loc10, Bottom
[ 116] put_by_id         loc6, PrivateSymbol.homeObject(@id3), loc9, Bottom
[ 125] define_data_property loc9, String (atomic) (identifier): constructor, ID: 4(const6), loc6, Int32: 89(const7)
[ 130] define_data_property loc6, String (atomic) (identifier): prototype, ID: 4(const8), loc9, Int32: 74(const9)
[ 135] mov               loc7, loc6
[ 138] resolve_scope     loc10, loc3, print(@id4), <GlobalProperty>, 1, 0x107dd80a0
[ 145] get_from_scope    loc7, loc10, print(@id4), 2048<ThrowIfNotFound|GlobalProperty|NotInitialization>, 123    predicting None
[ 153] get_by_id         loc11, loc6, __proto__(@id2)    predicting None
[ 162] stricteq          loc9, loc11, arg1
[ 166] call              loc7, loc7, 2, 16 (this at loc10) status(Could Take Slow Path)    Original; predicting None
[ 175] get_by_id         loc7, loc6, __proto__(@id2)    predicting None
[ 184] ret               loc7
```

which looks OK to me.

[Saam Barati]

Hmm, I wonder if this is a private name. ProxyObject has this code:


```
static JSValue performProxyGet(ExecState* exec, ProxyObject* proxyObject, JSValue receiver, PropertyName propertyName)
{
    VM& vm = exec->vm();
    auto scope = DECLARE_THROW_SCOPE(vm);
    if (UNLIKELY(!vm.isSafeToRecurseSoft())) {
        throwStackOverflowError(exec, scope);
        return { };
    }

    JSObject* target = proxyObject->target();

    auto performDefaultGet = [&] {
        return target->get(exec, propertyName);
    };

    if (vm.propertyNames->isPrivateName(Identifier::fromUid(&vm, propertyName.uid()))) {
        scope.release();
        return performDefaultGet();
    }
...
```

[GSkachkov]

I'll check this code, hopefully today.

[GSkachkov]

(In reply to Saam Barati from comment #9)
> Hmm, I wonder if this is a private name. ProxyObject has this code:
> 
> 
> ```
> static JSValue performProxyGet(ExecState* exec, ProxyObject* proxyObject,
> JSValue receiver, PropertyName propertyName)
> {
>     VM& vm = exec->vm();
>     auto scope = DECLARE_THROW_SCOPE(vm);
>     if (UNLIKELY(!vm.isSafeToRecurseSoft())) {
>         throwStackOverflowError(exec, scope);
>         return { };
>     }
> 
>     JSObject* target = proxyObject->target();
> 
>     auto performDefaultGet = [&] {
>         return target->get(exec, propertyName);
>     };
> 
>     if (vm.propertyNames->isPrivateName(Identifier::fromUid(&vm,
> propertyName.uid()))) {
>         scope.release();
>         return performDefaultGet();
>     }
> ...
> ```

I did debugging this code. I think issue little bit earlier. It happens in ProxyObject::getOwnPropertySlotCommon method. I've added small list of command that we do during search of __proto__, difference start in 2.2.c.1.1 point: 

Stack for Proxy: 
1 JSValue::getPropertySlot
 2 JSObject::getPropertySlot
  2.0 UNLIKELY(TypeInfo::overridesGetOwnPropertySlot(object->inlineTypeFlags())) - true
  2.1 object->getNonIndexPropertySlot(exec, propertyName, slot);
  2.1.1. LIKELY(!TypeInfo::overridesGetOwnPropertySlot(object->inlineTypeFlags())) -false
  2.1.1.1 JSFunction::getOwnPropertySlot
  2.1.1.1.1 JSObject::getOwnPropertySlot(
  2.1.1.1.1.1 JSObject::getOwnNonIndexPropertySlot(
  2.2.a LIKELY(structure->classInfo()->methodTable.getPrototype == defaultGetPrototype || slot.internalMethodType() == PropertySlot::InternalMethodType::VMInquiry) - true
  2.2.b prototype = structure->storedPrototype();
  2.2.c LIKELY(!TypeInfo::overridesGetOwnPropertySlot(object->inlineTypeFlags())) - false
  2.2.c.1 structure->classInfo()->methodTable.getOwnPropertySlot(object, exec, propertyName, slot)
--2.2.c.1.1 ProxyObject::getOwnPropertySlot(
  2.2.c.1.1.1 thisObject->getOwnPropertySlotCommon ------------------- WE CONTINUE DRILL DOWN -----------------------------------------------------------------------------------
--2.2.c.1.1.1.1 ProxyObject::getOwnPropertySlotCommon(
  2.2.c.1.1.1.1.1 PropertySlot::InternalMethodType::Get 
  2.2.c.1.1.1.1.1.1 ProxyObject::performGet
  2.2.c.1.1.1.1.1.1.1 static JSValue performProxyGet
  2.2.c.1.1.1.1.1.1.1.1 inline JSValue JSObject::get 
  2.2.c.1.1.1.1.1.1.1.1 JSObject::getPropertySlot ---- WE START LOOP OVER PARENT PROPERTIES ------------------------------------------------------------------------------------

Stack for Class: 
1 JSValue.getPropertySlot
2 Object->getPropertySlot
  2.0 UNLIKELY(TypeInfo::overridesGetOwnPropertySlot(object->inlineTypeFlags())) - true
  2.1 object->getNonIndexPropertySlot
  2.1.1 (LIKELY(!TypeInfo::overridesGetOwnPropertySlot(object->inlineTypeFlags()))) 
  2.1.1.1 JSFunction::getOwnPropertySlot
  2.1.1.1.1.  JSObject::getOwnPropertySlot(
  2.1.1.1.1.1 JSObject::getOwnNonIndexPropertySlot(.  
  2.2.a LIKELY(structure->classInfo()->methodTable.getPrototype == defaultGetPrototype || slot.internalMethodType() == PropertySlot::InternalMethodType::VMInquiry)
  2.2.b prototype = structure->storedPrototype();
  2.2.c LIKELY(!TypeInfo::overridesGetOwnPropertySlot(object->inlineTypeFlags())) - false
  2.2.c.1 structure->classInfo()->methodTable.getOwnPropertySlot(object, exec, propertyName, slot)
--2.2.c.1.1 JSObject::getOwnPropertySlot
--2.2.c.1.1.1 JSObject::getOwnNonIndexPropertySlot ------------------ WE EXIT THERE ------------------------------------------------------------------------------------------
  2.2.d. LIKELY(structure->classInfo()->methodTable.getPrototype == defaultGetPrototype || slot.internalMethodType() == PropertySlot::InternalMethodType::VMInquiry)
  2.2.e. prototype = structure->storedPrototype();
  2.2.f LIKELY(!TypeInfo::overridesGetOwnPropertySlot(object->inlineTypeFlags())) - true
  2.2.f.1 JSObject::getOwnNonIndexPropertySlot
  2.2.c.1.1.1.1.1.1.1.1.1 JSObject::getPropertySlot

In case of class we do two steps 2.2.c.1.1 and 2.2.c.1.1.1 and than back to loop over parents, but in case of Proxy we move deeper. I think the issue within ProxyObject::getOwnPropertySlotCommon method because we move to this function from structure->classInfo()->methodTable.getOwnPropertySlot(it overrides) but we expect to find in `OWN PROPERTIES`, but start we look to all properties, and start loop over parent of the Proxy. In current implementation we always navigate through all prorates of Proxy 'own' and 'parents'.

[Saam Barati]

Ok, I looked into this a bit more, and this function looks totally wrong to me:
```
static RegisterID* emitGetSuperFunctionForConstruct(BytecodeGenerator& generator)
{
    if (generator.isDerivedConstructorContext())
        return generator.emitGetById(generator.newTemporary(), generator.emitLoadDerivedConstructorFromArrowFunctionLexicalEnvironment(), generator.propertyNames().underscoreProto);

    RegisterID callee;
    callee.setIndex(CallFrameSlot::callee);
    return generator.emitGetById(generator.newTemporary(), &callee, generator.propertyNames().underscoreProto);
}
```

This is performing a get("__proto__"). FWIW, chrome does not do this. I think we're just happily assuming that "__proto__" gets us what we want, but in this case, we have a prototype chain like:

class A extends proxy {};
Internally in JSC, A's structure will have "proxy" as its prototype. Performing Get(A, "__proto__"), will cause us to perform get on the Proxy though. This is probably not the intention of the spec. We should see what it says for super. Perhaps this is some internal property.

e.g, check out this program:
```
if (typeof window !== "undefined")
    print = console.log;

function foo(p) {
    class A extends p { };
    return A;
}

let target = class B{};
let proxy = new Proxy(target, {
    get(t, property, receiver) {
        print("-----------------\nIn get:")
        print("t is target: ", t === target);
        print("prop: ", property);
        print("receiver is proxy: ", receiver === proxy);
        return Reflect.get(t, property, receiver);
    }
});

let c = foo(proxy);
print("c__proto__ === proxy:", c.__proto__ === proxy);
new c;
```

In JSC shell, we have this output:
```
-----------------
In get:
t is target:  true
prop:  prototype
receiver is proxy:  true
-----------------
In get:
t is target:  true
prop:  __proto__
receiver is proxy:  false
c__proto__ === proxy: true
-----------------
In get:
t is target:  true
prop:  __proto__
receiver is proxy:  false
```

In Chrome canary, there is this output:
```
-----------------
In get:
t is target:  true
prop:  prototype
receiver is proxy:  true
-----------------
In get:
t is target:  true
prop:  __proto__
receiver is proxy:  false
c__proto__ === proxy: true
```

I'll assume for the sake of argument that Chrome has the correct behavior w.r.t the spec. That means that we're performing an extra get when we shouldn't, and because this get has a Proxy in the lookup chain, we do the wrong thing. So I think super(...) should not be performing this get is my guess. We need to read the spec to verify.

[Saam Barati]

The spec performs GetPrototypeOf, which in this case, should probably just return Structure's prototype field, since I think it's provable that the function will not have overridden the MethodTable getPrototypeOf function.

See spec:
https://tc39.github.io/ecma262/#sec-super-keyword (super call)
and
https://tc39.github.io/ecma262/#sec-getsuperconstructor

I believe our bug is in assuming a false equivalency between Get("__proto__") and GetPrototypeOf

[GSkachkov]

Ok. I see what you mean. I'll examine spec and check Get/GetPropertyOf

Also I found interesting behavior of proxy:
```
var target = {};
proxy = new Proxy(target, {
    getPrototypeOf(t) { console.log("get prototype:", t); return t.__proto__; } 
});
proxy.__proto__ 
``` 
In Chrome & FireFox it prints:
// get prototype: ....
// Object ...
In jsc we do not print `get prototype: ....`
Is this separate issue or it should be fixed there.

[Saam Barati]

(In reply to GSkachkov from comment #14)
> Ok. I see what you mean. I'll examine spec and check Get/GetPropertyOf
> 
> Also I found interesting behavior of proxy:
> ```
> var target = {};
> proxy = new Proxy(target, {
>     getPrototypeOf(t) { console.log("get prototype:", t); return
> t.__proto__; } 
> });
> proxy.__proto__ 
> ``` 
> In Chrome & FireFox it prints:
> // get prototype: ....
> // Object ...
What's printing Object ... ?
> In jsc we do not print `get prototype: ....`
> Is this separate issue or it should be fixed there.

Sounds like a bug. It also sounds like a separate issue, but I'm not 100% sure. Perhaps this is needed

[GSkachkov]

(In reply to Saam Barati from comment #15)
> (In reply to GSkachkov from comment #14)
> > Ok. I see what you mean. I'll examine spec and check Get/GetPropertyOf
> > 
> > Also I found interesting behavior of proxy:
> > ```
> > var target = {};
> > proxy = new Proxy(target, {
> >     getPrototypeOf(t) { console.log("get prototype:", t); return
> > t.__proto__; } 
> > });
> > proxy.__proto__ 
> > ``` 
> > In Chrome & FireFox it prints:
> > // get prototype: ....
> > // Object ...
> What's printing Object ... ?
proxy.__proto__ === target.__proto__ // true. Correct for all browsers because we return t.__proto__, but if we change to another value we would have different results
> > In jsc we do not print `get prototype: ....`
> > Is this separate issue or it should be fixed there.
> 
> Sounds like a bug. It also sounds like a separate issue, but I'm not 100%
> sure. Perhaps this is needed
Ok. I'll create issue

[GSkachkov]

Can't reproduce issue. It seems that it was fixed by https://bugs.webkit.org/show_bug.cgi?id=164849

*** This bug has been marked as a duplicate of bug 164849 ***

[Saam Barati]

(In reply to GSkachkov from comment #17)
> Can't reproduce issue. It seems that it was fixed by
> https://bugs.webkit.org/show_bug.cgi?id=164849
> 
> *** This bug has been marked as a duplicate of bug 164849 ***

Can you check in a test for this?

rs=me to do this.

[GSkachkov]

(In reply to Saam Barati from comment #18)
> (In reply to GSkachkov from comment #17)
> > Can't reproduce issue. It seems that it was fixed by
> > https://bugs.webkit.org/show_bug.cgi?id=164849
> > 
> > *** This bug has been marked as a duplicate of bug 164849 ***
> 
> Can you check in a test for this?
> 
> rs=me to do this.

Yes, sure. I'll upload new patch with tests

[GSkachkov]

Created attachment 311701 [details]
Patch

[GSkachkov]

Patch landed 
Committed r217655: <http://trac.webkit.org/changeset/217655>