Bug 214695 – Fix JS bindings code to DECLARE_THROW_SCOPE in the functions that can throw instead of passing a ThrowScope around.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Mark Lam]

<rdar://problem/65927049>

[Mark Lam]

Created attachment 405061 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 405061 [details]
proposed patch.

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

> Source/WebCore/bindings/js/JSDOMOperation.h:53
> +        RELEASE_AND_RETURN(throwScope, (operation(&lexicalGlobalObject, &callFrame, thisObject, throwScope)));

If we use `RELEASE_AND_RETURN` here, I think it releases throwScope while we are passing it to operation.
Should `operation` create its own ThrowScope instead of passing it here?

[Mark Lam]

(In reply to Yusuke Suzuki from comment #2)
> If we use `RELEASE_AND_RETURN` here, I think it releases throwScope while we
> are passing it to operation.
> Should `operation` create its own ThrowScope instead of passing it here?

Good point.  Let me investigate.

[Mark Lam]

Created attachment 405125 [details]
proposed patch.

[Mark Lam]

Comment on attachment 405125 [details]
proposed patch.

The test failures look unrelated.  Let's get a review.

[Yusuke Suzuki]

Comment on attachment 405125 [details]
proposed patch.

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

> Source/WebCore/bindings/js/JSEventTargetCustom.h:77
> +        return operation(&lexicalGlobalObject, &callFrame, thisObject.get());

RELEASE_AND_RETURN(throwScope, ...)?

> Source/WebCore/bindings/scripts/test/JS/JSMapLike.cpp:220
>      JSValue result = toJS<IDLAny>(lexicalGlobalObject, throwScope, forwardSizeToMapLike(lexicalGlobalObject, thisObject));

Should we insert exception check after forwardSizeToMapLike(lexicalGlobalObject, thisObject)? Or should we release scope?

> Source/WebCore/bindings/scripts/test/JS/JSTestActiveDOMObject.cpp:326
> +        return jsTestActiveDOMObjectPrototypeFunctionOverloadedMethod1Body(lexicalGlobalObject, callFrame, castedThis);

Should we use RELEASE_AND_RETURN(throwScope, ...)?

[Mark Lam]

Comment on attachment 405125 [details]
proposed patch.

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

>> Source/WebCore/bindings/js/JSEventTargetCustom.h:77
>> +        return operation(&lexicalGlobalObject, &callFrame, thisObject.get());
> 
> RELEASE_AND_RETURN(throwScope, ...)?

Fixed.

>> Source/WebCore/bindings/scripts/test/JS/JSMapLike.cpp:220
>>      JSValue result = toJS<IDLAny>(lexicalGlobalObject, throwScope, forwardSizeToMapLike(lexicalGlobalObject, thisObject));
> 
> Should we insert exception check after forwardSizeToMapLike(lexicalGlobalObject, thisObject)? Or should we release scope?

For now, I'm just going to wrap RELEASE_AND_RETURN around toJS.  Looking at the implementation of toJS, it appears to expect that exceptions are already handled and is passed to it via an ExceptionOr.  If the ExceptionOr presents an exception, it will throw using the passed in throwScope.  The RELEASE_AND_RETURN works well with this model.  If we find evidence that we need to do more, I can look into additional changes then.

BTW, I did try breaking out the call to forwardSizeToMapLike() (or whatever is used to compute a value) and stashing its result in an auto temp before calling toJS with the temp.  It turns out that that this is not a workable idiom some of the return values are not copyable, and we can't always take a reference to the result.  Basically, I could not find a uniform way type correct way to stashing the temp result.  The correct solution would be to do the exception check inside toJS() which is what lead me to studying how toJS() works.

TLDR: I'm just going to wrap RELEASE_AND_RETURN around toJS for now.

>> Source/WebCore/bindings/scripts/test/JS/JSTestActiveDOMObject.cpp:326
>> +        return jsTestActiveDOMObjectPrototypeFunctionOverloadedMethod1Body(lexicalGlobalObject, callFrame, castedThis);
> 
> Should we use RELEASE_AND_RETURN(throwScope, ...)?

Fixed.

[Mark Lam]

Created attachment 405153 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 405153 [details]
proposed patch.

r=me

[Mark Lam]

Comment on attachment 405153 [details]
proposed patch.

Thanks for the review.  Landing now.

[EWS]

Committed r264855: <https://trac.webkit.org/changeset/264855>

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

[Yusuke Suzuki]

*** Bug 214714 has been marked as a duplicate of this bug. ***