Is internal method setup in object creation correct?

[domenic]

In https://github.com/heycam/webidl/pull/635#discussion_r256600690 @TimothyGu pointed out some problems with the setup, but I'm not sure I understand them, or maybe they got fixed in the meantime. Opening this issue to make sure we keep track.

My current understanding of https://heycam.github.io/webidl/#internally-create-a-new-object-implementing-the-interface is that it works out OK:

• Make an ordinary object with ordinary internal methods
• Define all the properties on it (using the ordinary [[DefineOwnProperty]] etc. which will behave in predictable ways)
• Override the internal methods to start doing the exotic platform object things as the last step.

This seems pretty good to me. @TimothyGu, are we missing something?

[Ms2ger]

At least in SpiderMonkey, I'm pretty sure changing the internal methods after creation isn't possible, so specifying it like that is a guaranteed mismatch with the code. Maybe that's fine.

[bzbarsky]

This matters in practice for exotic objects with unforgeable properties, right? Step 12 (handling globals) is not a problem because globals never need to have special internal methods. Though step 13 probably needs to explicitly exclude globals, actually.

In any case, exotic objects with unforgeable properties basically means Location and Document.

In Gecko, there are much bigger problems with mismatches than just the one you point out. For example, in Gecko you often can't apply the ordinary-object internal methods to exotic objects at all. And you can't just add properties to exotic objects. The way we handle this in practice is that legacy platform objects have an ordinary object hanging off them in an internal slot where all the "ordinary properties" bits live. The unforgeables just get defined directly on there during object creation, so there is no problem with the special internal methods, because they never get called. The observable effect should be the same as in the spec, though...

[TimothyGu]

I'm going to try to clarify my comment a bit. So the present issue here is that we are assigning internal methods as if they can be changed at any time, even after we have already called the old internal methods on that object. ES never really explicitly forbids that, but they never do it in practice, so we perhaps shouldn't either. Implementation internals more or less agree with this restriction, though the differences are not observable to JavaScript.

However, fixing this isn't as straightforward as just moving all the internal method assignment steps to be right after step 9, due to the desire to make the "define" algorithms otherwise unobservable to these overridden internal methods. So realistically we would have to do two things:

1. Move the internal method override assignments to be right after step 9.

2. Change the "define" algorithms to not call internal methods when assigning own properties. E.g., instead of saying:

   1. Let desc be the PropertyDescriptor{[[Value]]: method, [[Writable]]: modifiable, [[Enumerable]]: true, [[Configurable]]: modifiable}.
   2. Let id be op’s identifier.
   3. Perform ! DefinePropertyOrThrow(target, id, desc).

   Say something like

   1. Let desc be the PropertyDescriptor{[[Value]]: method, [[Writable]]: modifiable, [[Enumerable]]: true, [[Configurable]]: modifiable}.
   2. Let id be op’s identifier.
   3. Assert: Object target does not have an own property with key id.
   4. Create an own data property named id of object target whose [[Value]], [[Writable]], [[Enumerable]] and [[Configurable]] attribute values are described by desc.

   or (without creating the intermediate Property Descriptor Record)

   1. Let id be op’s identifier.
   2. Assert: Object target does not have an own property with key id.
   3. Create an own data property named id of object target whose [[Value]] attribute is method, [[Writable]] attribute is modifiable, [[Enumerable]] attribute is true, and [[Configurable]] attribute is modifiable.

   or (without mucking with own properties ourselves and use ES's abstract operations as much as possible)

   1. Let id be op’s identifier.
   2. Let current be ! OrdinaryGetOwnProperty(target, id).
   3. Assert: current is undefined.
   4. Let desc be the PropertyDescriptor{[[Value]]: method, [[Writable]]: modifiable, [[Enumerable]]: true, [[Configurable]]: modifiable}.
   5. Perform ! ValidateAndApplyPropertyDescriptor(target, id, true, desc, current).

   I personally prefer the second variant, as it's the most concise.

[Ms2ger]

Though step 13 probably needs to explicitly exclude globals, actually.

I'm not sure it's explicit enough for you, but it starts with "Otherwise, if..."

[bzbarsky]

I'm not sure it's explicit enough for you, but it starts with "Otherwise, if..."

That's explicit enough! Just reading (skimming, really) fail on my part.