Bug 165053

Summary: Fix exception scope verification failures in ProxyConstructor.cpp and ProxyObject.cpp.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, fpizlo, ggaren, jfbastien, keith_miller, msaboff, saam, ysuzuki
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 162351    
Attachments:
Description Flags
proposed patch.
mark.lam: review-
proposed patch. saam: review+

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>.