Bug 164972

Summary:

Fix exception scope verification failures in ArrayConstructor.cpp and ArrayPrototype.cpp

Product:

WebKit

Reporter:

Mark Lam <mark.lam>

Component:

JavaScriptCore

Assignee:

Mark Lam <mark.lam>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, fpizlo, ggaren, jfbastien, keith_miller, msaboff, saam, ysuzuki

Priority:

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+

Mark Lam

Reported 2016-11-18 17:16:50 PST

Patch coming.

Attachments
proposed patch. (27.22 KB, patch) 2016-11-18 17:18 PST, Mark Lam	no flags	Details Formatted Diff Diff
proposed patch. (27.92 KB, patch) 2016-11-18 17:24 PST, Mark Lam	no flags	Details Formatted Diff Diff
proposed patch. (41.96 KB, patch) 2016-11-21 12:56 PST, Mark Lam	mark.lam: review-	Details Formatted Diff Diff
proposed patch. (35.44 KB, patch) 2016-11-24 14:48 PST, Mark Lam	ggaren: review+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Mark Lam

Comment 1 2016-11-18 17:18:51 PST

Created attachment 295228 [details] proposed patch.

Mark Lam

Comment 2 2016-11-18 17:24:04 PST

Created attachment 295230 [details] proposed patch.

Mark Lam

Comment 3 2016-11-21 12:50:29 PST

Comment on attachment 295230 [details] proposed patch. New patch with more fixes and returning { } coming soon.

Mark Lam

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

Mark Lam

Comment 5 2016-11-21 13:32:35 PST

Comment on attachment 295311 [details] proposed patch. I think this is ready for a review.

Mark Lam

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

Mark Lam

Comment 7 2016-11-24 14:48:36 PST

Created attachment 295419 [details] proposed patch. Let's try this on the EWS first.

Mark Lam

Comment 8 2016-11-24 16:18:14 PST

Comment on attachment 295419 [details] proposed patch. Tests paas. Ready for a review.

Geoffrey Garen

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

Mark Lam

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

Mark Lam

Comment 11 2016-11-28 13:27:22 PST

Thanks for the review. Landed in r209011: <http://trac.webkit.org/r209011>.

Note You need to log in before you can comment on or make changes to this bug.