Handling of non-configurable stuff on Location is a bit confusing

[bzbarsky]

A few things:

1. https://html.spec.whatwg.org/multipage/history.html#location-getownproperty is doing something to preserve essential invariants, but only for built-in properties? Why bother doing it only for those?

2. https://html.spec.whatwg.org/multipage/history.html#location-defineownproperty looks like it will throw even if you do a defineProperty call that would be valid for a non-configurable readonly property: using a non-configurable descriptor with writable:false and the same value as the current value. Are we doing this because OrdinaryDefineOwnProperty would be confused by the changes to the descriptor that item 1 is about? Even if we want those changes, we could get the property descriptor using OrdinaryGetOwnProperty and then call ValidateAndApplyPropertyDescriptor directly, I would think...

@domenic @annevk I came across this while trying to implement this part of the spec. I just checked, and Object.defineProperty(location, "assign", { value: location.assign }) does not throw in any of Firefox, Chrome, Safari, but per current spec should, right? I doubt we'll want to start introducing exceptions that no one is throwing right now here.

[annevk]

This was defined a long time ago and seems to match the prose in https://github.com/annevk/html-cross-origin-objects/blob/3a6814d37870741ff5cb495a876ee15da7320bc4/README.md apart from some minor changes (and indenting that looks wrong). I also couldn't find discussion about this in that repository so I suspect it wasn't noticed.

1. Given that it matches what we try to do in [[DefineOwnProperty]] I suspect this was done to minimize the "weird" behavior.
2. I'm not sure there's much rationale here other than trying to protect built-in properties and not thinking about what are essentially no-ops. What kind of behavior do we want?

[bzbarsky]

What behavior we want... is unclear. If we're OK violating the ES invariants, then we can just remove [[DefaultProperties]] and all the steps that use it. That's what browsers do right now, I expect.

If we want to not violate the ES invariants, then I don't know what the right approach is. Once we figure out what we want to do for WindowProxy there, we can think about Location (which may have different compat constraints).

[annevk]

Okay, so wait for https://github.com/tc39/ecma262/issues/672 and https://github.com/tc39/ecma262/pull/688.

Reading https://tc39.github.io/ecma262/#sec-invariants-of-the-essential-internal-methods btw it seems we could simply return true for [[DefineOwnProperty]] here without violating the invariants. Not sure if that's intended though and it also wouldn't match implementations as those do some amount of equality checking.

[bzbarsky]

Okay, so wait for tc39/ecma262#672 and tc39/ecma262#688.

Yeah...

it seems we could simply return true for [[DefineOwnProperty]] here without violating the invariants.

Return true in what case?

[annevk]

When it's in [[DefaultProperties]].

[bzbarsky]

As far as I can tell, the second invariant for [[DefineOwnProperty]] would be violated if we did:

var desc = Object.getOwnPropertyDescriptor(location, "replace");
// desc.configurable === desc.writable === false, which we have now observed.
Object.defineProperty(location, "replace", 
                      { value: location.replace, configurable: true });

The invariant in question says:

• [[DefineOwnProperty]] must return false if P has previously been observed as a non-configurable own property of the target, unless either:
  1. P is a non-configurable writable own data property. A non-configurable writable data property can be changed into a non-configurable non-writable data property.
  2. All attributes in Desc are the SameValue as P's attributes.

In this case P has been observed as a non-configurable property. Exception 1 does not apply, because writable === false. Exception 2 does not apply because Desc has configurable: true but the corresponding attribute of P is false. So returning true would violate the invariant, unless I'm missing something.

[annevk]

Wouldn't configurable be true though due to how we define [[GetOwnProperty]]?

[bzbarsky]

Oh, if you left that bit, then sure. Sorry, I had misunderstood the proposal.

I guess that still doesn't do the thing Mark Miller wants, which is to know whether Object.defineProperty really succeeded, but that's not part of the invariants as specced...

[annevk]

Let's loop in @erights directly since I don't think either of us really care what we do here as long as it's web-compatible and TC39 is somewhat comfortable. (It's best to start from OP.)

[erights]

I am confused about what is going on here, but no matter what, we must not violate the essential invariants. Some questions:

Object.defineProperty(location, "assign", { value: location.assign })

Given that location.assign already exists, then for all objects, whether exotic or not, I would expect this call to succeed and do nothing. All the unstated attributes, writable, enumerable, configurable default to their current settings. This sets value to its current setting. So it is not asking for anything to change. Not changing anything should fully satisfy this request.

var desc = Object.getOwnPropertyDescriptor(location, "replace");
// desc.configurable === desc.writable === false, which we have now observed.
Object.defineProperty(location, "replace", 
                      { value: location.replace, configurable: true });

