Bug 162584

Summary: Add some needed CatchScopes in code that should not throw.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, fpizlo, ggaren, jfbastien, keith_miller, msaboff, ryanhaddad, saam
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 162588    
Bug Blocks: 162351    
Attachments:
Description Flags
proposed patch.
keith_miller: review+
Patch for re-landing. none

Description Mark Lam 2016-09-26 16:39:05 PDT 
Patch coming.
Comment 1 Mark Lam 2016-09-26 16:42:29 PDT 
Created attachment 289893 [details]
proposed patch.
Comment 2 Keith Miller 2016-09-26 16:50:14 PDT 
Comment on attachment 289893 [details]
proposed patch.

r=me.
Comment 3 Mark Lam 2016-09-26 17:00:09 PDT 
Thanks for the review.  I fixed a typo in the ChangeLog before landing.

Landed in r206405: <http://trac.webkit.org/r206405>.
Comment 4 Geoffrey Garen 2016-09-26 17:19:37 PDT 
Comment on attachment 289893 [details]
proposed patch.

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

> Source/JavaScriptCore/jsc.cpp:2549
> +    RETURN_IF_EXCEPTION(scope, 3);

How can GlobalObject::create throw?
Comment 5 Ryan Haddad 2016-09-26 19:38:05 PDT 
This change appears to have caused two LayoutTests to crash (EWS seems to have caught this, but the patch was landed before results could be verified).

plugins/npruntime/object-from-destroyed-plugin.html
plugins/npruntime/object-from-destroyed-plugin-in-subframe.html

https://build.webkit.org/results/Apple%20El%20Capitan%20Release%20WK2%20(Tests)/r206406%20(9738)/results.html
Comment 6 WebKit Commit Bot 2016-09-26 19:41:49 PDT 
Re-opened since this is blocked by bug 162588
Comment 7 Mark Lam 2016-09-27 11:41:07 PDT 
Comment on attachment 289893 [details]
proposed patch.

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

>> Source/JavaScriptCore/jsc.cpp:2549
>> +    RETURN_IF_EXCEPTION(scope, 3);
> 
> How can GlobalObject::create throw?

I forgot the details of what motivated this change in the first place.  I'll leave it out for now until the motivation arises again when I turn on exception check verification.

> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:246
> +    VM& vm = exec->vm();
> +    auto scope = DECLARE_CATCH_SCOPE(vm);
> +
>      String errorMessage = makeString(errorDescriptionForValue(exec, value)->value(exec), ' ', message);
> +    if (UNLIKELY(scope.exception())) {
> +        scope.clearException();
> +        errorMessage = message;
> +    }

This change appears to need additional support from exception checks that are missing.  I will leave it out for now until I have the missing exception checks in place.
Comment 8 Mark Lam 2016-09-27 11:53:47 PDT 
Created attachment 289989 [details]
Patch for re-landing.
Comment 9 Mark Lam 2016-09-27 13:35:55 PDT 
Tests are happy.  Revised patch landed in r206459: <http://trac.webkit.org/r206459>.