[1.8>1.9] [MERGE #4607 @boingoing] OS#14763260: Correctly update RegExp.$1 after RegExp.prototype.test matches and used a cached value

Merge pull request #4607 from boingoing:FixRegExpTestCache

Regression from #3802 - if we hit the cache, we don't update the Regex constructor object with the last match and so retrieving properties about the match from the regex constructor object fails.

Fixed by adding an invalidation mechanism to the `JavascriptRegExpConstructor`. If we hit the cache, we'll mark the Regex constructor object last match properties as invalidated and we will then compute them on-demand the first time they're needed.

Fixes:
https://microsoft.visualstudio.com/web/wi.aspx?id=14763260

Author: boingoing

lib/Runtime/Library/JavascriptRegExpConstructor.cpp

@@ -17,6 +17,7 @@ namespace Js
     JavascriptRegExpConstructor::JavascriptRegExpConstructor(DynamicType * type) :
         RuntimeFunction(type, &JavascriptRegExp::EntryInfo::NewInstance),
         reset(false),
+        invalidatedLastMatch(false),
         lastPattern(nullptr),
         lastMatch() // undefined
     {
@@ -53,17 +54,44 @@ namespace Js
         this->lastInput = lastInput;
         this->lastMatch = lastMatch;
         this->reset = true;
+        this->invalidatedLastMatch = false;
+    }
+
+    void JavascriptRegExpConstructor::InvalidateLastMatch(UnifiedRegex::RegexPattern* lastPattern, JavascriptString* lastInput)
+    {
+        AssertMsg(lastPattern != nullptr, "lastPattern should not be null");
+        AssertMsg(lastInput != nullptr, "lastInput should not be null");
+        AssertMsg(JavascriptOperators::GetTypeId(lastInput) != TypeIds_Null, "lastInput should not be JavaScript null");
+
+        this->lastPattern = lastPattern;
+        this->lastInput = lastInput;
+        this->lastMatch.Reset();
+        this->reset = true;
+        this->invalidatedLastMatch = true;
     }
 
     void JavascriptRegExpConstructor::EnsureValues()
     {
         if (reset)
         {
-            Assert(!lastMatch.IsUndefined());
             ScriptContext* scriptContext = this->GetScriptContext();
+            const CharCount lastInputLen = lastInput->GetLength();
+            const char16* lastInputStr = lastInput->GetString();
             UnifiedRegex::RegexPattern* pattern = lastPattern;
+
+            // When we perform a regex test operation it's possible the result of the operation will be loaded from a cache and the match will not be computed and updated in the ctor.
+            // In that case we invalidate the last match stored in the ctor and will need to compute it before it will be accessible via $1 etc.
+            // Since we only do this for the case of RegExp.prototype.test cache hit, we know several things:
+            //  - The regex is not global or sticky
+            //  - There was a match (test returned true)
+            if (invalidatedLastMatch)
+            {
+                this->lastMatch = RegexHelper::SimpleMatch(scriptContext, pattern, lastInputStr, lastInputLen, 0);
+                invalidatedLastMatch = false;
+            }
+
+            Assert(!lastMatch.IsUndefined());
             JavascriptString* emptyString = scriptContext->GetLibrary()->GetEmptyString();
-            const CharCount lastInputLen = lastInput->GetLength();
             // IE8 quirk: match of length 0 is regarded as length 1
             CharCount lastIndexVal = lastMatch.EndOffset();
             this->index = JavascriptNumber::ToVar(lastMatch.offset, scriptContext);
@@ -82,7 +110,7 @@ namespace Js
                 // every match is prohibitively slow. Instead, run the match again using the known last input string.
                 if (!pattern->WasLastMatchSuccessful())
                 {
-                    RegexHelper::SimpleMatch(scriptContext, pattern, lastInput->GetString(), lastInputLen, lastMatch.offset);
+                    RegexHelper::SimpleMatch(scriptContext, pattern, lastInputStr, lastInputLen, lastMatch.offset);
                 }
                 Assert(pattern->WasLastMatchSuccessful());
                 for (int groupId = 1; groupId < min(numGroups, NumCtorCaptures); groupId++)
@@ -100,6 +128,11 @@ namespace Js
                 captures[groupId] = emptyString;
             reset = false;
         }
+        else
+        {
+            // If we are not resetting the values, the last match cannot be invalidated.
+            Assert(!invalidatedLastMatch);
+        }
     }
 
     /*static*/

lib/Runtime/Library/JavascriptRegExpConstructor.h

@@ -53,12 +53,14 @@ namespace Js
         bool GetPropertyBuiltIns(PropertyId propertyId, Var* value, BOOL* result);
         bool SetPropertyBuiltIns(PropertyId propertyId, Var value, BOOL* result);
         void SetLastMatch(UnifiedRegex::RegexPattern* lastPattern, JavascriptString* lastInput, UnifiedRegex::GroupInfo lastMatch);
+        void InvalidateLastMatch(UnifiedRegex::RegexPattern* lastPattern, JavascriptString* lastInput);
 
         void EnsureValues();
 
         Field(UnifiedRegex::RegexPattern*) lastPattern;
         Field(JavascriptString*) lastInput;
         Field(UnifiedRegex::GroupInfo) lastMatch;
+        Field(bool) invalidatedLastMatch; // true if last match must be recalculated before use
         Field(bool) reset; // true if following fields must be recalculated from above before first use
         Field(Var) lastParen;
         Field(Var) lastIndex;

lib/Runtime/Library/RegexHelper.cpp

@@ -691,13 +691,20 @@ namespace Js
         {
             Assert(useCache);
             cachedResult = (cache->resultBV.Test(cacheIndex) != 0);
+
+            // If our cache says this test should produce a match (which we aren't going to compute),
+            // notify the Ctor to invalidate the last match so it must be recomputed before access.
+            if (cachedResult)
+            {
+                InvalidateLastMatchOnCtor(scriptContext, regularExpression, input);
+            }
+
             // for debug builds, let's still do the real test so we can validate values in the cache
 #if !DBG
             return JavascriptBoolean::ToVar(cachedResult, scriptContext);
 #endif
         }
 
-
         CharCount offset;
         if (!GetInitialOffset(isGlobal, isSticky, regularExpression, inputLength, offset))
         {
@@ -2087,6 +2094,16 @@ namespace Js
         }
     }
 
