161534 2016-09-02 09:46:04 -0700 Regression(r197648): JSObject::setPrototypeWithCycleCheck() allows for cycles but the rest of the code base does not 2016-09-08 13:11:00 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified NEW https://bugs.webkit.org/show_bug.cgi?id=161455 P2 Normal --- 155002 1 cdumez webkit-unassigned ggaren mark.lam msaboff saam oldest_to_newest 1225791 0 cdumez 2016-09-02 09:46:04 -0700 JSObject::setPrototypeWithCycleCheck() allows for cycles but the rest of the code base does not deal properly with cycles. This is because of the following check that was added: if (UNLIKELY(asObject(nextPrototype)->methodTable(vm)->getPrototype != defaultGetPrototype)) break; This was added to match the EcmaScript spec: - https://tc39.github.io/ecma262/#sec-ordinarysetprototypeof (step 8) However, if you create a cycles, we end up with an infinite loop later on under: Structure::anyObjectInChainMayInterceptIndexedAccesses() This is likely not the only place we traverse the prototype chain and except there is no cycle. I noticed this when running html/browsers/history/the-location-interface/allow_prototype_cycle_through_location.sub.html with the patch for Bug 161455 applied. 1225885 1 saam 2016-09-02 12:25:05 -0700 (In reply to comment #0) > JSObject::setPrototypeWithCycleCheck() allows for cycles but the rest of the > code base does not deal properly with cycles. > > This is because of the following check that was added: > if (UNLIKELY(asObject(nextPrototype)->methodTable(vm)->getPrototype != > defaultGetPrototype)) > break; > > This was added to match the EcmaScript spec: > - https://tc39.github.io/ecma262/#sec-ordinarysetprototypeof (step 8) > > However, if you create a cycles, we end up with an infinite loop later on > under: > Structure::anyObjectInChainMayInterceptIndexedAccesses() > > This is likely not the only place we traverse the prototype chain and except > there is no cycle. > > I noticed this when running > html/browsers/history/the-location-interface/ > allow_prototype_cycle_through_location.sub.html with the patch for Bug > 161455 applied. Yeah, we should teach the rest of the engine that it's OK to have cycles in the "getPrototypeDirect" chain. Currently, inside JSC itself, this will never happen. However, I guess we need to teach it this w.r.t the HTML spec. 1225908 2 ggaren 2016-09-02 13:01:39 -0700 > Yeah, we should teach the rest of the engine that it's OK to have cycles > in the "getPrototypeDirect" chain. > Currently, inside JSC itself, this will never happen. However, I guess we > need to > teach it this w.r.t the HTML spec. That's a pretty big change. Are we sure we need to allow cycles? What's the expected behavior if you find a cycle? Is it specified? 1226000 3 cdumez 2016-09-02 15:45:44 -0700 (In reply to comment #2) > > Yeah, we should teach the rest of the engine that it's OK to have cycles > > in the "getPrototypeDirect" chain. > > Currently, inside JSC itself, this will never happen. However, I guess we > > need to > > teach it this w.r.t the HTML spec. > > That's a pretty big change. Are we sure we need to allow cycles? What's the > expected behavior if you find a cycle? Is it specified? Note that even without my patch, it is fairly trivial to end up with an infinite loop since r197648 allows for cycles: <script> var target = { __proto__: null }; var proxy = new Proxy(target, {}); Object.prototype.__proto__ = { __proto__: proxy, }; var a = {}; a.test; </script> This seems to infinite loop for me on ToT, no DOM involved. 1226003 4 saam 2016-09-02 15:52:42 -0700 (In reply to comment #3) > (In reply to comment #2) > > > Yeah, we should teach the rest of the engine that it's OK to have cycles > > > in the "getPrototypeDirect" chain. > > > Currently, inside JSC itself, this will never happen. However, I guess we > > > need to > > > teach it this w.r.t the HTML spec. > > > > That's a pretty big change. Are we sure we need to allow cycles? What's the > > expected behavior if you find a cycle? Is it specified? > > Note that even without my patch, it is fairly trivial to end up with an > infinite loop since r197648 allows for cycles: > > <script> > var target = { > __proto__: null > }; > var proxy = new Proxy(target, {}); > Object.prototype.__proto__ = { > __proto__: proxy, > }; > > var a = {}; > a.test; > </script> > > This seems to infinite loop for me on ToT, no DOM involved. I believe the bug here is having ProxyObject's structure's prototype field be Object.prototype. That's wrong. 1226006 5 cdumez 2016-09-02 15:55:24 -0700 (In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > > Yeah, we should teach the rest of the engine that it's OK to have cycles > > > > in the "getPrototypeDirect" chain. > > > > Currently, inside JSC itself, this will never happen. However, I guess we > > > > need to > > > > teach it this w.r.t the HTML spec. > > > > > > That's a pretty big change. Are we sure we need to allow cycles? What's the > > > expected behavior if you find a cycle? Is it specified? > > > > Note that even without my patch, it is fairly trivial to end up with an > > infinite loop since r197648 allows for cycles: > > > > <script> > > var target = { > > __proto__: null > > }; > > var proxy = new Proxy(target, {}); > > Object.prototype.__proto__ = { > > __proto__: proxy, > > }; > > > > var a = {}; > > a.test; > > </script> > > > > This seems to infinite loop for me on ToT, no DOM involved. > > I believe the bug here is having ProxyObject's structure's prototype field > be Object.prototype. That's wrong. FYI, I tried fixing proxy.__proto__ to be null like so: http://pastebin.com/69HRjvPb But this causes some JSC test failures though: Tools/Scripts/run-jsc JSTests/stress/proxy-set-prototype-of.js Running 1 time(s): DYLD_FRAMEWORK_PATH=/Volumes/Data/WebKit/OpenSource/WebKitBuild/Release /Volumes/Data/WebKit/OpenSource/WebKitBuild/Release/jsc JSTests/stress/proxy-set-prototype-of.js Exception: Error: Bad assertion! assert@JSTests/stress/proxy-set-prototype-of.js:3:24 global code@JSTests/stress/proxy-set-prototype-of.js:31:19 1227726 6 cdumez 2016-09-08 13:11:00 -0700 EcmaScript bug: https://github.com/tc39/ecma262/issues/683