Summary: | Fix exception assertions in light of the TerminationException. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||||
Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | alecflett, beidson, benjamin, calvaris, cdumez, changseok, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, hi, jer.noble, joepeck, jsbell, keith_miller, mkwst, msaboff, philipj, rmorisset, saam, sergio, tzagallo, webkit-bug-importer, youennf | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=225132 | ||||||||||
Attachments: |
|
Description
Mark Lam
2021-04-27 16:50:46 PDT
Created attachment 427214 [details]
proposed patch.
Created attachment 427218 [details]
proposed patch.
Comment on attachment 427218 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=427218&action=review Have you benchmarked the effect on JetStream2? While the many DeferTermination don't appear to be on any hot path it would be reassuring to verify it. > Source/WebCore/bindings/js/JSDOMExceptionHandling.cpp:192 > + throwScope.assertNoExceptionExceptTermination(); What happens if you throw a regular exception while a termination exception is pending? Is one of the two lost? > Source/WebCore/contentextensions/ContentExtensionParser.cpp:107 > + if (scope.exception() || !isJSArray(object)) Maybe keep a scope.assertNoExceptionExceptTermination() here? It seems like you are converting an assert failure into a regular return in that error case. > Source/WebCore/html/HTMLMediaElement.cpp:7357 > + RETURN_IF_EXCEPTION(scope, reportExceptionAndReturnFalse()); In this file you sometimes just return false, and sometimes first clear and report the exception. Is it on purpose, and can you add a short comment explaining why? Comment on attachment 427218 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=427218&action=review >> Source/WebCore/bindings/js/JSDOMExceptionHandling.cpp:192 >> + throwScope.assertNoExceptionExceptTermination(); > > What happens if you throw a regular exception while a termination exception is pending? Is one of the two lost? TerminationException takes precedence. See the lowest level VM::throwException() that does the actual throw. >> Source/WebCore/contentextensions/ContentExtensionParser.cpp:107 >> + if (scope.exception() || !isJSArray(object)) > > Maybe keep a scope.assertNoExceptionExceptTermination() here? It seems like you are converting an assert failure into a regular return in that error case. Good catch. I missed that this follows a `typeValue.isObject()` check right above it. Hence, this toObject() is guaranteed to do just a pointer cast and not come across any RETURN_IF_EXCEPTION on the way. This means I can undo this change and keep the original assertion. >> Source/WebCore/html/HTMLMediaElement.cpp:7357 >> + RETURN_IF_EXCEPTION(scope, reportExceptionAndReturnFalse()); > > In this file you sometimes just return false, and sometimes first clear and report the exception. Is it on purpose, and can you add a short comment explaining why? This lambda function declares a CatchScope and always clears the exception and return false if an exception is found. The other 2 changes in this file is for a different function with a ThrowScope that propagates the exception (i.e. returns false without clearing the exception). That said, I should ensure that the TerminationException isn't just eaten without terminating. However, if that is an issue, it is a pre-existing one. This line of change here behaves consistently with the exception handling in other places in this function. I filed https://bugs.webkit.org/show_bug.cgi?id=225132 to track the review of HTMLMediaElement::didAddUserAgentShadowRoot() for proper handling of termination later. (In reply to Robin Morisset from comment #3) > Comment on attachment 427218 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427218&action=review > > Have you benchmarked the effect on JetStream2? While the many > DeferTermination don't appear to be on any hot path it would be reassuring > to verify it. I have not, but these changes are not expected to have any measurable performance cost. My plan is to land the change and watch the bots. We can always roll out / fix if we find a regression. Created attachment 427226 [details]
proposed patch.
Comment on attachment 427226 [details]
proposed patch.
r=me
Thanks for the explanation.
Thanks for the review. Landed in r276719: <http://trac.webkit.org/r276719>. |