OrdinarySetPrototypeOf and the HTML spec

[saambarati]

Currently, OrdinarySetPrototypeOf bails from its cycle checking loop early if it sees a [[GetPrototypeOf]] which isn't the default implementation. I'm guessing this is in awareness of the Proxy's [[GetPrototypeOf]]. Is that correct? If not, what is the reasoning behind this decision?
I'm looking at: https://tc39.github.io/ecma262/#sec-ordinarysetprototypeof

The HTML spec overrides [[GetPrototypeOf]] on the window proxy. Calling [[SetPrototypeOf]] on an object who's [[Prototype]] is WindowProxy can lead to cycles now.
I'm looking at: https://html.spec.whatwg.org/#windowproxy-getprototypeof

Like so:

let o = {__proto__: window};
window.__proto__.__proto__.__proto__.__proto__ = o

(I believe this throws cycle exceptions in browsers, however, it should not according to the spec, if I'm reading it correctly, and have constructed my example correctly.)

I believe that if we don't consider the browser, it's impossible to get a cycle if you directly loop over the [[Prototype]] property. However, with the HTML spec, I believe that's no longer true.

I wonder if the function can be more restrictive, and only bail out on the loop if we encounter a ProxyObject.[[GetPrototypeOf]] internal method, instead of bailing once we see the non-default [[GetPrototypeOf]].

What are people's thoughts? I'm mostly posting here to bring awareness to this issue and to understand previous discussions of it, or to have new discussions about it. We're currently running into issues with this property inside WebKit.

[domenic]

I think @jswalden also ran into this, judging by https://blog.whatwg.org/windowproxy-window-and-location#comments ...

(I have not taken the time to form an opinion on this. In general I think anything that is web-compatible is probably fine in these kind of edge cases?)

[littledan]

Cc @verwaest

[allenwb]

see https://bugs.ecmascript.org/show_bug.cgi?id=2437 for the background of this semantics

It seems perfectly reasonable for the HTML spec to provide modifications to steps of 8.c.i-ii of OrdinarySetPrototypeOf that take the WindowProxy object into account

[bterlson]

The rationale wasn't entirely proxy-based - generally, exotic objects can have a [[GetPrototypeOf]] implementation that can do arbitrary things and it's impossible to enforce that there are no observable circularities. Proxies are an obvious case where this can happen however.

That said, it totally makes sense that an embedding would want exotic objects with overridden [[GetPrototypeOf]] slots to nonetheless behave as if [[GetPrototypeOf]] were ordinary for the purposes of circularity checks. I can't think of a clean way to expose this through 262's layering API however. Another alternative is for HTML to ammend OrdinarySetPrototypeOf Step 8.c.i to state something like "If the [[GetPrototypeOf]] internal method of p is not the ordinary object internal method defined in 9.1.1 and not WindowProxy's [[GetPrototypeOf]] internal method, let done be true." Since this is so edge-casey I don't think this is too bad.

[domenic]

I still haven't taken the time to understand this issue in full, so I'm just assuming that you guys are right that this is the best solution :). But I would really prefer not to have that kind of monkeypatch and willful violation. Instead could we define something like a "non-circular exotic [[GetPrototypeOf]] internal method", change ES to have "is not the ordinary object internal method defined in 9.1.1 and is not a non-circular exotic [[GetPrototypeOf]] internal method", and then say in HTML that the [[GetPrototypeOf]] for WindowProxy and Location are non-circular exotic [[GetPrototypeOf]] internal methods?

[bterlson]

@domenic that would work too, but it's a lot of verbage to support this edge case. We also don't have a notion of special "kinds" for internal methods in the spec and I'd not like to add it just for this. It's a lot of conceptual overhead.

[domenic]

Then could we add a reference directly to WindowProxy's [[GetPrototypeOf]] to the ES spec?

[allenwb]

@domenic

Then could we add a reference directly to WindowProxy's [[GetPrototypeOf]] to the ES spec?

That would give special status to HTML. Whose to say that some other host environment spec. might not want to do something similar. It seems cleaner to me, to leave to such host environment specification to say what they require (and to negotiate with engines to support it).

[domenic]

Yes, that's exactly the negotation we're performing here :). I'd just like to get it written into the spec. As you say, other host environments might have the same requirement, so it'd be good if the spec had a mechanism for doing so.

[cdumez]

FYI, this is not just HTML's WindowProxy that is affected but also HTML's Location object.

[domenic]

We also don't have a notion of special "kinds" for internal methods in the spec and I'd not like to add it just for this.

