Bug 214695

Summary: Fix JS bindings code to DECLARE_THROW_SCOPE in the functions that can throw instead of passing a ThrowScope around.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: WebCore JavaScriptAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, beidson, cdumez, ews-watchlist, jsbell, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch.
none
proposed patch.
none
proposed patch. none

Description Mark Lam 2020-07-23 11:30:49 PDT
<rdar://problem/65927049>
Comment 1 Mark Lam 2020-07-23 11:33:32 PDT
Created attachment 405061 [details]
proposed patch.
Comment 2 Yusuke Suzuki 2020-07-23 11:35:48 PDT
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?
Comment 3 Mark Lam 2020-07-23 11:38:11 PDT
(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.
Comment 4 Mark Lam 2020-07-23 20:59:58 PDT
Created attachment 405125 [details]
proposed patch.
Comment 5 Mark Lam 2020-07-23 21:42:40 PDT
Comment on attachment 405125 [details]
proposed patch.

The test failures look unrelated.  Let's get a review.
Comment 6 Yusuke Suzuki 2020-07-23 21:52:30 PDT
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, ...)?
Comment 7 Mark Lam 2020-07-24 09:05:48 PDT
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.
Comment 8 Mark Lam 2020-07-24 09:24:45 PDT
Created attachment 405153 [details]
proposed patch.
Comment 9 Yusuke Suzuki 2020-07-24 13:37:32 PDT
Comment on attachment 405153 [details]
proposed patch.

r=me
Comment 10 Mark Lam 2020-07-24 13:40:30 PDT
Comment on attachment 405153 [details]
proposed patch.

Thanks for the review.  Landing now.
Comment 11 EWS 2020-07-24 14:02:04 PDT
Committed r264855: <https://trac.webkit.org/changeset/264855>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405153 [details].
Comment 12 Yusuke Suzuki 2020-07-24 14:50:50 PDT
*** Bug 214714 has been marked as a duplicate of this bug. ***