Bug 212512 – [JSC] JSBigInt allocation should be graceful for OOM

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

<rdar://problem/63600600>

[Yusuke Suzuki]

Created attachment 400553 [details]
Patch

[Yusuke Suzuki]

Created attachment 400554 [details]
Patch

[Yusuke Suzuki]

Created attachment 400555 [details]
Patch

[Yusuke Suzuki]

Created attachment 400556 [details]
Patch

[Yusuke Suzuki]

Created attachment 400557 [details]
Patch

[Mark Lam]

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.

[Mark Lam]

Comment on attachment 400557 [details]
Patch

r=me if EWS bots are green.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 400559 [details]
Patch

[Yusuke Suzuki]

Created attachment 400563 [details]
Patch

[Caio Lima]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 400639 [details]
Patch for landing

[Yusuke Suzuki]

Committed r262342: <https://trac.webkit.org/changeset/262342>

[Yusuke Suzuki]

Committed r262353: <https://trac.webkit.org/changeset/262353>