Bug 162584 - Add some needed CatchScopes in code that should not throw.
Summary: Add some needed CatchScopes in code that should not throw.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on: 162588
Blocks: 162351
  Show dependency treegraph
 
Reported: 2016-09-26 16:39 PDT by Mark Lam
Modified: 2016-09-27 13:35 PDT (History)
9 users (show)

See Also:


Attachments
proposed patch. (9.10 KB, patch)
2016-09-26 16:42 PDT, Mark Lam
keith_miller: review+
Details | Formatted Diff | Diff
Patch for re-landing. (6.81 KB, patch)
2016-09-27 11:53 PDT, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>.