Bug 253814

Question for JSC people (added in cc). Do you know what I need to do in order to make concat() behave correctly with this new IDL type?

(In reply to Chris Dumez from comment #1)
> Question for JSC people (added in cc). Do you know what I need to do in
> order to make concat() behave correctly with this new IDL type?

The type of adoptedStyleSheets is a JSC::JSObservableArray, which is a subclass of JSC::JSArray.

(In reply to Chris Dumez from comment #2)
> (In reply to Chris Dumez from comment #1)
> > Question for JSC people (added in cc). Do you know what I need to do in
> > order to make concat() behave correctly with this new IDL type?
> 
> The type of adoptedStyleSheets is a JSC::JSObservableArray, which is a
> subclass of JSC::JSArray.

Looks like concat relies on isJSArray():

inline bool isJSArray(JSCell* cell)
{
    ASSERT((cell->classInfo() == JSArray::info()) == (cell->type() == ArrayType));
    return cell->type() == ArrayType;
}

So `cell->type() == ArrayType` is likely false for JSC::JSObservableArray.

(In reply to Chris Dumez from comment #3)
> (In reply to Chris Dumez from comment #2)
> > (In reply to Chris Dumez from comment #1)
> > > Question for JSC people (added in cc). Do you know what I need to do in
> > > order to make concat() behave correctly with this new IDL type?
> > 
> > The type of adoptedStyleSheets is a JSC::JSObservableArray, which is a
> > subclass of JSC::JSArray.
> 
> Looks like concat relies on isJSArray():
> 
> inline bool isJSArray(JSCell* cell)
> {
>     ASSERT((cell->classInfo() == JSArray::info()) == (cell->type() ==
> ArrayType));
>     return cell->type() == ArrayType;
> }
> 
> So `cell->type() == ArrayType` is likely false for JSC::JSObservableArray.

cell->type() is DerivedArrayType for JSObservableArray, not ArrayType.

Ecmascript for concat [1] says to rely on IsConcatSpreadable() [2]. IsConcatSpreadable() checks the @@isConcatSpreadable symbol and then fallback to calling IsArray() [3].

ObservableArray is defined here [4].

[1] https://tc39.es/ecma262/multipage/indexed-collections.html#sec-array.prototype.concat
[2] https://tc39.es/ecma262/multipage/indexed-collections.html#sec-isconcatspreadable
[3] https://tc39.es/ecma262/multipage/abstract-operations.html#sec-isarray
[4] https://webidl.spec.whatwg.org/#es-observable-array

(In reply to Chris Dumez from comment #5)
> Ecmascript for concat [1] says to rely on IsConcatSpreadable() [2].
> IsConcatSpreadable() checks the @@isConcatSpreadable symbol and then
> fallback to calling IsArray() [3].
> 
> ObservableArray is defined here [4].
> 
> [1]
> https://tc39.es/ecma262/multipage/indexed-collections.html#sec-array.
> prototype.concat
> [2]
> https://tc39.es/ecma262/multipage/indexed-collections.html#sec-
> isconcatspreadable
> [3] https://tc39.es/ecma262/multipage/abstract-operations.html#sec-isarray
> [4] https://webidl.spec.whatwg.org/#es-observable-array

From https://webidl.spec.whatwg.org/#observable-array-exotic-object:
```
An observable array exotic object is a specific type of ECMAScript Proxy exotic object which is created using the proxy traps defined in this section. They are defined in this manner because the ECMAScript specification includes special treatment for Proxy exotic objects that have Array instances as their proxy target, and we want to ensure that observable array types are exposed to ECMAScript code with this special treatment intact.
```

So I believe concat should indeed spread the observable array here. However, our implementation currently relies on isJSArray(), not isArray() (which would be what the spec says).

(In reply to Chris Dumez from comment #6)
> (In reply to Chris Dumez from comment #5)
> > Ecmascript for concat [1] says to rely on IsConcatSpreadable() [2].
> > IsConcatSpreadable() checks the @@isConcatSpreadable symbol and then
> > fallback to calling IsArray() [3].
> > 
> > ObservableArray is defined here [4].
> > 
> > [1]
> > https://tc39.es/ecma262/multipage/indexed-collections.html#sec-array.
> > prototype.concat
> > [2]
> > https://tc39.es/ecma262/multipage/indexed-collections.html#sec-
> > isconcatspreadable
> > [3] https://tc39.es/ecma262/multipage/abstract-operations.html#sec-isarray
> > [4] https://webidl.spec.whatwg.org/#es-observable-array
> 
> From https://webidl.spec.whatwg.org/#observable-array-exotic-object:
> ```
> An observable array exotic object is a specific type of ECMAScript Proxy
> exotic object which is created using the proxy traps defined in this
> section. They are defined in this manner because the ECMAScript
> specification includes special treatment for Proxy exotic objects that have
> Array instances as their proxy target, and we want to ensure that observable
> array types are exposed to ECMAScript code with this special treatment
> intact.
> ```
> 
> So I believe concat should indeed spread the observable array here. However,
> our implementation currently relies on isJSArray(), not isArray() (which
> would be what the spec says).

I have tried several approaches here but I keep hitting assertions. Seems existing code is not happy dealing with a JSObservableArray, even though it is a JSArray subclass.

Pull request: https://github.com/WebKit/WebKit/pull/11449

<rdar://problem/106667776>

Pull request: https://github.com/WebKit/WebKit/pull/11465

Committed 261604@main (9d30cd47aaab): <https://commits.webkit.org/261604@main>

Reviewed commits have been landed. Closing PR #11465 and removing active labels.