Bug 191730 - RegExpObject's collectMatches should not be using JSArray::push to fill in its match results.
Summary: RegExpObject's collectMatches should not be using JSArray::push to fill in it...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-15 17:58 PST by Mark Lam
Modified: 2018-11-15 21:33 PST (History)
8 users (show)

See Also:

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 an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 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>
Comment 1 Mark Lam 2018-11-15 18:08:54 PST 
Created attachment 355006 [details]
proposed patch.
Comment 2 David Kilzer (:ddkilzer) 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
Comment 3 EWS Watchlist 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
Comment 4 Saam Barati 2018-11-15 20:03:29 PST 
Comment on attachment 355006 [details]
proposed patch.

r=me
Comment 5 Mark Lam 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.
Comment 6 Mark Lam 2018-11-15 20:27:11 PST 
Created attachment 355017 [details]
patch for landing.
Comment 7 Mark Lam 2018-11-15 21:33:56 PST 
Thanks for the review.  Landed in r238270: <http://trac.webkit.org/r238270>.