+    void RegexHelper::InvalidateLastMatchOnCtor(ScriptContext* scriptContext, JavascriptRegExp* regularExpression, JavascriptString* lastInput, bool useSplitPattern)
+    {
+        Assert(lastInput);
+
+        UnifiedRegex::RegexPattern* pattern = useSplitPattern
+            ? regularExpression->GetSplitPattern()
+            : regularExpression->GetPattern();
+        scriptContext->GetLibrary()->GetRegExpConstructor()->InvalidateLastMatch(pattern, lastInput);
+    }
+
     bool RegexHelper::GetInitialOffset(bool isGlobal, bool isSticky, JavascriptRegExp* regularExpression, CharCount inputLength, CharCount& offset)
     {
         if (isGlobal || isSticky)

lib/Runtime/Library/RegexHelper.h

@@ -58,6 +58,8 @@ namespace Js
             , UnifiedRegex::GroupInfo lastSuccessfulMatch
             , bool useSplitPattern );
 
+        static void InvalidateLastMatchOnCtor(ScriptContext* scriptContext, JavascriptRegExp* regularExpression, JavascriptString* lastInput, bool useSplitPattern = false);
+
         static bool GetInitialOffset(bool isGlobal, bool isSticky, JavascriptRegExp* regularExpression, CharCount inputLength, CharCount& offset);
         static JavascriptArray* CreateMatchResult(void *const stackAllocationPointer, ScriptContext* scriptContext, bool isGlobal, int numGroups, JavascriptString* input);
         static void FinalizeMatchResult(ScriptContext* scriptContext, bool isGlobal, JavascriptArray* arr, UnifiedRegex::GroupInfo match);

test/Regex/bug_OS14763260.js

@@ -0,0 +1,47 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+WScript.LoadScriptFile("..\\UnitTestFramework\\UnitTestFramework.js");
+
+var tests = [
+  {
+    name: "Verify last match invalidated as expected",
+    body: function () {
+        const r1 = /(abc)/;
+        const r2 = /(def)/;
+        const s1 = "abc";
+        const s2 = " def";
+         
+        r1.test(s1);
+        
+        assert.areEqual("abc", RegExp.input, "RegExp.input property calculated correctly");
+        assert.areEqual("abc", RegExp['$_'], "RegExp.$_ property calculated correctly");
+        assert.areEqual("abc", RegExp.lastMatch, "RegExp.lastMatch property calculated correctly");
+        assert.areEqual("abc", RegExp['$&'], "RegExp.$& property calculated correctly");
+        assert.areEqual("abc", RegExp.$1, "RegExp.$1 property calculated correctly");
+        assert.areEqual(0, RegExp.index, "RegExp.index property calculated correctly");
+        
+        r2.test(s2);
+        
+        assert.areEqual(" def", RegExp.input, "RegExp.input property calculated correctly");
+        assert.areEqual(" def", RegExp['$_'], "RegExp.$_ property calculated correctly");
+        assert.areEqual("def", RegExp.lastMatch, "RegExp.lastMatch property calculated correctly");
+        assert.areEqual("def", RegExp['$&'], "RegExp.$& property calculated correctly");
+        assert.areEqual("def", RegExp.$1, "RegExp.$1 property calculated correctly");
+        assert.areEqual(1, RegExp.index, "RegExp.index property calculated correctly");
+        
+        r1.test(s1);
+
+        assert.areEqual("abc", RegExp.input, "Stale RegExp.input property should be invalidated by second r1.test(s1)");
+        assert.areEqual("abc", RegExp['$_'], "Stale RegExp.$_ property should be invalidated by second r1.test(s1)");
+        assert.areEqual("abc", RegExp.lastMatch, "Stale RegExp.lastMatch should be invalidated by second r1.test(s1)");
+        assert.areEqual("abc", RegExp['$&'], "Stale RegExp.$& property should be invalidated by second r1.test(s1)");
+        assert.areEqual("abc", RegExp.$1, "Stale RegExp.$1 should be invalidated by second r1.test(s1)");
+        assert.areEqual(0, RegExp.index, "Stale RegExp.index property should be invalidated by second r1.test(s1)");
+    }
+  },
+];
+
+testRunner.runTests(tests, { verbose: WScript.Arguments[0] != "summary" });

test/Regex/rlexe.xml

@@ -206,4 +206,10 @@
       <compile-flags>-args summary -endargs</compile-flags>
     </default>
   </test>
+  <test>
+    <default>
+      <files>bug_OS14763260.js</files>
+      <compile-flags>-args summary -endargs</compile-flags>
+    </default>
+  </test>
 </regress-exe>