Bug 191730

Summary:

RegExpObject's collectMatches should not be using JSArray::push to fill in its match results.

Product:

WebKit

Reporter:

Mark Lam <mark.lam>

Component:

JavaScriptCore

Assignee:

Mark Lam <mark.lam>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ddkilzer, ews-watchlist, guijemont, guijemont+jsc-armv7-ews, keith_miller, msaboff, saam, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
proposed patch.	ews-watchlist: commit-queue-
patch for landing.	none

Mark Lam

Reported 2018-11-15 17:58:03 PST

According to the spec https://www.ecma-international.org/ecma-262/9.0/index.html#sec-regexp.prototype-@@match, the RegExp match results are filling in using the spec's CreateDataProperty() which does not consult the prototype for setters. JSArray:push() consults the prototype for setters. We should be using putDirectIndex() instead. <rdar://problem/46048517>

Attachments
proposed patch. (3.36 KB, patch) 2018-11-15 18:08 PST, Mark Lam	ews-watchlist: commit-queue-	Details Formatted Diff Diff
patch for landing. (4.48 KB, patch) 2018-11-15 20:27 PST, Mark Lam	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Mark Lam

Comment 1 2018-11-15 18:08:54 PST

Created attachment 355006 [details] proposed patch.

David Kilzer (:ddkilzer)

Comment 2 2018-11-15 18:49:20 PST

Comment on attachment 355006 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=355006&action=review > JSTests/ChangeLog:10 > 2018-11-13 Saam Barati <sbarati@apple.com> Nit: Blank line before the new entry

EWS Watchlist

Comment 3 2018-11-15 19:41:27 PST

Comment on attachment 355006 [details] proposed patch. Attachment 355006 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/10011421 New failing tests: stress/regress-187006.js.ftl-eager-no-cjit-b3o1 stress/regress-187006.js.ftl-no-cjit-small-pool stress/regress-187006.js.ftl-no-cjit-validate-sampling-profiler stress/regress-187006.js.default stress/regress-187006.js.dfg-eager-no-cjit-validate stress/regress-187006.js.ftl-eager stress/regress-187006.js.dfg-eager stress/regress-187006.js.dfg-maximal-flush-validate-no-cjit stress/regress-187006.js.ftl-no-cjit-no-put-stack-validate stress/regress-187006.js.no-ftl stress/regress-187006.js.ftl-no-cjit-b3o1 stress/regress-187006.js.no-llint stress/regress-187006.js.ftl-eager-no-cjit stress/regress-187006.js.ftl-no-cjit-no-inline-validate stress/regress-187006.js.no-cjit-validate-phases stress/regress-187006.js.no-cjit-collect-continuously apiTests

Saam Barati

Comment 4 2018-11-15 20:03:29 PST

Comment on attachment 355006 [details] proposed patch. r=me

Mark Lam

Comment 5 2018-11-15 20:22:57 PST

(In reply to Build Bot from comment #3) > New failing tests: > stress/regress-187006.js.ftl-eager-no-cjit-b3o1 ... > apiTests Turns out, the regress-187006.js test is invalid. It expects a setter in Array.prototype to be called when RegExp.match creates its match array, which violates the spec.

Mark Lam

Comment 6 2018-11-15 20:27:11 PST

Created attachment 355017 [details] patch for landing.

Mark Lam

Comment 7 2018-11-15 21:33:56 PST

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

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