We could also define a special kind of exotic object (the precedent being immutable prototype exotic objects) and test if p is one of those. E.g. a "circular-prototype-checked exotic object"

[allenwb]

I'd just like to get it written into the spec. As you say, other host environments might have the same requirement, so it'd be good if the spec had a mechanism for doing so.

My point was more that other environments might have different requirements or requirements relating to other parts of the ES spec. Trying to accommodate such implementation specific requirements adds spec. complexity that impacts everybody. (arguably, this current issue relates to existing complexity that was added as an implementor accommodation). In addition, such accommodations can be seen as sanctioning such extensions, when we might prefer to discourage them.

Finally, look at it from an architectural layering perspective. Many environments can be layered on top of ES. Architecturally "down links" rather than "up links" are preferable and more maintainable.

[domenic]

I just don't think it's reasonable to say that for something as basic as OrdinaryGetPrototypeOf, web browsers cannot look at the ES spec, and must instead implement the HTML spec's version. If we set that kind of precedent, then the ES spec becomes entirely unreliable as a guide to what implementations should implement and the behaviors and invariants developers can assume. @erights has commented on this in the past in other issues.

Instead, I'd like to use the tried-and-true practice of decoupling dependencies via agnostic, targeted hooks of the sort described.

If we want to be even more generic, to accomodate those varying options you describe, then I guess the canonical path is to create a HostOrdinarySetPrototypeOfSteps(O, V) operation, and insert it after step 8 of OrdinarySetPrototypeOf. That seems like over-engineering to me compared to adding a special kind of exotic object, or similar. But if that's what the editor would prefer I'm happy to do that too.

[bterlson]

IIUC circular-prototype-checked exotic object isn't right - the ability to depend on GetPrototypeOf for circularity checks is orthogonal to the exoticness of an object. In other words, a named exotic object is just an object with a specific set of non-standard slots so a circular-prototype-checked exotic object wouldn't work because it actually describes a constraint on the behavior of [[GetPrototypeOf]] rather than a particular slot value.

Another option would be a slot flag for ordinary objects, something like [[GetPrototypeOfReliableForCircularityChecks]]. Its value is true for ordinary objects and false for proxy objects. Problem I see with this design is that it assumes that objects that override [[GetPrototypeOf]] are reliable when probably assuming the opposite is safest.

I still feel that any option is a lot of machinery for an edge case with a pretty tiny spec patch :-P It makes 262 harder to understand and doesn't help HTML all that much.

[domenic]

I still feel that any option is a lot of machinery for an edge case with a pretty tiny spec patch :-P It makes 262 harder to understand and doesn't help HTML all that much.

I am really having trouble understanding this perspective. Similarly, I find it a bit baffling that you are calling this an edge case, given how we have multiple implementers who have complained about this. Sure, from a web developer's point of view it probably won't come up much, but we've heard twice that this complicates engines significantly.

262 could become very easy to understand if we just replaced OrdinarySetPrototypeOf with the text "set the prototype please". That wouldn't make it accurate, or reflect what implementations implement. I think the goal should not be ease of understanding, but ease of understanding given that we are speccing what engines implement.

It helps HTML a lot; getting the exotic behaviors of WindowProxy and Location correct is a huge win for the web platform, as they have historically been underspecified and a source of security issues. We wrote a blog post about this at https://blog.whatwg.org/windowproxy-window-and-location. Making sure specs actually align with the implementations here has been a big project, and to have it stall on the finish line because ES isn't willing to open up and do its part is quite frustrating.

I'm begging you to give me a hook here. I don't care whether it's special [[SetPrototypeOf]] internal methods, special object types, special internal slots, or HostIsGetPrototypeOfIsReliableForCircularityChecks. I will do the work and PR anything you agree to. Let's just actually get this working, and not dismiss the problem and encourage more willful violations of ES from HTML.

[allenwb]

@bterlson
Note that the check for the ordinary [[GetPrototypeOf]] in step 8.c.i is essentially a guard to ensure that the object has a [[Prototype]] internal slot that contains the value that [[GetPrototypeOf]] returns when called. That's what implementations actually care about, that following such prototype chains (skipping actually calling [[GetPrototypeOf]] will never produce a circularity.

[bterlson]

@domenic given all of the changes that have landed in 262 for layering purposes I don't think it's right to claim that we're not opening up or doing our part.

[domenic]

@bterlson certainly I didn't mean to imply that! I was talking about this specific instance. Part of the reason it's so surprising to see these kinds of attitudes is precisely because of the past collaboration.

[bterlson]

