Bug 210728 – StructuredClone algorithm should be aware of BigInt

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Yusuke Suzuki]

...

[Yusuke Suzuki]

Created attachment 396940 [details]
Patch

WIP

[Mark Lam]

Comment on attachment 396940 [details]
Patch

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

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1073
> +            write(BigIntTag);

Shouldn't this be BigIntObjectTag?

[Yusuke Suzuki]

Comment on attachment 396940 [details]
Patch

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

>> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1073
>> +            write(BigIntTag);
> 
> Shouldn't this be BigIntObjectTag?

This is correct because we are serializing BigInt here instead of BigIntObject.

[Mark Lam]

Comment on attachment 396940 [details]
Patch

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

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:395
> + *      <sign:uint8_t> <lengthInUint64:uint32_t> <contents:uint64_t{lengthInUint64}>

Should lengthInUint64 be lengthInUint32?

[Mark Lam]

Comment on attachment 396940 [details]
Patch

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

>> Source/WebCore/bindings/js/SerializedScriptValue.cpp:395
>> + *      <sign:uint8_t> <lengthInUint64:uint32_t> <contents:uint64_t{lengthInUint64}>
> 
> Should lengthInUint64 be lengthInUint32?

Oh, never mind.  I see what you mean by lengthInUint64.  It's the number of uint64s.

[Yusuke Suzuki]

Created attachment 396950 [details]
Patch

[Mark Lam]

Comment on attachment 396950 [details]
Patch

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

r=me

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3003
> +                if (digit64 <= static_cast<uint64_t>((-static_cast<int64_t>(INT32_MIN))))

You can remove 1 set of () around -static_cast<int64_t>(INT32_MIN).

[Yusuke Suzuki]

Comment on attachment 396950 [details]
Patch

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

Thanks!

>> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3003
>> +                if (digit64 <= static_cast<uint64_t>((-static_cast<int64_t>(INT32_MIN))))
> 
> You can remove 1 set of () around -static_cast<int64_t>(INT32_MIN).

Fixed.

[Yusuke Suzuki]

Created attachment 396952 [details]
Patch

Patch for landing, seeing EWS

[Yusuke Suzuki]

Created attachment 396955 [details]
Patch

Patch for landing, watching EWS, fixing GCC build failure

[Radar WebKit Bug Importer]

<rdar://problem/62048672>

[Yusuke Suzuki]

WK1, WK2 succeeds. And in Debug, only one test is failing (crashing) and it is crashing before this patch according to history. Landing.

[Yusuke Suzuki]

Committed r260358: <https://trac.webkit.org/changeset/260358>

[Saam Barati]

Comment on attachment 396955 [details]
Patch

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

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3005
> +            if (sign) {

This logic of if it can fit in a big int 32 should not be here. Instead, we should put that logic in JSC. Maybe even having a constructor on BigInt that tries to return it in inline form if it can.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3039
> +            ASSERT(sizeof(JSBigInt::Digit) == sizeof(uint32_t));

Why not “else if constexpr” here? And else case can have static_assert(false)

[Saam Barati]

Comment on attachment 396955 [details]
Patch

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

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:395
> + *      <sign:uint8_t> <lengthInUint64:uint32_t> <contents:uint64_t{lengthInUint64}>

Why not just call this digitLength and not do the weird “/ 2” on 32-but?

[Mark Lam]

Comment on attachment 396955 [details]
Patch

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

>> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3039
>> +            ASSERT(sizeof(JSBigInt::Digit) == sizeof(uint32_t));
> 
> Why not “else if constexpr” here? And else case can have static_assert(false)

Wouldn’t the if constexpr above endure that this else is effectively constexpr too?

[Mark Lam]

(In reply to Mark Lam from comment #16)
> Comment on attachment 396955 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396955&action=review
> 
> >> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3039
> >> +            ASSERT(sizeof(JSBigInt::Digit) == sizeof(uint32_t));
> > 
> > Why not “else if constexpr” here? And else case can have static_assert(false)
> 
> Wouldn’t the if constexpr above endure that this else is effectively
> constexpr too?

Ensure, not endure.

[Yusuke Suzuki]

Comment on attachment 396955 [details]
Patch

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

>> Source/WebCore/bindings/js/SerializedScriptValue.cpp:395
>> + *      <sign:uint8_t> <lengthInUint64:uint32_t> <contents:uint64_t{lengthInUint64}>
> 
> Why not just call this digitLength and not do the weird “/ 2” on 32-but?

Because `JSBigInt::Digit` has different size in 32bit v.s. 64bit while structured-cloning data should be machine independent (we are even handling endianess for various data).

>> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3005
>> +            if (sign) {
> 
> This logic of if it can fit in a big int 32 should not be here. Instead, we should put that logic in JSC. Maybe even having a constructor on BigInt that tries to return it in inline form if it can.

Sounds good. As a follow-up patch, I will put this into `JSBigInt` in JSC.

>>>> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3039
>>>> +            ASSERT(sizeof(JSBigInt::Digit) == sizeof(uint32_t));
>>> 
>>> Why not “else if constexpr” here? And else case can have static_assert(false)
>> 
>> Wouldn’t the if constexpr above endure that this else is effectively constexpr too?
> 
> Ensure, not endure.

This does not work since `constexpr if` is not compile-time if statement.
For example,

if constexpr (true) {
} else {
    static_assert(false);
}

becomes compile-error. This is because constexpr-if feature is suppressing template-materialization in non-taken branch. `static_assert(sizeof(JSBigInt::Digit) == sizeof(uint32_t));` does not include template materialization so this becomes compile-error.

[Yusuke Suzuki]

Comment on attachment 396955 [details]
Patch

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

>>> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3005
>>> +            if (sign) {
>> 
>> This logic of if it can fit in a big int 32 should not be here. Instead, we should put that logic in JSC. Maybe even having a constructor on BigInt that tries to return it in inline form if it can.
> 
> Sounds good. As a follow-up patch, I will put this into `JSBigInt` in JSC.

Ah, now I think keeping this here is better. The reason is that Structured-Cloning must have backward-compat format.
This means that this serialization code must work after the change. I think this is better to keep this code in SerializedScriptValue.cpp.
For example, if you put this to JSBigInt.cpp and if you change the code without knowing that the deserialization format must not be changed without changing SerializedScriptValue.cpp's format version etc., then we have a bad time.