[regexp] Don't cache exec method in Regexp.proto[@@split]

[regexp] Don't cache exec method in Regexp.proto[@@split]

The call to RegExpSubclassExec may refer to a different exec method
since splitter is newly constructed previously to the call.

BUG=v8:5351
Committed: https://crrev.com/515994b8ca5710409bd0a8f21ac64844fea839f0
Cr-Commit-Position: refs/heads/master@{#39774}

[jgruber, 2016-09-26 14:10:28]

The CQ bit was checked by jgruber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 14:10:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jgruber, 2016-09-26 14:12:38]

jgruber@chromium.org changed reviewers:
+ littledan@chromium.org

[Dan Ehrenberg, 2016-09-26 14:16:27]

https://codereview.chromium.org/2370733003/diff/1/src/js/regexp.js
File src/js/regexp.js (right):

https://codereview.chromium.org/2370733003/diff/1/src/js/regexp.js#newcode348
src/js/regexp.js:348: var exec = this.exec;
Doesn't this initial, additional read of exec violate the spec in a new way
(e.g. if exec is a getter)?

[jgruber, 2016-09-26 14:19:37]

https://codereview.chromium.org/2370733003/diff/1/src/js/regexp.js
File src/js/regexp.js (right):

https://codereview.chromium.org/2370733003/diff/1/src/js/regexp.js#newcode348
src/js/regexp.js:348: var exec = this.exec;
On 2016/09/26 14:16:27, Dan Ehrenberg wrote:
> Doesn't this initial, additional read of exec violate the spec in a new way
> (e.g. if exec is a getter)?

I'm not sure which additional read you mean? This is identical to OLD except
that 'var exec' has been removed from line 347.

[Dan Ehrenberg, 2016-09-26 14:30:33]

https://codereview.chromium.org/2370733003/diff/1/src/js/regexp.js
File src/js/regexp.js (right):

https://codereview.chromium.org/2370733003/diff/1/src/js/regexp.js#newcode373
src/js/regexp.js:373: result = RegExpSubclassExec(splitter, string);
By not passing exec here, an additional read of exec is caused on the first
iteration. The initial read into the exec variable is synthetic, to be able to
do the fast path. It is observable, but we make up for it by using that value.

A path to recovering correctness would be to call a function to check that exec
is a data property (and the receiver is not a Proxy) and falling back to the
slow path otherwise, before reading the exec property.

[commit-bot: I haz the power, 2016-09-26 14:42:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-26 14:42:57]

Dry run: This issue passed the CQ dry run.

[jgruber, 2016-09-26 14:57:23]

https://codereview.chromium.org/2370733003/diff/1/src/js/regexp.js
File src/js/regexp.js (right):

https://codereview.chromium.org/2370733003/diff/1/src/js/regexp.js#newcode373
src/js/regexp.js:373: result = RegExpSubclassExec(splitter, string);
On 2016/09/26 14:30:33, Dan Ehrenberg wrote:
> By not passing exec here, an additional read of exec is caused on the first
> iteration. The initial read into the exec variable is synthetic, to be able to
> do the fast path. It is observable, but we make up for it by using that value.
> 
> A path to recovering correctness would be to call a function to check that
exec
> is a data property (and the receiver is not a Proxy) and falling back to the
> slow path otherwise, before reading the exec property.

I see.

But do we care about the additional exec read? It already violated the spec in
that it was read at the beginning of split instead of during the first loop
iteration.

I'm also fine with adding data property checks, but that complicates the
implementation even further.

[Dan Ehrenberg, 2016-09-26 16:33:34]

lgtm

OTOH there are already other similar spec violations, and exec was not being
read in exactly the right way. Since this fixes a real bug of greater magnitude
(totally incorrect answer vs extra read of what's probably a data property), I
think it's fine to go in as long as you file another bug to fix it up and
eliminate the extra exec read in a follow-on patch.

[jgruber, 2016-09-27 13:29:22]

The CQ bit was checked by jgruber@chromium.org

[jgruber, 2016-09-27 13:29:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
littledan@chromium.org
Link to the patchset: https://codereview.chromium.org/2370733003/#ps20001
(title: "Rebase")

[commit-bot: I haz the power, 2016-09-27 13:29:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 14:02:19]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-27 14:02:41]

Description was changed from

==========
[regexp] Don't cache exec method in Regexp.proto[@@split]

The call to RegExpSubclassExec may refer to a different exec method
since splitter is newly constructed previously to the call.

BUG=v8:5351
==========

to

==========
[regexp] Don't cache exec method in Regexp.proto[@@split]

The call to RegExpSubclassExec may refer to a different exec method
since splitter is newly constructed previously to the call.

BUG=v8:5351

Committed: https://crrev.com/515994b8ca5710409bd0a8f21ac64844fea839f0
Cr-Commit-Position: refs/heads/master@{#39774}
==========

[commit-bot: I haz the power, 2016-09-27 14:02:42]

Patchset 2 (id:??) landed as
https://crrev.com/515994b8ca5710409bd0a8f21ac64844fea839f0
Cr-Commit-Position: refs/heads/master@{#39774}