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

Yusuke Suzuki
Reported 2020-07-21 12:20:52 PDT
We should have exception check after promise operation
Attachments
Patch (6.33 KB, patch)
2020-07-21 12:21 PDT, Yusuke Suzuki
no flags
Patch (4.55 KB, patch)
2020-07-23 03:12 PDT, Yusuke Suzuki
youennf: review+
Patch for landing (4.61 KB, patch)
2020-07-23 13:46 PDT, Yusuke Suzuki
no flags
Yusuke Suzuki
Comment 1 2020-07-21 12:21:40 PDT
Yusuke Suzuki
Comment 2 2020-07-21 12:21:42 PDT
Mark Lam
Comment 3 2020-07-21 12:27:16 PDT
Comment on attachment 404849 [details] Patch r=me
Yusuke Suzuki
Comment 4 2020-07-23 03:12:42 PDT
youenn fablet
Comment 5 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...)?
Mark Lam
Comment 6 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?
Mark Lam
Comment 7 2020-07-23 06:56:07 PDT
r=me too.
Yusuke Suzuki
Comment 8 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.
Yusuke Suzuki
Comment 9 2020-07-23 13:46:26 PDT
Created attachment 405072 [details] Patch for landing
Yusuke Suzuki
Comment 10 2020-07-23 15:32:30 PDT
webaudio/webaudio-gc.html crashes but it was before this patch.
EWS
Comment 11 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].
Note You need to log in before you can comment on or make changes to this bug.