rdar://76694909
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>.