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

src/js/regexp.js

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 (function(global, utils) {
 
 'use strict';
 
 %CheckIsBootstrapping();
 
@@ skipping 326 matching lines @@
   if (!IS_RECEIVER(this)) {
     throw %make_type_error(kIncompatibleMethodReceiver,
                         "RegExp.prototype.@@split", this);
   }
   string = TO_STRING(string);
   var constructor = SpeciesConstructor(this, GlobalRegExp);
   var flags = TO_STRING(this.flags);
 
   // TODO(adamk): this fast path is wrong as we doesn't ensure that 'exec'
   // is actually a data property on RegExp.prototype.
-  var exec;
   if (IS_REGEXP(this) && constructor === GlobalRegExp) {
-    exec = this.exec;
+    var exec = this.exec;

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

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]

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

     if (exec === RegExpExecJS) {
       return %_Call(RegExpSplit, this, string, limit);
     }
   }
 
   var unicode = %StringIndexOf(flags, 'u', 0) >= 0;
   var sticky = %StringIndexOf(flags, 'y', 0) >= 0;
   var newFlags = sticky ? flags : flags + "y";
   var splitter = new constructor(this, newFlags);
   var array = new GlobalArray();
   var arrayIndex = 0;
   var lim = (IS_UNDEFINED(limit)) ? kMaxUint32 : TO_UINT32(limit);
   var size = string.length;
   var prevStringIndex = 0;
   if (lim === 0) return array;
   var result;
   if (size === 0) {
     result = RegExpSubclassExec(splitter, string);
     if (IS_NULL(result)) %AddElement(array, 0, string);
     return array;
   }
   var stringIndex = prevStringIndex;
   while (stringIndex < size) {
     splitter.lastIndex = stringIndex;
-    result = RegExpSubclassExec(splitter, string, exec);
-    // Ensure exec will be read again on the next loop through.
-    exec = UNDEFINED;
+    result = RegExpSubclassExec(splitter, string);

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

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.

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

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.

     if (IS_NULL(result)) {
       stringIndex += AdvanceStringIndex(string, stringIndex, unicode);
     } else {
       var end = MinSimple(TO_LENGTH(splitter.lastIndex), size);
       if (end === prevStringIndex) {
         stringIndex += AdvanceStringIndex(string, stringIndex, unicode);
       } else {
         %AddElement(
             array, arrayIndex,
             %_SubString(string, prevStringIndex, stringIndex));
@@ skipping 741 matching lines @@
   to.GetSubstitution = GetSubstitution;
   to.InternalRegExpMatch = InternalRegExpMatch;
   to.InternalRegExpReplace = InternalRegExpReplace;
   to.IsRegExp = IsRegExp;
   to.RegExpExec = DoRegExpExec;
   to.RegExpInitialize = RegExpInitialize;
   to.RegExpLastMatchInfo = RegExpLastMatchInfo;
 });
 
 })