Named properties visibility algorithm leads to ES invariant violation

[bzbarsky]

Consider the testcase at https://bug1297411.bmoattachments.org/attachment.cgi?id=8784910. This does the following:

1. Define a non-configurable non-writable property on an [OverrideBuiltins] interface.
2. Modify the object so it would have a named prop with the same name (e.g. add an input to a form).

Per the letter of the IDL spec right now, this leads to an ES invariant violation: getOwnPropertyDescriptor after step 2 lands at http://heycam.github.io/webidl/#named-properties-object-getownproperty and then ends up in http://heycam.github.io/webidl/#dfn-named-property-visibility which returns true in step 4, so the returned descriptor ends up claiming a configurable property whose value is the input element.

Current browser behavior is as follows:

1. Safari more or less follows this broken spec as written (except it claims the named prop is not configurable and not writable, but has its value change anyway).
2. WebKit nightly follows the spec exactly for forms but has the Safari behavior for document.
3. Chrome matches WebKit nightly for forms but preserves the invariant for document in a way I'll describe below.
4. Gecko preserves the invariant in a way I'll describe below.
5. Edge both preserves the invariant for document in a way I'll describe below and violates it for forms in exactly the way Safari does.

I see two conceivable ways of preserving the invariant here:

1. Switch the order of steps 4 and 5 in http://heycam.github.io/webidl/#dfn-named-property-visibility so that named props don't shadow own properties even for [OverrideBuiltins] interfaces. This is the approach Gecko takes for both form and document and seems to be the approach Edge takes for document. If this is done, then I believe steps 1 and 2 of the algorithm become unnecessary, by the way, but only because we never have objects with onforgeable properties that are being set up after they have named props...
2. Replace steps 1 and 2 with a check for an own non-configurable property with the given name and return false if found. This seems to be the approach Blink takes for document. Again, this is ok because we never have objects with unforgeable properties that are being set up after they have named props. If we wanted to have this be conceptually sound in general we'd need to leave steps 1 and 2 (which might be combinable?) and add a step between the current steps 2 and 3.

The two options lead to quite different behavior on the testcase at https://bugzilla.mozilla.org/attachment.cgi?id=8784908 -- option 1 has the expando shadow the named prop, and option 2 has the named prop shadow the expando.

In practice, I expect both options are web compatible enough and I think option 1 is simpler to both specify and implement. But I would be interested in hearing what other people think.

@cdumez, @domenic, @travisleithead, thoughts?

[annevk]

Also @ajklein.

[ajklein]

Blink gets the document case right for a much simpler reason: it doesn't actually implement a named getter for Document (see https://bugs.chromium.org/p/chromium/issues/detail?id=594008 for some discussion).

@bzbarsky I'm curious to hear more of your intuition about why (1) doesn't have compat issues, as It seems like it defeats much of the point of [OverrideBuiltins] in the first place.

[bzbarsky]

My inituition for why (1) doesn't have compat issues is that Firefox has been shipping it for years with no known compat issues, and Edge is shipping it for document objects as well, with no compat issues.

The main point of [OverrideBuiltins] is to allow shadowing stuff on the proto chain. It's not actually that common for people to define expandos on form elements (though probably a bit more common on documents)...

[bzbarsky]

and Edge is shipping it for document objects as well, with no compat issues.

Well, more precisely as far as I can tell Edge implements (1) and I have not heard them raise any issues about any of this. This does not mean they have not encountered compat issues, obviously.

I stand by the Firefox part of what I said, though. ;)

[yuki3]

Given there is no compat issue, I'd support Option 1: Switch the order of steps 4 and 5 in the named property visibility algorithm, making all own properties have priority over named properties. I'm happy to make the change in Blink bindings once it gets spec'ed.

[bzbarsky]

OK, we've landed the "option 1" solution. Please do let me know if there are objections...

[cdumez]

It would be good to have this covered in a web platform test.

[bzbarsky]

I'm adding some tests in https://bugzilla.mozilla.org/show_bug.cgi?id=1297304 that will get upstreamed to wpt. They test this and related bits for the return value of getElementsByTagName, and for an HTML <form>. It might be good to add tests for document too; I can do that once that stuff lands by more or less copy-pasting things, but I figured I should see whether there were review comments first.

[bzbarsky]

OK, checked in that Gecko patch with the wpt tests; they should get upstreamed sometime in the next week or three.