Tweak [[SetPrototypeOf]] custom internal methods #2400
Conversation
For Location and Window exotic objects, attempting to set their [[Prototype]] to its current value should not throw, in order to align with other immutable prototype exotic objects on the platform. Fixes #2035.
This expands the existing tests for Location to also cover WindowProxy, and to test the newly-introduced logic of allowing [[SetPrototypeOf]] if the value is the same as before: see whatwg/html#2400. It also expands coverage to include cross origin versions of the tests. The cross-origin tests here are slightly redundant with those under the name "cross-origin-objects", but their additional test coverage of all the vagaries of prototype-setting, and their simplicitly, argues for keeping them as well.
This expands the existing tests for Location to also cover WindowProxy, and to test the newly-introduced logic of allowing [[SetPrototypeOf]] if the value is the same as before: see whatwg/html#2400. It also expands coverage to include cross origin versions of the tests. The cross-origin tests here are slightly redundant with those under the name "cross-origin-objects", but their additional test coverage of all the vagaries of prototype-setting, and their simplicitly, argues for keeping them as well.
There was a problem hiding this comment.
I'd prefer if we land the tests first (and link them from the commit).
I think the only way this is observable cross-origin if you get the prototype object first in another frame, then set document.domain in that other frame, and then check if the operation does not throw. Would be good to test that as what's being tested now doesn't work I think (getting the prototype will simply return null across origins).
This does not cause any security regressions and if it makes things more consistent I guess that's worthwhile, although it is more churn.
|
Spec change LGTM, thanks for following up here (and sorry for being delinquent and not fixing this myself). |
All implementations that actually implement the new spec do it this way, in fact. |
|
Oh okay, commit message should probably say we're also aligning with implementations then. |
|
So we don't forget, as @domenic found out during test review, this patch doesn't work since it attempts to change the [[Prototype]] slot of a proxy object, rather than the Window object. |
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 do not expose it observably. (A notable case is HTML's WindowProxy.) As such, it should look up the prototype using [[GetPrototypeOf]] instead of trying to look at the possibly-nonexistant [[Prototype]] slot directly. Some background: * web-platform-tests/wpt#5015 (comment) * whatwg/html#2400
|
Blocked on tc39/ecma262#841. |
domenic
added
the
do not merge yet
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: * web-platform-tests/wpt#5015 (comment) * whatwg/html#2400
|
FWIW I believe Chrome does not implement the 'location' side of this, and enforces the same origin policy just with the cross-origin access checks (if I understand things correctly). See bug. Anyway, if/when Chrome does implement the spec, this change to make a uniform way of having immutable prototypes should make things easier. cc @verwaest |
|
A thought: To avoid needing changes on the ES side, what if we instead do the access check in line here, and then call out to SetImmutablePrototype? Location still has a |
|
It's not entirely clear to me what you're advocating. There's several options:
It's also not clear to me what your solution for WindowProxy is. Also inline everything there? |
|
@annevk I'm not suggesting any normative change vs this text; I don't have a real opinion on whether we should switch to Chrome's behavior. I'm good with this normative change for now. I was also brainstorming editorial ways we could get around the possibility that we won't have the refactoring in ES, without including a lot of duplication. I forgot about origins changing--seems like that could be a problem for my suggestion. |
|
If we don't make the linked change to ES, then we might as well not use SetImmutablePrototype at all, as it does't buy us anything. ("Return false" is shorter than "Return SetImmutablePrototype(...)".) If we want to maintain a world where all specs use the same consistent mechanism, then changing ES is the way to go. Currently Chrome, Safari, and Firefox all implement "allow null to be set cross-origin" for WindowProxy. |
|
Oh, I didn't know that! So that means Chrome, Firefox, and Safari do indeed use the [[GetPrototypeOf]] hook and not look at the internal [[Prototype]] slot. So ECMAScript not changing would be against what browsers are doing. |
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: * web-platform-tests/wpt#5015 (comment) * whatwg/html#2400
domenic
removed
the
do not merge yet
|
@annevk this and the tests should be good to merge now. |
This expands the existing tests for Location to also cover WindowProxy, and to test the newly-introduced logic of allowing [[SetPrototypeOf]] if the value is the same as before: see whatwg/html#2400. It also expands coverage to include cross origin versions of the tests. The cross-origin tests here are slightly redundant with those under the name "cross-origin-objects", but their additional test coverage of all the vagaries of prototype-setting and different cross-origin situations argues for keeping them as well.
|
Here are all the bugs filed on the general prototype-setting behavior for Location and WindowProxy. |
For Location and Window exotic objects, attempting to set their
[[Prototype]] to its current value should not throw, in order to align
with other immutable prototype exotic objects on the platform.
Fixes #2035.
/cc @littledan
Tests: web-platform-tests/wpt#5015