We're collaborating - I've even made a proposal! My feelings on the matter should not be interpreted as a "No way" - I often don't get my way :) Anyway, let's not make this into a political issue and focus on finding the best way to spec this in 262. We can worry about politics if we think it needs-consensus :-P

[domenic]

OK, sorry about that! I guess I was just frustrated by some specific wording around "edge case" and "doesn't help" and so on. Apologies for letting that spill out. Let's indeed continue to work together and move past my outburst :)

I am liking the direction of some sort of "reliable for circularity checks" flag. I think it's probably safe to assume that exotic objects defined in specs will be reliable in that way, and so set the default to reliable for non-proxies, but I can understand being cautious instead. The HostIsGetPrototypeOfIsReliableForCircularityChecks idea I alluded to above might be a way of flipping that default, by making the default behavior be to return false.

[saambarati]

@allenwb
What about making ProxyObject start with [[Prototype]] of null (note, the only place a ProxyObject's [[Prototype]] will be checked is in this loop). Then, the loop can loop over [[Prototype]] instead of checking the value of [[GetPrototypeOf]].
Thoughts?

[allenwb]

@saambarati
ProxyObjects don't have a [[Prototype]] internal slot and more generally exotic objects may or may not have such a slot.
But regardless, the WindowProxy apparently isn't an actual ProxyObject.

[allenwb]

@domenic
exposing the ordinary Object [[GetPrototypeOf]] and a [[Prototype]] internal property is essentially the tag that means "reliable for circularity checks". So instead of over-riding [[GetPrototypeOf]] why not use the ordinary Object [[GetPrototypeOf]] plus a [[Prototype]] slot and when you set up a WindowProxy, cache the value currently computed by https://html.spec.whatwg.org/#windowproxy-getprototypeof in the ProxyObject's [[Prototype]].

[claudepache]

Interestingly, in Firefox, even without the cycle check, it is impossible to have a prototype chain cycle involving window or any object in its prototype chain, because the entire prototype chain of window is locked down https://bugzilla.mozilla.org/show_bug.cgi?id=1052139.

There is still the issue of the Location objects; although I wonder whether it makes sense to have a mutable prototype chain on such objects anyway.

[saambarati]

@allenwb
I understand that ProxyObject does not have a [[Prototype]] slot. I'm proposing that we make it null (and other objects in the future that may fall into the same category). I know WindowProxy is unrelated to ProxyObject, however, if we only consider the TC39 spec, ProxyObject is the only type of object that overrides [[GetPrototypeOf]], that's what I meant by having that loop be over [[Prototype]] instead of checking the [[GetPrototypeOf]] method.

[jswalden]

FWIW. There are only two classes of objects with overridden [[GetPrototypeOf]] in HTML: every global object that can load scripts, and window.location. Because of https://bugzilla.mozilla.org/show_bug.cgi?id=1052139 and #308 noted earlier, [[Prototype]] mutation on any object that might form a cycle through a global will fail, so no cycle there. Only window.location can participate in cycles.

Functionally, internally I turned the "the [[GetPrototypeOf]] internal method of p is not the ordinary object internal method defined in 9.1.1" check into a new proxy trap. (This new trap was already required to implement this language with respect to Proxy objects. HTML's overridden-[[Prototype]] objects just defined fresh versions of that existing trap.) This is as easy as adding any new trap to SpiderMonkey -- easier, even, because existing [[GetPrototypeOf]] trap definitions pointed out all the places to change (not always true of new traps). Some definitions of the trap are tricky because DOM window objects are represented in a funky way in the presence of the hated document.domain -- but not super-tricky. If "we've heard twice that this complicates engines significantly" is meant to include SpiderMonkey/Gecko, I think that overstates it. The extra engineering time wasn't "significant".

I have no love for [[Prototype]] cycles through window.location. The wry WHATWG blog comment was more for entertainment and amusement at a bizarre quirk, primarily for myself but also for the few others so deep in the weeds, or willing to go there to understand a joke. It was not a claim that window.location cycles were difficult to implement or significantly complicated SpiderMonkey.

Short of additionally making the [[Prototype]] of Location.prototype immutable (it's the only object in window.location's chain with a mutable [[Prototype]]), I see no fix that would require no complexifying MOP alterations. [[Prototype]] mutation is generally stupid, this case especially so, so that change would be fine with me, if we wanted to avoid cyclic [[Prototype]] chains in this one weird case. But I'm quite unbothered if things stayed as-is.

[domenic]

@jswalden

There are only two classes of objects with overridden [[GetPrototypeOf]] in HTML: every global object that can load scripts, and window.location

Can you explain why the former is "every global object that can load scripts", and not just WindowProxy?

However, maybe it is moot, because since the global object prototype chains are immutable now, they cannot be involved in cycles. So Location is the only problem.

[jswalden]

@domenic Actually, it probably is just WindowProxy -- looking back now, I think I misspoke. But yes, that particular nuance is moot because non-WindowProxy global objects have immutable prototypes and so can't be in cycles.

[littledan]

@jswalden I think it's just Location.prototype that would be affected, actually. That's not an immutable prototype exotic object. By contrast, any mutation to a WindowProxy's __proto__ needs to fail, even if it doesn't form a cycle.

[jswalden]

Yes. I think we're both on the same page, just that there's a bit of pronoun trouble going on, and separate references to distinct bits of oddity.

[ljharb]

This is both a long and old thread; could someone clarify if there's still something that needs to be done here?

[annevk]

@ljharb basically, what should the behavior for

o = {__proto__: self.location};
self.location.__proto__ = o

be, I think, and does that require changes to ECMAScript? It seems that in Firefox and Safari self.location.__proto__ is immutable these days, whereas Chrome does some kind of cycle detection.

[ljharb]

Since the spec now has language for "immutable prototype exotic objects", could location be specified as one of those in HTML?

[annevk]

Ah, I suspect so. Would be great if @jswalden could confirm that's indeed what we've done for Firefox (and perhaps @saambarati for Safari given they raised this), as I'm not entirely confident in my testing.

[annevk]

Created a PR for IDL assuming I'm correct: whatwg/webidl#606.

[annevk]

Update at whatwg/webidl#606 (comment). I was wrong, the problematic code is Location.prototype.__proto__ = { __proto__: location} which throws in Chrome/Safari (that a cycle was detected) and makes the current tab non-functioning in Firefox.

So we're back to 1) implementers willing to change Location.prototype to be immutable as per my PR 2) providing a hook in ECMAScript for cycle detection and throwing and making Location.prototype use that (same approach as document.all seems reasonable).

[annevk]

I chatted with @jswalden once more and at least Firefox doesn't consider the current state of things to be a problem here. Web-platform-tests already covers this in html/browsers/history/the-location-interface/allow_prototype_cycle_through_location.sub.html which Safari and Chrome currently fail (due to detecting cycles for Location).

@saambarati @syg is this something that can be fixed in Safari and Chrome, respectively? I.e., allow cycles for Location.prototype.

[annevk]

@saambarati @syg ping.

[shvaikalesh]

@annevk

Some background on WebKit issue: we have a few helpers that determine if a fast path can be taken. As an example, for obj.foo = 1 we first traverse the prototype chain and check parents' structure flags to ensure there is no [[Prototype]] with non-ordinary internal method or accessor / non-writable property. If the helper succeeds, we simply define "foo" on obj without looking up "foo" property on each [[Prototype]].

For performance reasons, these helpers need to get [[Prototype]] directly, which was causing infinite loops if there is a cycle in Location's prototype chain, so we currently perform cycle checks for all objects except Proxy. This solution isn't perfect because it goes up the prototype chain of a cross-origin object.

However, these helpers should also bail for Proxy in prototype chain to avoid calling into userland trap. We just need to expand our checks from "is Proxy" to "has non-ordinary [[GetPrototypeOf]] method", aligning the helpers with current spec for OrdinarySetPrototypeOf:

8.c.i. If p.[[GetPrototypeOf]] is not the ordinary object internal method defined in 10.1.1, set done to true.

and implement it. I will submit a patch once the last helper that doesn't check for non-ordinary [[GetPrototypeOf]] method is fixed.

[annevk]

@shvaikalesh would that mean WebKit will then pass the WPT test? If so, we can close this and #1986.

[shvaikalesh]

@annevk That's right. Cyclic prototype chains are already a thing because of Proxy; I don't think adding cyclic checks for Location is worth the spec change.

Please track this bug for progress on that (probably) last helper, which is a blocker for the SetPrototypeOf issue.

[annevk]

Maybe @yuki3 can comment on the Chromium/V8 side of this?

[yuki3]

Let me check my understanding first. We'd like to support cyclic prototype chains for WindowProxy, Location, and Proxy at least because their [[GetPrototypeOf]] is not the ordinary object internal method (in OrdinarySetPrototypeOf). Chromium/V8 will be the last thing that doesn't support this.

I think that this is an area of V8. If V8 team is going to provide appropriate behaviors and APIs, I can make changes in Blink accordingly (just use the new APIs).

I filed a V8 issue. Please add comments to correct my understanding and/or add more contexts.