Bug 165053 - Fix exception scope verification failures in ProxyConstructor.cpp and ProxyObject.cpp.
Summary: Fix exception scope verification failures in ProxyConstructor.cpp and ProxyOb...
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:
Blocks: 162351
  Show dependency treegraph
 
Reported: 2016-11-23 16:32 PST by Mark Lam
Modified: 2016-11-29 11:10 PST (History)
8 users (show)

See Also:


Attachments
proposed patch. (19.76 KB, patch)
2016-11-23 16:34 PST, Mark Lam
mark.lam: review-
Details | Formatted Diff | Diff
proposed patch. (17.29 KB, patch)
2016-11-25 14:00 PST, Mark Lam
saam: review+
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-11-23 16:32:12 PST
Patch coming.
Comment 1 Mark Lam 2016-11-23 16:34:24 PST
Created attachment 295384 [details]
proposed patch.
Comment 2 Mark Lam 2016-11-24 13:38:32 PST
Comment on attachment 295384 [details]
proposed patch.

It is invalid to replace returning encodedJSValue() with returning { }.  On 32-bit builds, the former is non-zero, while the latter is 0.  Will fix this patch.
Comment 3 Mark Lam 2016-11-25 14:00:06 PST
Created attachment 295427 [details]
proposed patch.
Comment 4 Saam Barati 2016-11-28 14:01:48 PST
Comment on attachment 295427 [details]
proposed patch.

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

> Source/JavaScriptCore/runtime/ProxyObject.cpp:944
>      bool targetIsExensible = target->isExtensible(exec);
> +    RETURN_IF_EXCEPTION(scope, void());

This seems like it could be easily testable through JS code by ensuring that getOwnPropertyNames below is not called if this throws an exception.
Comment 5 Mark Lam 2016-11-29 11:06:28 PST
(In reply to comment #4)
> Comment on attachment 295427 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295427&action=review
> 
> > Source/JavaScriptCore/runtime/ProxyObject.cpp:944
> >      bool targetIsExensible = target->isExtensible(exec);
> > +    RETURN_IF_EXCEPTION(scope, void());
> 
> This seems like it could be easily testable through JS code by ensuring that
> getOwnPropertyNames below is not called if this throws an exception.

I thought this was easy too, but upon trying, I found that it is easy to replicate the condition of the unchecked exception, however, it is not easy to create a condition where the unchecked exception is detectable.  This is because the exception will eventually get checked at many places before the VM gets back to any JS code that can observe the issue.  Hence, I won't be able to write this test.

I'll land this patch as is.
Comment 6 Mark Lam 2016-11-29 11:10:06 PST
Thanks for the review.  Landed in r209080: <http://trac.webkit.org/r209080>.