Yes, this must fail with an error. The WindowProxy special case is intended only for WindowProxy. Without extending this special case (please!), the code above must fail with a thrown error.

[annevk]

@erights what requires [[GetOwnProperty]] to say it's non-configurable? If [[GetOwnProperty]] just always says it's configurable, we could make [[SetOwnProperty]] always succeed without changing anything, no? (Note that WindowProxy and Location are the only two objects that can be obtained cross-site, so therefore any special cases might have to apply to both, but we can of course see if things can be restricted somehow.)

[erights]

Yes. The best solution is to always report that the property is configurable. Any attempt to make it non-configurable must fail with a thrown error. If we can just do that, we avoid all hard questions.

[annevk]

Where is it defined that making it non-configurable (presumably this is via setting Property Descriptor's [[Configurable]] to false) has to throw? I cannot find it in ValidateAndApplyPropertyDescriptor or the invariants. (I'm not sure whether making it throw is a problem here though.)

[erights]

Here's another sanity test: Can the behavior be faithfully emulated by a membrane? If we simply have location refuse to make non-configurable properties in the normal way, the answer is yes.

The special case we made for WindowProxy, where Object.defineProperty returns false rather than throwing, does violate this constraint. It cannot be faithfully emulated by a Proxy. We knowingly allowed this exception with the understanding it was just for this one object --- the browser WindowProxy, and not even the global of any other host.

I would rather not make such an exception for location. Let's try to do the sane thing first and see if we get away with it. If we can't get away with it, then let's figure out how to cope armed with the specific knowledge of how the simple thing failed.

[erights]

attn @tvcutsem

[bzbarsky]

So if I understand correctly, the proposal is:

1. When setting up the Location object initially, there is no magic behavior for [[DefineOwnProperty]] or [[GetOwnProperty]] (because it needs to have properties that behave as-is they were non-configurable). Once the setup is complete, we set a flag on the Location object to indicate that.
2. When the flag is set, [[GetOwnProperty]] claims all descriptors as configurable.
3. When the flag is set, [[DefineOwnProperty]] returns false if a descriptor with configurable: false is passed in, right?

Things that are not clear to me:

• What should happen in [[DefineOwnProperty]] if a descriptor with no configurable attribute at all is passed in?
• How do we prevent redefinition of the unforgeable properties, if [[GetOwnProperty]] claims they are configurable? Should we keep explicitly returning false from [[DefineOwnProperty]] for them like the spec does now?

[tvcutsem]

Arriving late to the party, and I don't have all the context, but: from an ES-perspective it should be perfectly fine to report these properties as configurable: true and at the same time prevent them from being changed by clients by returning false from [[DefineOwnProperty]] even if those changes would be legitimate on a non-exotic ES object.

Clients might be surprised to see exceptions pop up when there should be none (i.e. a client observes a configurable property, tries to change it, and fails), but this is allowed behavior, and could be implemented with a standard ES Proxy. The reverse, letting a client observe a non-configurable property whose values then change afterward, is a violation of the ES invariants.

[bzbarsky]

Clients might be surprised to see exceptions pop up

Right, that is the issue. If real-world code would run into those exceptions, having them is not web-compatible. We know this is an issue for Window. We don't have any data for Location so far.

The reverse, letting a client observe a non-configurable property whose values then change afterward, is a violation of the ES invariants.

Right, that's what we're trying to solve here: right now this ES-invariant violating behavior is what you get in browsers.

[bathos]

I was surprised to see location still violates ES. Other interfaces like NodeList seem to use the solution @tvcutsem described (note: the always-configurable recipe implies [[PreventExtensions]] must also be denied, which is also true for NodeList).

Does something make Location uniquely difficult to address that way? Web compat issue?

[bzbarsky]

Fear of those, yes, see above. For Window, we tried that and it was very non-web-compatible. It's possible that for Location it might be. Someone would need to try shipping it, basically, or at least measuring.

[annevk]

So I think we should do the same as we did for WindowProxy, personally. It's not like we'll need it beyond these two objects and having some consistency between them seems good. Or at least better than trying to find different workarounds for each.

[ExE-Boss]

Well, Location objects, unlike WindowProxy, are discarded on navigation:

const iframe = document.createElement("iframe");
document.body.append(iframe);

const oldLocation = iframe.contentWindow.location;
iframe.contentWindow.location.href = "https://example.org";

console.assert(oldLocation !== iframe.contentWindow.location);

Because of this, for Location objects, non‑configurable property visibility will never change within the confines of an origin, so the Invariants of the Essential Internal Methods will be preserved within the confines of a Realm, since a Realm’s origin cannot change after construction.

[bzbarsky]

non‑configurable property visibility will never change within the confines of an origin

Unless document.domain is set. Then they will change.

since a Realm’s origin cannot change after construction.

Except for the document.domain bits....