Bug 214680 - Add exception check for WebCore createRejectedPromiseWithTypeError
Summary: Add exception check for WebCore createRejectedPromiseWithTypeError
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on: 214698 214700
Blocks:
  Show dependency treegraph
 
Reported: 2020-07-23 03:41 PDT by Yusuke Suzuki
Modified: 2020-07-23 14:24 PDT (History)
2 users (show)

See Also:


Attachments
Patch (7.72 KB, patch)
2020-07-23 03:42 PDT, Yusuke Suzuki
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2020-07-23 03:41:43 PDT
Add exception check for WebCore createRejectedPromiseWithTypeError
Comment 1 Yusuke Suzuki 2020-07-23 03:42:22 PDT
Created attachment 405030 [details]
Patch
Comment 2 Yusuke Suzuki 2020-07-23 03:42:25 PDT
<rdar://problem/65925490>
Comment 3 Mark Lam 2020-07-23 10:58:11 PDT
Comment on attachment 405030 [details]
Patch

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

r=me with fix.

> Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp:228
> +    auto rejectFunction = promiseConstructor->get(&lexicalGlobalObject, vm.propertyNames->builtinNames().rejectPrivateName());
> +    EXCEPTION_ASSERT(!scope.exception() || isTerminatedExecutionException(vm, scope.exception()));

Are you sure that getting the property with rejectPrivateName() will never throw?   I see that it can be lazily generated using promiseConstructorRejectCodeGenerator().  Can this trigger an OOME / StackOverflow?  Regardless, I think you need a RETURN_IF_EXCEPTION() after this.  Even if we're seeing a termination exception, we still need to bail.
Comment 4 Yusuke Suzuki 2020-07-23 13:17:33 PDT
Comment on attachment 405030 [details]
Patch

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

>> Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp:228
>> +    EXCEPTION_ASSERT(!scope.exception() || isTerminatedExecutionException(vm, scope.exception()));
> 
> Are you sure that getting the property with rejectPrivateName() will never throw?   I see that it can be lazily generated using promiseConstructorRejectCodeGenerator().  Can this trigger an OOME / StackOverflow?  Regardless, I think you need a RETURN_IF_EXCEPTION() after this.  Even if we're seeing a termination exception, we still need to bail.

OK, maybe, just using RETURN_IF_EXCEPTION() is better. Changed.
Comment 5 Yusuke Suzuki 2020-07-23 13:37:47 PDT
Committed r264788: <https://trac.webkit.org/changeset/264788>
Comment 6 Yusuke Suzuki 2020-07-23 14:17:08 PDT
Re-opened since this is blocked by bug 214698