Summary: | Class Proxy can't be extended | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dominic Szablewski <dominic.szablewski> | ||||||||
Component: | JavaScriptCore | Assignee: | GSkachkov <gskachkov> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | buildbot, fpizlo, gskachkov, joepeck, keith_miller, mark.lam, msaboff, rniwa, saam, ysuzuki | ||||||||
Priority: | P2 | ||||||||||
Version: | Safari Technology Preview | ||||||||||
Hardware: | Mac | ||||||||||
OS: | macOS 10.12 | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 171915 | ||||||||||
Attachments: |
|
Description
Dominic Szablewski
2017-03-01 10:00:25 PST
Created attachment 307233 [details]
Patch
Fix with Workaround
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? What does the spec say here? Were we performing the Get() twice? This is observable behavior. 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 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
(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__ ``` 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 ``` 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. 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'll check this code, hopefully today. (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'. 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. 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 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. (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 (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 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 *** (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. (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 Created attachment 311701 [details]
Patch
Patch landed Committed r217655: <http://trac.webkit.org/changeset/217655> |