non-readonly [FrozenArray] attributes are footguns as currently specced

[bzbarsky]

I've brought this up before, but I can't find any relevant issues, so trying to put it all down in one place.

Consider this IDL:

attribute FrozenArray<long> arr;

What happens when the setter is called? Per spec we land in https://heycam.github.io/webidl/#es-to-frozen-array which converts the value passed to the setter to a sequence, then converts that sequence to a JS Array object, freezes it, and passes that off to the underlying spec-defined setter.

OK, but now the spec-defined setter is getting a JS Array, and spec authors have no idea how to deal with those. A typical example of a spec using this stuff (not linking, because I am not trying to call out anyone in particular) looks like this:

For each entry in input
....
Set image’s sizes to entry’s sizes

Both of those statements are problematic in this case. "For each entry in input" is ambiguous when input is a JS Array. Does that mean calling input.forEach? Does it mean calling the original value of Array.prototype.forEach? Does it mean doing the equivalent of for (let entry of input)? Does it mean doing for (var i = 0; i < input.length; ++i) { let entry = input[i]; // etc? All of those produce observably different behaviors. Spec authors probably (1) don't realize those options exist, (2) don't realize they do different things. Implementors are left to make up something.

"Set image's sizes to entry's sizes" is similarly problematic: input is a JS Array, so entry is a JS value. In this case, because the FrozenArray type parameter is a dictionary type we know that Type(entry) is Object, but that says nothing about how "entry's sizes" is computed. The principled thing would be to convert the object to the relevant dictionary type, but that is a little silly because we already had that dictionary in our sequence before we created the JS Array, and dictionaries don't actually roundtrip through to-JS-and-back conversions if someone has messed with Object.prototype...

OK, so what do implementations do?

• Gecko doesn't implement FrozenArray at all, for some of the above reasons; the thing we do implement has the semantics of a sequence-typed attribute with automatic caching at the binding layer on get and automatic cache invalidation on set.

• Blink claims to implement FrozenArray, but doesn't follow the spec and passes a sequence instead of a JS Array to the underlying setter, as far as I can get. Getters are expected to return a JS Array; the typical Blink implementation of a FrozenArray API returns a new JS Array on each get, which is also problematic.

• WebKit seems to pass a sequence-like thing, not a JS Array for the setter and likewise expect one for the getter; I didn't dig enough into whether they cache on get or not. There are no non-test writable FrozenArray-typed attributes in WebKit, as far as I can see.

I would like to propose that we more or less reify the Gecko semantics into the IDL spec: an ES-to-FrozenArray conversion creates a sequence, while a FrozenArray-to-ES conversion takes a sequence as input, creates a frozen JS Array from it, and caches it. At least for the attribute getter case. I'm not sure about method return values or FrozenArray used in dictionary members, but I'm also not sure we have any use cases for that sort of thing, and perhaps we should just disallow it....

@domenic @annevk @Ms2ger @heycam @tobie @saschanaz

[bzbarsky]

Alternately, we could eliminate FrozenArray altogether and do the "freeze and cache" thing via extended attributes on the type or attribute, I suppose. I'm not sure that's better than having a dedicated type, but it would make it clear that the underlying implementation sees a sequence.

[annevk]

I like the extended attribute idea. Something like (not entirely sure about placement of extended attributes, forgive me):

[Cached] attribute [FrozenArray] sequence<Item> arr;

(With [Cached] from #212. And [FrozenArray] can only be used if [Cached] is used or if it's a method, and the type is a sequence.)

[saschanaz]

Eliminating FrozenArray would syntactically allow sequence<> in attributes, and that might unexpectedly encourage authors to write non-frozen sequence attributes (because they won't get parser errors anymore).

[bzbarsky]

Eliminating FrozenArray would syntactically allow sequence<> in attributes,

For what it's worth, Gecko's parser allows sequence<> in attributes only if [Cached] is used. There's no problem with non-frozen sequence-typed attributes per se; there's a problem with non-cached ones, since you'd get a new value on each get...

[karlt]

Would [FrozenArray] and [Cached] extend to this?

callback MyCallback =
  void ( [Cached] [FrozenArray] sequence< [FrozenArray] sequence<Item> > parameter )

The desired behavior of [Cached] here is that the same ECMAScript Array is produced on sequence-to-ES conversion when the same sequence is passed for parameter, to avoid a copy each time.

[bzbarsky]

I don't think that's currently being proposed, especially because there is no concept of "same sequence", really: sequences are passed by value, so the best you could do is define that two sequences are "the same" if the lengths are the same and all the items are "the same", but then you have to define what "the same" means for Item in that case...

[karlt]

Thanks. I can see the problem if sequences do not have identifying handles.

I'm trying to understand in "a FrozenArray-to-ES conversion takes a sequence as input, creates a frozen JS Array from it, and caches it" where the result would be cached.

Is there a proposed description of [Cached]?

Does [Cached] indicate that the language binding for an attribute (for example, or only attributes even) should cache?
And each specification would indicate when the language binding caches should be invalidated (if required)?

I can see how that would not extend well to callback parameters.

Currently,

callback MyCallback = void ( FrozenArray<FrozenArray<Item>> parameter )

would pass by reference to an ECMAScript callback.
I understand the proposal here would change that to pass by copy.
Specifications preferring pass by reference could still retain the same behavior through

callback MyCallback = void ( object parameter )

That is arguably more informative, because it is clearly an Object, and less informative, because the nature of the Object would need more description in prose.

[bzbarsky]

Is there a proposed description of [Cached]?

Sure. I would define it as follows, informally, for the ES binding:

[Cached] is an extended attribute that can be used on regular attributes of a non-callback interface. When it is used, objects implementing that interface have a slot that corresponds to that attribute. When the getter for the attribute runs, if there value in the slot is not undefined that value is returned. Otherwise, the normal getter steps are executed, and the result is stored in the slot. When the setter for the attribute runs, the value in the slot is cleared

In spec terms, https://heycam.github.io/webidl/#internally-create-a-new-object-implementing-the-interface step 4 would add more slots to the list as needed. https://heycam.github.io/webidl/#dfn-attribute-getter would have extra steps to check the slot as needed and to store the gotten value into the slot after the https://heycam.github.io/webidl/#get-the-underlying-value step and doing the converting to an ES value. https://heycam.github.io/webidl/#dfn-attribute-setter would have extra steps to clear the slot, probably after the "Perform the actions" step, and probably only if that step does not throw. I think that's about all that's needed, plus possibly some language about being able to clear the slot manually from spec algorithms as needed if the value changes other than through the setter.

Does [Cached] indicate that the language binding for an attribute (for example, or only attributes even) should cache?

That is my proposal, yes.

I understand the proposal here would change that to pass by copy.

If we got rid of the FrozenArray type altogether, you mean? It would depend on what type got used instead. If we kept FrozenArray but just changes what ES-to-IDL conversion means for it, there would be no difference.

Are there specific APIs you're thinking of that use that pattern? I'm not aware of any offhand; https://wicg.github.io/CSS-Parser-API/ has FrozenArray<FrozenArray<CSSParserValue>> but that's a readonly regular attribute on an interface.

Specifications preferring pass by reference could still retain the same behavior through

Indeed.

[karlt]

I understand the proposal here would change that to pass by copy.

If we got rid of the FrozenArray type altogether, you mean? It would depend on what type got used instead. If we kept FrozenArray but just changes what ES-to-IDL conversion means for it, there would be no difference.

If we kept FrozenArray but changed its IDL-to-ES conversion as initially proposed, then it would be pass by copy for ECMAScript callbacks. If the ES-to-IDL conversion is changed to produce a sequence, then the IDL-to-ES conversion will also need to change.

To be clear, I'm not advocating either way here, just watching for how this might impact direction in WebAudio/web-audio-api#1933

[domenic]

I agree this is a problem.

I think one route toward fixing this is using the classes in #796 to completely replace all FrozenArray<> usage. What follows is my response to this thread, predicated on us deciding that is not sufficient and we want to keep frozen arrays.

---

My initial impulse was toward just "fixing" FrozenArray<> rather than introducing [Cached], as I think "a frozen array type" is a reasonable semantic, and it'd cause a bit less churn for the ecosystem. However, it'd be good to hear if [Cached] has other use cases besides this one.

Another factor that makes me consider [FrozenArray] sequence<> over FrozenArray<> is that we could naturally restrict [FrozenArray] usage to attributes, ignoring it for dictionary members, operation arguments, or operation return values. I guess we might be able to restrict FrozenArray<> the same way, but it seems weirder.

Regardless, I think it's worth thinking through what we want the spec authoring experience to be. Here's a stab:

• readonly frozen array attribute:
  • Spec authors do not specify the getter behavior; that is auto-generated.
  • Spec authors can "reset the frozen array value for instance's attrName to x" where x is an Infra list/IDL sequence. This:
    • Clones x and stores it as the backing list.
    • Creates a frozen JS array value and starts returning it from the getter.
  • Spec authors can reference "instance's attrName's backing list", which is an Infra list/IDL sequence. This can be "for each"ed over, etc., but must not be mutated. Instead of mutating it, spec authors must use "reset the frozen array value".
• non-readonly frozen array attribute:
  • Mostly the same as above, plus an auto-generated setter
  • Spec authors need to take care to re-consult the backing list, and not take a reference to it (unless they really do mean to store a snapshot of what it was, ignoring future author mutations).

This seems reasonable, although a bit fragile. Is it possible to improve?

[annevk]

Did you see #212? We can use [Cached] for lots of things.

[bzbarsky]

I think one route toward fixing this is using the classes in #796 to completely replace all FrozenArray<> usage.

So in that world, things that currently have readonly FrozenArray attributes would return either ReadonlyArray or ReactToAbleArray? And current code that uses the attibute setter, assuming any such APIs are shipping, would be modified to modify the ReactToAbleArray?

Here's a stab:

This seems pretty reasonable to me, with the additional note that the "create and freeze" step is not observable, and could be done lazily on get, as far as I can tell, as long as the global used for the creation can't change over time. In either case that global should be defined, presumably to the relevant global of the object involved.

We could consider having mutations to the backing infra list automagically invoke "reset the frozen array value" to the new value, so spec authors can mutate it directly. That does involve some slightly weird action at a distance that might be non-obvious when reading and implementing a spec... I don't have any great ideas for improving that so far.

[domenic]

So in that world, things that currently have readonly FrozenArray attributes would return either ReadonlyArray or ReactToAbleArray?

Yes. There are potential compat issues (most notably breaking Array.isArray()), and I'm a bit unsure whether browsers will want to take them on, which is why I focused that thread on providing something for new APIs. But it's a possibility.

And current code that uses the attibute setter, assuming any such APIs are shipping, would be modified to modify the ReactToAbleArray?

I think the attribute setters could still work. I guess specs would use the same "reaction" spec text to update their internal data when setter is called as when the ReactToAbleArray is mutated.