Summary: | StructuredClone algorithm should be aware of BigInt | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||||
Component: | DOM | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | alecflett, annulen, beidson, ews-watchlist, gyuyoung.kim, jsbell, keith_miller, mark.lam, msaboff, ryuan.choi, saam, 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
Yusuke Suzuki
2020-04-19 18:18:05 PDT
Created attachment 396940 [details]
Patch
WIP
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 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 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 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. Created attachment 396950 [details]
Patch
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 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. Created attachment 396952 [details]
Patch
Patch for landing, seeing EWS
Created attachment 396955 [details]
Patch
Patch for landing, watching EWS, fixing GCC build failure
WK1, WK2 succeeds. And in Debug, only one test is failing (crashing) and it is crashing before this patch according to history. Landing. Committed r260358: <https://trac.webkit.org/changeset/260358> 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 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 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? (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 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 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. |