[[Set]] algorithm for legacy platform objects does not seem to be compatible with the Storage interface

[samweinig]

By my reading of the [[Set]] algorithm for legacy platform objects (https://heycam.github.io/webidl/#legacy-platform-object-set), it seems to state that an interface that doesn't support indexed properties, but does support named properties, will not allow setting indexed properties (by way of conversion to DOMString). (See step 1.2).

At the same time, the Storage interface (https://html.spec.whatwg.org/multipage/webstorage.html#the-storage-interface) only supports named properties, so as per my understanding, something like:

window.localStorate[7] = "foo";

should not be allowed. But, browsers (I tested Safari and Firefox) do allow this.

[domenic]

@bzbarsky, can you confirm we are not missing something here? Otherwise I think the correct fix is to just remove "P is not an array index property name," from step 1.2 of that algorithm.

[domenic]

OK, we were missing something. It will fall through to the default [[Set]], which will call [[DefineOwnProperty]], which will handle the named property stuff.

This raises the question of why [[Set]] has shortcut steps defined for named/indexed properties, instead of always falling back to [[DefineOwnProperty]]. There are undoubtedly edge cases where this matters, and I even have vague memories of discussing them in the past. We should figure out what they are, and document them in the spec for future people.

[bzbarsky]

Yes, agreed. I'm not quite sure what those edge cases are either, at the moment, nor whether they still exist now that setters are always creators as well. It's possible that the current setup was created back when we could have setters that were not creators...

[samweinig]

Ok, I see how my original concern is not accurate, as it will fall back to [[DefineOwnProperty]], but now it seems odd that the special cases in step 1 don't match the behavior of [[DefineOwnProperty]]. Specifically, they don't take into account unforgeable properties or [OverrideBuiltins].

[bzbarsky]

That does seem like a problem, good catch. This is relevant only for named setters, as far as I can tell, so in practice only for Storage and DOMStringMap. Neither one has unforgeable attributes, so we're ok there, a bit by accident.

OK, so now the overridebuiltins bit. Storage also isn't OverrideBuiltins (unlike DOMStringMap). So say we do Object.defineProperty(storageObj, "foo", { value: "bar" }). Per the letter of the spec as written right now, if the storage does NOT have a value for "foo" so far it will set the value to "bar", but if it does it will fall through to OrdinaryDefineOwnProperty, which will define a normal property on the object and start shadowing the storage's value.

I wonder what current UAs actually do here with that defineProperty bit.

[bzbarsky]

OK, I will go further and say that I suspect the spec's [[DefineOwnProperty]] is just wrong for things with named setters. I wrote a testcase:

var s = sessionStorage;
Object.defineProperty(s, "foo", { value: "a"});
console.log(s.getItem("foo"));
Object.defineProperty(s, "foo", { value: "b"});
console.log(s.getItem("foo"));

live at https://jsfiddle.net/fxc59fdp/ and the results are:

Firefox: logs "a" then "b"
Chrome: logs "a" then "b"
Safari: logs null then throws a "TypeError: Attempting to change value of a readonly property."
Edge: logs "a" then "b"

none of which match what the spec says (which is "a" then "a" right now).

[samweinig]

I'm not sure I follow exactly what steps you take to there. Is the crux of argument in [[DefineOwnProperty]]'s step 2.2:

"If O implements an interface with the [OverrideBuiltins] extended attribute or O does not have an own property named P, then:"

Where, in second call to Object.defineProperty in your example, neither of the conditions in this clause hold? (e.g. it is not [OverrideBuiltins] and O does have an own property named P).

Since the "have an own property" part is not linked to a specific algorithm, I took that to mean (now I feel foolish for this interpretation) that meant non-named getter properties. And that is why if you look at a ToT WebKit, it will have the same result as the other browsers.

[bzbarsky]

Is the crux of argument in [[DefineOwnProperty]]'s step 2.2

Yes.

Where, in second call to Object.defineProperty in your example, neither of the conditions in this clause hold?

Yes.

I took that to mean (now I feel foolish for this interpretation) that meant non-named getter properties

Hmm. If we define it that way, things do work ok, I think. We should make that very clear in IDL, though!

[samweinig]

Hmm. If we define it that way, things do work ok, I think. We should make that very clear in IDL, though!

Agreed. Everywhere it says things like "have an own property" without an explicit call to a ES hook is troubling.

[samweinig]

One algorithm in specific that uses "has an own property named P" is the named property visibility algorithm (https://heycam.github.io/webidl/#dfn-named-property-visibility) which is used in the legacy platform object [[GetOwnProperty]] implementation (via LegacyPlatformObjectGetOwnProperty). If that is taken literally, it seems to cause an infinite loop.

[Ms2ger]

This issue in the OP was fixed in #706. I'm going to say the other comments about "have an own property" are duplicating #42.