Layering: change SetImmutablePrototype to use [[GetPrototypeOf]]

[domenic]

SetImmutablePrototype is meant for use by a variety of objects beyond the ECMA-262 spec. Some of those objects do not have a [[Prototype]], or have a [[Prototype]] but use custom [[GetPrototypeOf]] logic to act is they don't sometimes. (Respectively: HTML's WindowProxy and Location objects.) As such, SetImmutablePrototype should look up the prototype using [[GetPrototypeOf]] instead of trying to look at the possibly-nonexistant [[Prototype]] slot directly.

Some background:

• Test more precisely setting window/location's [[Prototype]] web-platform-tests/wpt#5015 (comment)
• Tweak [[SetPrototypeOf]] custom internal methods whatwg/html#2400

/cc @annevk @littledan.

This should have no normative impact on ECMA262 since Object's [[GetPrototypeOf]]() just returns [[Prototype]]. It makes https://github.com/whatwg/html/pull/2400 work correctly though. There are tests for the web platform parts of this in https://github.com/web-platform-tests/wpt/pull/5015.

[erights]

Possibly relevant:
tvcutsem/es-lab#21

I don't think it affects the conclusion.

[allenwb]

SetImmutablePrototype is meant for use by a variety of objects beyond the ECMA-262 spec. Some of those objects do not have a [[Prototype]], or have a [[Prototype]]

That's clearly incorrect (at the usage site) because SetImmutablePrototype is defined in 9.4.7 Immutable Prototype Exotic Objects and Immutable Prototype Exotic Objects are required to have a [[Prototype]] internal slot.

The proposed change isn't enough to fix this. At the very least it would be moved out of 9.4.7. But that doesn't seem like enough for other sorts of exotic objects. It's the definition of various exotic objects that makes the [[GetPrototypeOf]]/[[SetPrototypeOf]] abstractions concrete via a specific implementation. We need to keep in mind that these are specifications, not implementations. The way the spec is currently written actually says something important about the [[Prototype]] internal slot of Immutable Prototype Exotic Objects.

[domenic]

The point of SetImmutablePrototype being factored out, instead of inlined, was so that multiple specs could easily converge on the same behavior without manually synchronizing.

So I can move it out if the editor thinks that makes things clearer, although personally I think it's OK to keep things inside a section they are relevant too, even if they are also relevant to things not defined in that section.

The way the spec is currently written does say something important about [[Prototype]] of Immutable Prototype Exotic Objects, but that is not changed at all by this PR, as all Immutable Prototype Exotic Objects have a [[GetPrototypeOf]]() that just returns [[Prototype]], so it has no impact on them.

[annevk]

Wouldn't it be more logical for HTML's WindowProxy object [[SetPrototypeOf]] to invoke SetImmutablePrototype on its [[Window]] internal slot? It forwards all other operations after all.

[domenic]

That doesn't help with the cross-origin case where [[Window]]'s [[Prototype]] is non-null but we want to treat it as null. Same for Location. So we'd need to both make this change and that change. But when we make this change, that change is redundant anyway.

[annevk]

In the cross-origin case you cannot get hold of the actual prototype object so it doesn't matter as far as I can tell. (You can get hold of it in one cross-origin scenario, where you set document.domain after you grab it, but in that case it seems fine for it to not throw for that object.)

[domenic]

You can get hold of it in one cross-origin scenario, where you set document.domain after you grab it, but in that case it seems fine for it to not throw for that object.

Hmm, I guess that is our disagreement. It seems bad to treat such cross-origin windows specially. They should only allow setting the prototype to null, whereas you're proposing disallowing setting the prototype to null (despite getPrototypeOf returning null) and allowing setting it to the old value.

I think we want to maintain the invariant that for all objects with "immutable prototypes" (even if they are not formally Immutable Prototype Exotic Objects like %ObjectPrototype%), Object.setPrototypeOf(obj, Object.getPrototypeOf(obj)) does not throw, but any other value does.

[annevk]

Interesting, that is indeed different from what I had in mind this would do. I'd be curious to hear what @littledan and @evilpie (or someone else from Mozilla as determined by @evilpie) think.

[littledan]

I don't have really strong opinions here, but a couple thoughts:

• The refactoring seems OK; I don't have any of my own objections.
• I agree with Domenic's analysis here that this abstract operation would be meaningful for any object, not just ordinary objects, that want to implement immutable prototypes.
• The way that I did the implementation in V8 means that we can reuse the path I implemented for only "ordinary-ish" objects (which includes all objects currently in the web platform). Location is ordinary enough for the same path to work. In particular, note how Location objects still have a [[Prototype]] internal slot--it's just guarded by access checks.
• This discussion about factoring is about a pretty small amount of spec text; the worst case outcome is that we decide to keep ES "cleaner" and make HTML "messier" or vice-versa, but resulting in a pretty small amount of duplication/mislayering either way. For this reason, I'm OK with either outcome.
• I don't quite understand @allenwb's vision for how spec layering would work. Does HTML follow what you'd like? If not, do you have an example of another set of specs that does layering in the way that you think is what we should be doing?

[annevk]

If you can access internal slots you have access to [[Prototype]], there's no checks at that level. So from that description it sounds like you implemented how I thought it would work...

[bterlson]

@allenwb I don't understand the important bit being said by the current spec, can you expand?

The refactoring seems good to me - I agree that this abstract op is might as well be useful for objects that implement [[Get/SetPrototypeOf]] slots for [[Prototype]] access. [[Prototype]] is not an essential internal method so doesn't seem right to depend on it.

Re: where to put the clause, seems fine to leave it where it is (it's already called from Module Namespace Exotic Objects). I will ponder moving it out to a separate clause.