Bug 214610

Summary: We should have exception check after promise operation
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: mark.lam, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
youennf: review+
Patch for landing none

Description Yusuke Suzuki 2020-07-21 12:20:52 PDT
We should have exception check after promise operation
Comment 1 Yusuke Suzuki 2020-07-21 12:21:40 PDT
Created attachment 404849 [details]
Patch
Comment 2 Yusuke Suzuki 2020-07-21 12:21:42 PDT
<rdar://problem/65881794>
Comment 3 Mark Lam 2020-07-21 12:27:16 PDT
Comment on attachment 404849 [details]
Patch

r=me
Comment 4 Yusuke Suzuki 2020-07-23 03:12:42 PDT
Created attachment 405029 [details]
Patch
Comment 5 youenn fablet 2020-07-23 05:53:03 PDT
Comment on attachment 405029 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp:187
> +    EXCEPTION_ASSERT(!scope.exception() || isTerminatedExecutionException(lexicalGlobalObject.vm(), scope.exception()));

Should we add this line in other places like reject(Exception...)?
Comment 6 Mark Lam 2020-07-23 06:55:51 PDT
Comment on attachment 405029 [details]
Patch

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

>> Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp:187
>> +    EXCEPTION_ASSERT(!scope.exception() || isTerminatedExecutionException(lexicalGlobalObject.vm(), scope.exception()));
> 
> Should we add this line in other places like reject(Exception...)?

Let's only do this as needed for now.  I think we need a better mental model for TerminatedExecutionExceptions, OOMEs, and StackOverflowErrors better for CatchScope.  We may want to replace these eventually.  No need to do additional work until we have the better model.

> LayoutTests/js/dom/promise-should-have-exception-check-on-operation.html:9
> +SubtleCrypto.prototype.deriveBits();

From the win EWS, it looks like SubtleCrypto isn't available on OS(Windows).  Can you confirm if this is the case, and if so, add a TestExpectation for Windows port to expect this test to fail?
Comment 7 Mark Lam 2020-07-23 06:56:07 PDT
r=me too.
Comment 8 Yusuke Suzuki 2020-07-23 11:44:10 PDT
Comment on attachment 405029 [details]
Patch

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

>> LayoutTests/js/dom/promise-should-have-exception-check-on-operation.html:9
>> +SubtleCrypto.prototype.deriveBits();
> 
> From the win EWS, it looks like SubtleCrypto isn't available on OS(Windows).  Can you confirm if this is the case, and if so, add a TestExpectation for Windows port to expect this test to fail?

Nice catch. I'll fix it.
Comment 9 Yusuke Suzuki 2020-07-23 13:46:26 PDT
Created attachment 405072 [details]
Patch for landing
Comment 10 Yusuke Suzuki 2020-07-23 15:32:30 PDT
webaudio/webaudio-gc.html crashes but it was before this patch.
Comment 11 EWS 2020-07-23 15:35:10 PDT
Committed r264802: <https://trac.webkit.org/changeset/264802>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405072 [details].