Bug 212512

Summary: [JSC] JSBigInt allocation should be graceful for OOM
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, beidson, ews-watchlist, jsbell, keith_miller, mark.lam, msaboff, saam, ticaiolima, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
mark.lam: review+
Patch
none
Patch
none
Patch for landing none

Description Yusuke Suzuki 2020-05-28 23:40:47 PDT 
<rdar://problem/63600600>
Comment 1 Yusuke Suzuki 2020-05-28 23:57:40 PDT 
Created attachment 400553 [details]
Patch
Comment 2 Yusuke Suzuki 2020-05-29 00:06:55 PDT 
Created attachment 400554 [details]
Patch
Comment 3 Yusuke Suzuki 2020-05-29 00:12:53 PDT 
Created attachment 400555 [details]
Patch
Comment 4 Yusuke Suzuki 2020-05-29 00:22:41 PDT 
Created attachment 400556 [details]
Patch
Comment 5 Yusuke Suzuki 2020-05-29 00:33:03 PDT 
Created attachment 400557 [details]
Patch
Comment 6 Mark Lam 2020-05-29 00:35:46 PDT 
Comment on attachment 400554 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSBigInt.cpp:85
> +inline JSBigInt* JSBigInt::createZero(JSGlobalObject* nullOrGlobalObjectForOOM, VM& vm)

Use Optional?

> Source/JavaScriptCore/runtime/JSBigInt.cpp:95
> +inline JSBigInt* JSBigInt::createWithLength(JSGlobalObject* nullOrGlobalObjectForOOM, VM& vm, unsigned length)

Is there a good reason to not use Optional<JSGlobalObject> here?

> Source/JavaScriptCore/runtime/JSBigInt.cpp:126
> +inline JSBigInt* JSBigInt::createFrom(JSGlobalObject* nullOrGlobalObjectForOOM, VM& vm, int32_t value)

Ditto: use Optional here?

> Source/JavaScriptCore/runtime/JSBigInt.cpp:318
> +JSValue JSBigInt::parseInt(JSGlobalObject* nullOrGlobalObjectForOOM, VM& vm, StringView s, uint8_t radix, ErrorParseMode parserMode, ParseIntSign sign)

Ditto.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:1541
> +bool JSBigInt::absoluteDivWithDigitDivisor(JSGlobalObject* nullOrGlobalObjectForOOM, VM& vm, BigIntImpl x, Digit divisor, JSBigInt** quotient, Digit& remainder)

Ditto.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:2155
> +String JSBigInt::toStringBasePowerOfTwo(VM& vm, JSGlobalObject* nullOrGlobalObjectForOOM, JSBigInt* x, unsigned radix)

Ditto.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:2304
> +JSBigInt* JSBigInt::rightTrim(JSGlobalObject* nullOrGlobalObjectForOOM, VM& vm)

Ditto.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:2345
> +JSBigInt* JSBigInt::allocateFor(JSGlobalObject* nullOrGlobalObjectForOOM, VM& vm, unsigned radix, unsigned charcount)

Ditto.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:2427
> +JSValue JSBigInt::parseInt(JSGlobalObject* nullOrGlobalObjectForOOM, VM& vm, CharType* data, unsigned length, unsigned startIndex, unsigned radix, ErrorParseMode errorParseMode, ParseIntSign sign, ParseIntMode parseMode)

Ditto.
Comment 7 Mark Lam 2020-05-29 00:38:07 PDT 
Comment on attachment 400557 [details]
Patch

r=me if EWS bots are green.
Comment 8 Yusuke Suzuki 2020-05-29 01:48:39 PDT 
Comment on attachment 400554 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:85
>> +inline JSBigInt* JSBigInt::createZero(JSGlobalObject* nullOrGlobalObjectForOOM, VM& vm)
> 
> Use Optional?

For now, I would like to keep the consistency with JSString's `JSGlobalObject* nullOrGlobalObjectForOOM`. We should consider introducing this, but I would like to do that at once.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:95
>> +inline JSBigInt* JSBigInt::createWithLength(JSGlobalObject* nullOrGlobalObjectForOOM, VM& vm, unsigned length)
> 
> Is there a good reason to not use Optional<JSGlobalObject> here?

Ditto.
Comment 9 Yusuke Suzuki 2020-05-29 01:50:14 PDT 
Created attachment 400559 [details]
Patch
Comment 10 Yusuke Suzuki 2020-05-29 03:07:29 PDT 
Created attachment 400563 [details]
Patch
Comment 11 Caio Lima 2020-05-29 08:43:40 PDT 
Comment on attachment 400563 [details]
Patch

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

LGTM as well. I pointed out the issue breaking on 32-bits.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:2558
> +                    RELEASE_AND_RETURN(scope, JSBigInt::createFrom(nullOrGlobalObjectForOOM, static_cast<int32_t>(maybeResult)));

This call will cause segfault when globalObject is `nullptr`. This happens when `parseInt` is called  from `BytecodeGenerator.cpp`, for instance. This is one of the reasons of failures on ARMv7 and MIPS bots.
Comment 12 Yusuke Suzuki 2020-05-29 17:13:35 PDT 
Comment on attachment 400563 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:2558
>> +                    RELEASE_AND_RETURN(scope, JSBigInt::createFrom(nullOrGlobalObjectForOOM, static_cast<int32_t>(maybeResult)));
> 
> This call will cause segfault when globalObject is `nullptr`. This happens when `parseInt` is called  from `BytecodeGenerator.cpp`, for instance. This is one of the reasons of failures on ARMv7 and MIPS bots.

Nice catch. We should call `JSBigInt::createFrom(nullOrGlobalObjectForOOM, vm, static_cast<int32_t>(maybeResult))` instead.
Comment 13 Yusuke Suzuki 2020-05-29 17:27:38 PDT 
Created attachment 400639 [details]
Patch for landing
Comment 14 Yusuke Suzuki 2020-05-30 00:46:59 PDT 
Committed r262342: <https://trac.webkit.org/changeset/262342>
Comment 15 Yusuke Suzuki 2020-05-30 20:01:14 PDT 
Committed r262353: <https://trac.webkit.org/changeset/262353>