Bug 210728

Summary: StructuredClone algorithm should be aware of BigInt
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: DOMAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, annulen, beidson, ews-watchlist, gyuyoung.kim, jsbell, keith_miller, mark.lam, msaboff, ryuan.choi, sbarati, sergio, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=210731
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Yusuke Suzuki 2020-04-19 18:18:05 PDT
...
Comment 1 Yusuke Suzuki 2020-04-19 21:59:29 PDT
Created attachment 396940 [details]
Patch

WIP
Comment 2 Mark Lam 2020-04-19 22:24:27 PDT
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?
Comment 3 Yusuke Suzuki 2020-04-19 22:26:29 PDT
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.
Comment 4 Mark Lam 2020-04-19 22:50:55 PDT
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?
Comment 5 Mark Lam 2020-04-19 23:27:49 PDT
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.
Comment 6 Yusuke Suzuki 2020-04-19 23:50:35 PDT
Created attachment 396950 [details]
Patch
Comment 7 Mark Lam 2020-04-19 23:59:05 PDT
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).
Comment 8 Yusuke Suzuki 2020-04-20 00:01:41 PDT
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.
Comment 9 Yusuke Suzuki 2020-04-20 00:03:45 PDT
Created attachment 396952 [details]
Patch

Patch for landing, seeing EWS
Comment 10 Yusuke Suzuki 2020-04-20 00:21:12 PDT
Created attachment 396955 [details]
Patch

Patch for landing, watching EWS, fixing GCC build failure
Comment 11 Radar WebKit Bug Importer 2020-04-20 01:45:07 PDT
<rdar://problem/62048672>
Comment 12 Yusuke Suzuki 2020-04-20 02:51:30 PDT
WK1, WK2 succeeds. And in Debug, only one test is failing (crashing) and it is crashing before this patch according to history. Landing.
Comment 13 Yusuke Suzuki 2020-04-20 02:54:40 PDT
Committed r260358: <https://trac.webkit.org/changeset/260358>
Comment 14 Saam Barati 2020-04-20 09:44:39 PDT
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)
Comment 15 Saam Barati 2020-04-20 09:50:39 PDT
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?
Comment 16 Mark Lam 2020-04-20 09:53:40 PDT
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?
Comment 17 Mark Lam 2020-04-20 09:54:24 PDT
(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.
Comment 18 Yusuke Suzuki 2020-04-20 10:12:52 PDT
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.
Comment 19 Yusuke Suzuki 2020-04-20 10:29:01 PDT
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.