Regression(r197648): JSObject::setPrototypeWithCycleCheck() allows for cycles but the rest of the code base does not

• Component: JavaScriptCore
• Status: NEW ---

[Chris Dumez]

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.

[Saam Barati]

(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.

[Geoffrey Garen]

> 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?

[Chris Dumez]

(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.

[Saam Barati]

(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.

[Chris Dumez]

(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

[Chris Dumez]

EcmaScript bug:
https://github.com/tc39/ecma262/issues/683