<rdar://problem/65927049>
Created attachment 405061 [details] proposed patch.
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?
(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.
Created attachment 405125 [details] proposed patch.
Comment on attachment 405125 [details] proposed patch. The test failures look unrelated. Let's get a review.
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 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.
Created attachment 405153 [details] proposed patch.
Comment on attachment 405153 [details] proposed patch. r=me
Comment on attachment 405153 [details] proposed patch. Thanks for the review. Landing now.
Committed r264855: <https://trac.webkit.org/changeset/264855> All reviewed patches have been landed. Closing bug and clearing flags on attachment 405153 [details].
*** Bug 214714 has been marked as a duplicate of this bug. ***