Bug 164972

Summary: Fix exception scope verification failures in ArrayConstructor.cpp and ArrayPrototype.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, 165035    
Attachments:
Description Flags
proposed patch.
none
proposed patch.
none
proposed patch.
mark.lam: review-
proposed patch. ggaren: review+

Description Mark Lam 2016-11-18 17:16:50 PST 
Patch coming.
Comment 1 Mark Lam 2016-11-18 17:18:51 PST 
Created attachment 295228 [details]
proposed patch.
Comment 2 Mark Lam 2016-11-18 17:24:04 PST 
Created attachment 295230 [details]
proposed patch.
Comment 3 Mark Lam 2016-11-21 12:50:29 PST 
Comment on attachment 295230 [details]
proposed patch.

New patch with more fixes and returning { } coming soon.
Comment 4 Mark Lam 2016-11-21 12:56:23 PST 
Created attachment 295311 [details]
proposed patch.

Still running tests but preliminary results look good.  Let's get some EWS testing while we wait.
Comment 5 Mark Lam 2016-11-21 13:32:35 PST 
Comment on attachment 295311 [details]
proposed patch.

I think this is ready for a review.
Comment 6 Mark Lam 2016-11-24 13:45:24 PST 
Comment on attachment 295311 [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 7 Mark Lam 2016-11-24 14:48:36 PST 
Created attachment 295419 [details]
proposed patch.

Let's try this on the EWS first.
Comment 8 Mark Lam 2016-11-24 16:18:14 PST 
Comment on attachment 295419 [details]
proposed patch.

Tests paas.  Ready for a review.
Comment 9 Geoffrey Garen 2016-11-28 12:14:22 PST 
Comment on attachment 295419 [details]
proposed patch.

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

r=me

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1253
> +    ASSERT(!scope.exception() || !isValid);

I think you want to assert equality here. Otherwise, you don't check for scope.exception() && isValid.
Comment 10 Mark Lam 2016-11-28 13:12:18 PST 
(In reply to comment #9)
> Comment on attachment 295419 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295419&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1253
> > +    ASSERT(!scope.exception() || !isValid);
> 
> I think you want to assert equality here. Otherwise, you don't check for
> scope.exception() && isValid.

This assertion needs to be as is because it is possible for speciesWatchpointsValid(0 to return false (i.e. isValid is false) without throwing an exception.  The reverse is not true i.e. if an exception was thrown, then isValid must be false.

I confirmed this by running the test JSTests/stress/array-concat-on-frozen-object.js.
Comment 11 Mark Lam 2016-11-28 13:27:22 PST 
Thanks for the review.  Landed in r209011: <http://trac.webkit.org/r209011>.