Bug 225128 – Fix exception assertions in light of the TerminationException.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

rdar://76694909

[Mark Lam]

Created attachment 427214 [details]
proposed patch.

[Mark Lam]

Created attachment 427218 [details]
proposed patch.

[Robin Morisset]

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?

[Mark Lam]

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.

[Mark Lam]

(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.

[Mark Lam]

Created attachment 427226 [details]
proposed patch.

[Robin Morisset]

Comment on attachment 427226 [details]
proposed patch.

r=me
Thanks for the explanation.

[Mark Lam]

Thanks for the review.  Landed in r276719: <http://trac.webkit.org/r276719>.