Bug 190997 – [WebIDL] Support serializing sequences and FrozenArrays of non-interfaces

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andy Estes]

[WebIDL] Support serializing sequences and FrozenArrays of non-interfaces

[Andy Estes]

rdar://problem/35983035

[Andy Estes]

Created attachment 353247 [details]
Patch

[Wenson Hsieh]

Comment on attachment 353247 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353247&action=review

> Source/WebCore/bindings/scripts/test/TestSerialization.idl:42
> +    attribute sequence<TestSerializationInheritFinal> twelthInterfaceSequenceAttribute;

twelth!

[Andy Estes]

(In reply to Wenson Hsieh from comment #3)
> Comment on attachment 353247 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353247&action=review
> 
> > Source/WebCore/bindings/scripts/test/TestSerialization.idl:42
> > +    attribute sequence<TestSerializationInheritFinal> twelthInterfaceSequenceAttribute;
> 
> twelth

🤦‍♂️

[Andy Estes]

Created attachment 353254 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 353254 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353254&action=review

> Source/WebCore/bindings/scripts/test/JS/JSTestSerialization.cpp:646
> +    auto tenthFrozenArrayAttributeValue = jsTestSerializationTenthFrozenArrayAttributeGetter(state, thisObject, throwScope);
> +    throwScope.assertNoException();
> +    result->putDirect(vm, Identifier::fromString(&vm, "tenthFrozenArrayAttribute"), tenthFrozenArrayAttributeValue);
> +
> +    auto eleventhSequenceAttributeValue = jsTestSerializationEleventhSequenceAttributeGetter(state, thisObject, throwScope);
> +    throwScope.assertNoException();
> +    result->putDirect(vm, Identifier::fromString(&vm, "eleventhSequenceAttribute"), eleventhSequenceAttributeValue);

Is this actually putting the raw array on the serialized object? Should this need to be copied/cloned somewhere?

I'd expect if there is:

    interface TestSerialization {
        attribute sequence<DOMString> seq;
        serializer = { attribute };
    }

That if I have an instance that looked like:

    let instance = new TestSerialization;
    instance.seq = ["one", "two"];

The toJSON's seq should be separate from the instance's seq:

    assert( instance.toJSON().seq !== instance.seq ); // (1)

Such that modifications to the toJSON result should not affect the instance's:

    let json = instance.toJSON();
    assert( instance.seq.length === 2 );
    json.seq.push("test");
    assert( instance.seq.length === 2 ); // (2)

Right now it looks like we are directly putting the value on to the JSON. So I'd think (1) and (2) would fail.

--

Honestly the spec doesn't seem too clear to me on this. (Note serializer became toJSON a while back):
https://heycam.github.io/webidl/#idl-tojson-operation

It's example only uses primitives, where getting the underlying value is a copy. It doesn't show what to do with non-primitives, or I'm missing it.

[Andy Estes]

(In reply to Joseph Pecoraro from comment #6)
> Comment on attachment 353254 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353254&action=review
> 
> > Source/WebCore/bindings/scripts/test/JS/JSTestSerialization.cpp:646
> > +    auto tenthFrozenArrayAttributeValue = jsTestSerializationTenthFrozenArrayAttributeGetter(state, thisObject, throwScope);
> > +    throwScope.assertNoException();
> > +    result->putDirect(vm, Identifier::fromString(&vm, "tenthFrozenArrayAttribute"), tenthFrozenArrayAttributeValue);
> > +
> > +    auto eleventhSequenceAttributeValue = jsTestSerializationEleventhSequenceAttributeGetter(state, thisObject, throwScope);
> > +    throwScope.assertNoException();
> > +    result->putDirect(vm, Identifier::fromString(&vm, "eleventhSequenceAttribute"), eleventhSequenceAttributeValue);
> 
> Is this actually putting the raw array on the serialized object?
eleventhSequenceAttributeValue (for example) is a newly-created JSValue. jsTestSerializationEleventhSequenceAttributeGetter() calls toJS() on the wrapped object's Vector, which converts the vector's items into JSValues in a MarkedArgumentBuffer then creates a new array with JSC::constructArray().

But now that I think about it, we won't always get a new value if the attribute uses [CachedAttribute], [CustomGetter], or [CustomSetter]. I'll think about how to handle those.

In the meantime, I wrote a layout test to check your questions:

> I'd expect if there is:
> 
>     interface TestSerialization {
>         attribute sequence<DOMString> seq;
>         serializer = { attribute };
>     }
> 
> That if I have an instance that looked like:
> 
>     let instance = new TestSerialization;
>     instance.seq = ["one", "two"];
> 
> The toJSON's seq should be separate from the instance's seq:
> 
>     assert( instance.toJSON().seq !== instance.seq ); // (1)
This assertion is true (the sequences are separate).

> 
> Such that modifications to the toJSON result should not affect the
> instance's:
> 
>     let json = instance.toJSON();
>     assert( instance.seq.length === 2 );
>     json.seq.push("test");
>     assert( instance.seq.length === 2 ); // (2)
These two assertions are also true (the sequences are separate).

[Joseph Pecoraro]

> > The toJSON's seq should be separate from the instance's seq:
> > 
> >     assert( instance.toJSON().seq !== instance.seq ); // (1)
> This assertion is true (the sequences are separate).
> 
> > 
> > Such that modifications to the toJSON result should not affect the
> > instance's:
> > 
> >     let json = instance.toJSON();
> >     assert( instance.seq.length === 2 );
> >     json.seq.push("test");
> >     assert( instance.seq.length === 2 ); // (2)
> These two assertions are also true (the sequences are separate).

Awesome, then this is looking good to me.

[Andy Estes]

Created attachment 361462 [details]
Patch

[Brent Fulgham]

Comment on attachment 361462 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=361462&action=review

r=me

> Source/WebCore/ChangeLog:14
> +        left that as a follow-up task, since I don't see any IDLs that currently need this.

If there are specs that support this idea, I think we should file a Bugzilla now that tracks this, and reference it in your FIXME comment.

But, if no IDL needs this at all I guess we don't have to track work that isn't even conceptually needed.

> Source/WebCore/bindings/scripts/CodeGenerator.pm:957
> +        # FIXME: Teach GenerateSerializerDefinition to serialize sequences of interfaces, then remove this line.

FIXME(Bug), please...

[Andy Estes]

Comment on attachment 361462 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=361462&action=review

>> Source/WebCore/ChangeLog:14
>> +        left that as a follow-up task, since I don't see any IDLs that currently need this.
> 
> If there are specs that support this idea, I think we should file a Bugzilla now that tracks this, and reference it in your FIXME comment.
> 
> But, if no IDL needs this at all I guess we don't have to track work that isn't even conceptually needed.

Filed bug #194439 and will add a comment below.

Thanks for reviewing!

[Andy Estes]

Created attachment 361511 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 361511 [details]
Patch

Clearing flags on attachment: 361511

Committed r241198: <https://trac.webkit.org/changeset/241198>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.