CodeGeneratorJS should release the throwScope before doing a void call at end of a function.

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Mark Lam]

<rdar://problem/66272080>

[Mark Lam]

Created attachment 405472 [details]
proposed patch.

[Chris Dumez]

Comment on attachment 405472 [details]
proposed patch.

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

> Source/WebCore/ChangeLog:3
> +        CodeGeneratorJS should release the throwScope before doing a void call at end of a function.

Could you explain in the ChangeLog why?

[Mark Lam]

(In reply to Chris Dumez from comment #2)
> Comment on attachment 405472 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405472&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        CodeGeneratorJS should release the throwScope before doing a void call at end of a function.
> 
> Could you explain in the ChangeLog why?

Thanks for the review.  I'll add a comment in the ChangeLog.

[Mark Lam]

Landed in r265046: <http://trac.webkit.org/r265046>.

[Sam Weinig]

(In reply to Mark Lam from comment #3)
> (In reply to Chris Dumez from comment #2)
> > Comment on attachment 405472 [details]
> > proposed patch.
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=405472&action=review
> > 
> > > Source/WebCore/ChangeLog:3
> > > +        CodeGeneratorJS should release the throwScope before doing a void call at end of a function.
> > 
> > Could you explain in the ChangeLog why?
> 
> Thanks for the review.  I'll add a comment in the ChangeLog.

Hey Mark, I don't understand the explanation in the ChangeLog. Can you give an example of a void function throwing that necessitated this? If there was a bug this was fixing, can it have a test case? I could see this easily being regressed given how non-intuitive this change is.

[Mark Lam]

(In reply to Sam Weinig from comment #5)
> (In reply to Mark Lam from comment #3)
> > (In reply to Chris Dumez from comment #2)
> > > Comment on attachment 405472 [details]
> > > proposed patch.
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=405472&action=review
> > > 
> > > > Source/WebCore/ChangeLog:3
> > > > +        CodeGeneratorJS should release the throwScope before doing a void call at end of a function.
> > > 
> > > Could you explain in the ChangeLog why?
> > 
> > Thanks for the review.  I'll add a comment in the ChangeLog.
> 
> Hey Mark, I don't understand the explanation in the ChangeLog. Can you give
> an example of a void function throwing that necessitated this? If there was
> a bug this was fixing, can it have a test case? I could see this easily
> being regressed given how non-intuitive this change is.

As indicated in the ChangeLog, this change is "covered by existing tests".  Specifically, this test: LayoutTests/webrtc/missing-exception-checks-RTCPeerConnection-generateCertificate.html.

In this case, the potential exception comes from here:

    static inline JSC::EncodedJSValue jsRTCPeerConnectionConstructorFunctionGenerateCertificateBody(JSC::JSGlobalObject* lexicalGlobalObject, JSC::CallFrame* callFrame, Ref<DeferredPromise>&& promise)
    {
        auto& vm = JSC::getVM(lexicalGlobalObject);
        auto throwScope = DECLARE_THROW_SCOPE(vm);
        UNUSED_PARAM(throwScope);
        UNUSED_PARAM(callFrame);
        if (UNLIKELY(callFrame->argumentCount() < 1))
            return throwVMError(lexicalGlobalObject, throwScope, createNotEnoughArgumentsError(lexicalGlobalObject));
        EnsureStillAliveScope argument0 = callFrame->uncheckedArgument(0);
        auto keygenAlgorithm = convert<IDLUnion<IDLObject, IDLDOMString>>(*lexicalGlobalObject, argument0.value());
        RETURN_IF_EXCEPTION(throwScope, encodedJSValue());
        throwScope.release();  // <======= This was added by this patch.
        RTCPeerConnection::generateCertificate(*jsCast<JSDOMGlobalObject*>(lexicalGlobalObject), WTFMove(keygenAlgorithm), WTFMove(promise)); // <== A simulated throw was detected coming from below this call.
        return JSValue::encode(jsUndefined());
    }

[Sam Weinig]

(In reply to Mark Lam from comment #6)
> (In reply to Sam Weinig from comment #5)
> > (In reply to Mark Lam from comment #3)
> > > (In reply to Chris Dumez from comment #2)
> > > > Comment on attachment 405472 [details]
> > > > proposed patch.
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=405472&action=review
> > > > 
> > > > > Source/WebCore/ChangeLog:3
> > > > > +        CodeGeneratorJS should release the throwScope before doing a void call at end of a function.
> > > > 
> > > > Could you explain in the ChangeLog why?
> > > 
> > > Thanks for the review.  I'll add a comment in the ChangeLog.
> > 
> > Hey Mark, I don't understand the explanation in the ChangeLog. Can you give
> > an example of a void function throwing that necessitated this? If there was
> > a bug this was fixing, can it have a test case? I could see this easily
> > being regressed given how non-intuitive this change is.
> 
> As indicated in the ChangeLog, this change is "covered by existing tests". 
> Specifically, this test:
> LayoutTests/webrtc/missing-exception-checks-RTCPeerConnection-
> generateCertificate.html.

Sorry for all the questions, but was that test failing before this change and now it is passing?

> 
> In this case, the potential exception comes from here:
> 
>     static inline JSC::EncodedJSValue
> jsRTCPeerConnectionConstructorFunctionGenerateCertificateBody(JSC::
> JSGlobalObject* lexicalGlobalObject, JSC::CallFrame* callFrame,
> Ref<DeferredPromise>&& promise)
>     {
>         auto& vm = JSC::getVM(lexicalGlobalObject);
>         auto throwScope = DECLARE_THROW_SCOPE(vm);
>         UNUSED_PARAM(throwScope);
>         UNUSED_PARAM(callFrame);
>         if (UNLIKELY(callFrame->argumentCount() < 1))
>             return throwVMError(lexicalGlobalObject, throwScope,
> createNotEnoughArgumentsError(lexicalGlobalObject));
>         EnsureStillAliveScope argument0 = callFrame->uncheckedArgument(0);
>         auto keygenAlgorithm = convert<IDLUnion<IDLObject,
> IDLDOMString>>(*lexicalGlobalObject, argument0.value());
>         RETURN_IF_EXCEPTION(throwScope, encodedJSValue());
>         throwScope.release();  // <======= This was added by this patch.
>        
> RTCPeerConnection::
> generateCertificate(*jsCast<JSDOMGlobalObject*>(lexicalGlobalObject),
> WTFMove(keygenAlgorithm), WTFMove(promise)); // <== A simulated throw was
> detected coming from below this call.
>         return JSValue::encode(jsUndefined());
>     }

In that case, might we want to limit this to cases where we pass some js context information (in that case the global object, but I could imagine a case where we pass the CallFrame as well) to the function?

[Mark Lam]

(In reply to Sam Weinig from comment #7)
> (In reply to Mark Lam from comment #6)
> > (In reply to Sam Weinig from comment #5)
> > > (In reply to Mark Lam from comment #3)
> > > > (In reply to Chris Dumez from comment #2)
> > > > > Comment on attachment 405472 [details]
> > > > > proposed patch.
> > > > > 
> > > > > View in context:
> > > > > https://bugs.webkit.org/attachment.cgi?id=405472&action=review
> > > > > 
> > > > > > Source/WebCore/ChangeLog:3
> > > > > > +        CodeGeneratorJS should release the throwScope before doing a void call at end of a function.
> > > > > 
> > > > > Could you explain in the ChangeLog why?
> > > > 
> > > > Thanks for the review.  I'll add a comment in the ChangeLog.
> > > 
> > > Hey Mark, I don't understand the explanation in the ChangeLog. Can you give
> > > an example of a void function throwing that necessitated this? If there was
> > > a bug this was fixing, can it have a test case? I could see this easily
> > > being regressed given how non-intuitive this change is.
> > 
> > As indicated in the ChangeLog, this change is "covered by existing tests". 
> > Specifically, this test:
> > LayoutTests/webrtc/missing-exception-checks-RTCPeerConnection-
> > generateCertificate.html.
> 
> Sorry for all the questions, but was that test failing before this change
> and now it is passing?

Yes, An Apple engineer told me that this test regressed and I investigated, leading to this fix.

> > In this case, the potential exception comes from here:
> > 
> >     static inline JSC::EncodedJSValue
> > jsRTCPeerConnectionConstructorFunctionGenerateCertificateBody(JSC::
> > JSGlobalObject* lexicalGlobalObject, JSC::CallFrame* callFrame,
> > Ref<DeferredPromise>&& promise)
> >     {
> >         auto& vm = JSC::getVM(lexicalGlobalObject);
> >         auto throwScope = DECLARE_THROW_SCOPE(vm);
> >         UNUSED_PARAM(throwScope);
> >         UNUSED_PARAM(callFrame);
> >         if (UNLIKELY(callFrame->argumentCount() < 1))
> >             return throwVMError(lexicalGlobalObject, throwScope,
> > createNotEnoughArgumentsError(lexicalGlobalObject));
> >         EnsureStillAliveScope argument0 = callFrame->uncheckedArgument(0);
> >         auto keygenAlgorithm = convert<IDLUnion<IDLObject,
> > IDLDOMString>>(*lexicalGlobalObject, argument0.value());
> >         RETURN_IF_EXCEPTION(throwScope, encodedJSValue());
> >         throwScope.release();  // <======= This was added by this patch.
> >        
> > RTCPeerConnection::
> > generateCertificate(*jsCast<JSDOMGlobalObject*>(lexicalGlobalObject),
> > WTFMove(keygenAlgorithm), WTFMove(promise)); // <== A simulated throw was
> > detected coming from below this call.
> >         return JSValue::encode(jsUndefined());
> >     }
> 
> In that case, might we want to limit this to cases where we pass some js
> context information (in that case the global object, but I could imagine a
> case where we pass the CallFrame as well) to the function?

As a convention, we don't pass CallFrame anymore.  Instead, we pass the globalObject, which is what you see being passed here.

There might be cases where the last called function couldn't actually throw anything.  Releasing the ThrowScope before such a call at the end is still valid to do though perhaps unnecessary.  We can certainly add more smarts into CodeGeneratorJS to detect all these special cases, but then, one would have to modify CodeGeneratorJS to do that ... which is not a cheap price to pay (and I already did quite a bit of this recently for handling ThrowScopes with some degree of smarts).  I'm not sure the tradeoff is worth it to more at this time.

My vote is to replace CodeGeneeratorJS.pm with something more sane that will allow us to generate better code ... but that's a big project to be explored in the future.

[Ryan Haddad]

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

[Sam Weinig]

(In reply to Mark Lam from comment #8)
> (In reply to Sam Weinig from comment #7)
> > (In reply to Mark Lam from comment #6)
> > > (In reply to Sam Weinig from comment #5)
> > > > (In reply to Mark Lam from comment #3)
> > > > > (In reply to Chris Dumez from comment #2)
> > > > > > Comment on attachment 405472 [details]
> > > > > > proposed patch.
> > > > > > 
> > > > > > View in context:
> > > > > > https://bugs.webkit.org/attachment.cgi?id=405472&action=review
> > > > > > 
> > > > > > > Source/WebCore/ChangeLog:3
> > > > > > > +        CodeGeneratorJS should release the throwScope before doing a void call at end of a function.
> > > > > > 
> > > > > > Could you explain in the ChangeLog why?
> > > > > 
> > > > > Thanks for the review.  I'll add a comment in the ChangeLog.
> > > > 
> > > > Hey Mark, I don't understand the explanation in the ChangeLog. Can you give
> > > > an example of a void function throwing that necessitated this? If there was
> > > > a bug this was fixing, can it have a test case? I could see this easily
> > > > being regressed given how non-intuitive this change is.
> > > 
> > > As indicated in the ChangeLog, this change is "covered by existing tests". 
> > > Specifically, this test:
> > > LayoutTests/webrtc/missing-exception-checks-RTCPeerConnection-
> > > generateCertificate.html.
> > 
> > Sorry for all the questions, but was that test failing before this change
> > and now it is passing?
> 
> Yes, An Apple engineer told me that this test regressed and I investigated,
> leading to this fix.
> 
> > > In this case, the potential exception comes from here:
> > > 
> > >     static inline JSC::EncodedJSValue
> > > jsRTCPeerConnectionConstructorFunctionGenerateCertificateBody(JSC::
> > > JSGlobalObject* lexicalGlobalObject, JSC::CallFrame* callFrame,
> > > Ref<DeferredPromise>&& promise)
> > >     {
> > >         auto& vm = JSC::getVM(lexicalGlobalObject);
> > >         auto throwScope = DECLARE_THROW_SCOPE(vm);
> > >         UNUSED_PARAM(throwScope);
> > >         UNUSED_PARAM(callFrame);
> > >         if (UNLIKELY(callFrame->argumentCount() < 1))
> > >             return throwVMError(lexicalGlobalObject, throwScope,
> > > createNotEnoughArgumentsError(lexicalGlobalObject));
> > >         EnsureStillAliveScope argument0 = callFrame->uncheckedArgument(0);
> > >         auto keygenAlgorithm = convert<IDLUnion<IDLObject,
> > > IDLDOMString>>(*lexicalGlobalObject, argument0.value());
> > >         RETURN_IF_EXCEPTION(throwScope, encodedJSValue());
> > >         throwScope.release();  // <======= This was added by this patch.
> > >        
> > > RTCPeerConnection::
> > > generateCertificate(*jsCast<JSDOMGlobalObject*>(lexicalGlobalObject),
> > > WTFMove(keygenAlgorithm), WTFMove(promise)); // <== A simulated throw was
> > > detected coming from below this call.
> > >         return JSValue::encode(jsUndefined());
> > >     }
> > 
> > In that case, might we want to limit this to cases where we pass some js
> > context information (in that case the global object, but I could imagine a
> > case where we pass the CallFrame as well) to the function?
> 
> As a convention, we don't pass CallFrame anymore.  Instead, we pass the
> globalObject, which is what you see being passed here.
> 
> There might be cases where the last called function couldn't actually throw
> anything.  Releasing the ThrowScope before such a call at the end is still
> valid to do though perhaps unnecessary.  We can certainly add more smarts
> into CodeGeneratorJS to detect all these special cases, but then, one would
> have to modify CodeGeneratorJS to do that ... which is not a cheap price to
> pay (and I already did quite a bit of this recently for handling ThrowScopes
> with some degree of smarts).  I'm not sure the tradeoff is worth it to more
> at this time.
> 
> My vote is to replace CodeGeneeratorJS.pm with something more sane that will
> allow us to generate better code ... but that's a big project to be explored
> in the future.

Thanks for the details. In the future, if you could include information, like which test was failing and that this fixes it (and, what caused the regression if it is known), in the bug and changelog, that would be very helpful.