Skip to content

Commit 0615a51

Browse files
boingoingboingoing
committed
[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
2 parents 5644e6c + 17fcf4a commit 0615a51

6 files changed

Lines changed: 111 additions & 4 deletions

File tree

‎lib/Runtime/Library/JavascriptRegExpConstructor.cpp‎

Lines changed: 36 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ namespace Js
1717
JavascriptRegExpConstructor::JavascriptRegExpConstructor(DynamicType * type) :
1818
RuntimeFunction(type, &JavascriptRegExp::EntryInfo::NewInstance),
1919
reset(false),
20+
invalidatedLastMatch(false),
2021
lastPattern(nullptr),
2122
lastMatch() // undefined
2223
{
@@ -53,17 +54,44 @@ namespace Js
5354
this->lastInput = lastInput;
5455
this->lastMatch = lastMatch;
5556
this->reset = true;
57+
this->invalidatedLastMatch = false;
58+
}
59+
60+
void JavascriptRegExpConstructor::InvalidateLastMatch(UnifiedRegex::RegexPattern* lastPattern, JavascriptString* lastInput)
61+
{
62+
AssertMsg(lastPattern != nullptr, "lastPattern should not be null");
63+
AssertMsg(lastInput != nullptr, "lastInput should not be null");
64+
AssertMsg(JavascriptOperators::GetTypeId(lastInput) != TypeIds_Null, "lastInput should not be JavaScript null");
65+
66+
this->lastPattern = lastPattern;
67+
this->lastInput = lastInput;
68+
this->lastMatch.Reset();
69+
this->reset = true;
70+
this->invalidatedLastMatch = true;
5671
}
5772

5873
void JavascriptRegExpConstructor::EnsureValues()
5974
{
6075
if (reset)
6176
{
62-
Assert(!lastMatch.IsUndefined());
6377
ScriptContext* scriptContext = this->GetScriptContext();
78+
const CharCount lastInputLen = lastInput->GetLength();
79+
const char16* lastInputStr = lastInput->GetString();
6480
UnifiedRegex::RegexPattern* pattern = lastPattern;
81+
82+
// 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.
83+
// 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.
84+
// Since we only do this for the case of RegExp.prototype.test cache hit, we know several things:
85+
// - The regex is not global or sticky
86+
// - There was a match (test returned true)
87+
if (invalidatedLastMatch)
88+
{
89+
this->lastMatch = RegexHelper::SimpleMatch(scriptContext, pattern, lastInputStr, lastInputLen, 0);
90+
invalidatedLastMatch = false;
91+
}
92+
93+
Assert(!lastMatch.IsUndefined());
6594
JavascriptString* emptyString = scriptContext->GetLibrary()->GetEmptyString();
66-
const CharCount lastInputLen = lastInput->GetLength();
6795
// IE8 quirk: match of length 0 is regarded as length 1
6896
CharCount lastIndexVal = lastMatch.EndOffset();
6997
this->index = JavascriptNumber::ToVar(lastMatch.offset, scriptContext);
@@ -82,7 +110,7 @@ namespace Js
82110
// every match is prohibitively slow. Instead, run the match again using the known last input string.
83111
if (!pattern->WasLastMatchSuccessful())
84112
{
85-
RegexHelper::SimpleMatch(scriptContext, pattern, lastInput->GetString(), lastInputLen, lastMatch.offset);
113+
RegexHelper::SimpleMatch(scriptContext, pattern, lastInputStr, lastInputLen, lastMatch.offset);
86114
}
87115
Assert(pattern->WasLastMatchSuccessful());
88116
for (int groupId = 1; groupId < min(numGroups, NumCtorCaptures); groupId++)
@@ -100,6 +128,11 @@ namespace Js
100128
captures[groupId] = emptyString;
101129
reset = false;
102130
}
131+
else
132+
{
133+
// If we are not resetting the values, the last match cannot be invalidated.
134+
Assert(!invalidatedLastMatch);
135+
}
103136
}
104137

105138
/*static*/

‎lib/Runtime/Library/JavascriptRegExpConstructor.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,12 +53,14 @@ namespace Js
5353
bool GetPropertyBuiltIns(PropertyId propertyId, Var* value, BOOL* result);
5454
bool SetPropertyBuiltIns(PropertyId propertyId, Var value, BOOL* result);
5555
void SetLastMatch(UnifiedRegex::RegexPattern* lastPattern, JavascriptString* lastInput, UnifiedRegex::GroupInfo lastMatch);
56+
void InvalidateLastMatch(UnifiedRegex::RegexPattern* lastPattern, JavascriptString* lastInput);
5657

5758
void EnsureValues();
5859

5960
Field(UnifiedRegex::RegexPattern*) lastPattern;
6061
Field(JavascriptString*) lastInput;
6162
Field(UnifiedRegex::GroupInfo) lastMatch;
63+
Field(bool) invalidatedLastMatch; // true if last match must be recalculated before use
6264
Field(bool) reset; // true if following fields must be recalculated from above before first use
6365
Field(Var) lastParen;
6466
Field(Var) lastIndex;

‎lib/Runtime/Library/RegexHelper.cpp‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -691,13 +691,20 @@ namespace Js
691691
{
692692
Assert(useCache);
693693
cachedResult = (cache->resultBV.Test(cacheIndex) != 0);
694+
695+
// If our cache says this test should produce a match (which we aren't going to compute),
696+
// notify the Ctor to invalidate the last match so it must be recomputed before access.
697+
if (cachedResult)
698+
{
699+
InvalidateLastMatchOnCtor(scriptContext, regularExpression, input);
700+
}
701+
694702
// for debug builds, let's still do the real test so we can validate values in the cache
695703
#if !DBG
696704
return JavascriptBoolean::ToVar(cachedResult, scriptContext);
697705
#endif
698706
}
699707

700-
701708
CharCount offset;
702709
if (!GetInitialOffset(isGlobal, isSticky, regularExpression, inputLength, offset))
703710
{
@@ -2087,6 +2094,16 @@ namespace Js
20872094
}
20882095
}
20892096

2097+
void RegexHelper::InvalidateLastMatchOnCtor(ScriptContext* scriptContext, JavascriptRegExp* regularExpression, JavascriptString* lastInput, bool useSplitPattern)
2098+
{
2099+
Assert(lastInput);
2100+
2101+
UnifiedRegex::RegexPattern* pattern = useSplitPattern
2102+
? regularExpression->GetSplitPattern()
2103+
: regularExpression->GetPattern();
2104+
scriptContext->GetLibrary()->GetRegExpConstructor()->InvalidateLastMatch(pattern, lastInput);
2105+
}
2106+
20902107
bool RegexHelper::GetInitialOffset(bool isGlobal, bool isSticky, JavascriptRegExp* regularExpression, CharCount inputLength, CharCount& offset)
20912108
{
20922109
if (isGlobal || isSticky)

‎lib/Runtime/Library/RegexHelper.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,8 @@ namespace Js
5858
, UnifiedRegex::GroupInfo lastSuccessfulMatch
5959
, bool useSplitPattern );
6060

61+
static void InvalidateLastMatchOnCtor(ScriptContext* scriptContext, JavascriptRegExp* regularExpression, JavascriptString* lastInput, bool useSplitPattern = false);
62+
6163
static bool GetInitialOffset(bool isGlobal, bool isSticky, JavascriptRegExp* regularExpression, CharCount inputLength, CharCount& offset);
6264
static JavascriptArray* CreateMatchResult(void *const stackAllocationPointer, ScriptContext* scriptContext, bool isGlobal, int numGroups, JavascriptString* input);
6365
static void FinalizeMatchResult(ScriptContext* scriptContext, bool isGlobal, JavascriptArray* arr, UnifiedRegex::GroupInfo match);

‎test/Regex/bug_OS14763260.js‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
//-------------------------------------------------------------------------------------------------------
2+
// Copyright (C) Microsoft. All rights reserved.
3+
// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
4+
//-------------------------------------------------------------------------------------------------------
5+
6+
WScript.LoadScriptFile("..\\UnitTestFramework\\UnitTestFramework.js");
7+
8+
var tests = [
9+
{
10+
name: "Verify last match invalidated as expected",
11+
body: function () {
12+
const r1 = /(abc)/;
13+
const r2 = /(def)/;
14+
const s1 = "abc";
15+
const s2 = " def";
16+
17+
r1.test(s1);
18+
19+
assert.areEqual("abc", RegExp.input, "RegExp.input property calculated correctly");
20+
assert.areEqual("abc", RegExp['$_'], "RegExp.$_ property calculated correctly");
21+
assert.areEqual("abc", RegExp.lastMatch, "RegExp.lastMatch property calculated correctly");
22+
assert.areEqual("abc", RegExp['$&'], "RegExp.$& property calculated correctly");
23+
assert.areEqual("abc", RegExp.$1, "RegExp.$1 property calculated correctly");
24+
assert.areEqual(0, RegExp.index, "RegExp.index property calculated correctly");
25+
26+
r2.test(s2);
27+
28+
assert.areEqual(" def", RegExp.input, "RegExp.input property calculated correctly");
29+
assert.areEqual(" def", RegExp['$_'], "RegExp.$_ property calculated correctly");
30+
assert.areEqual("def", RegExp.lastMatch, "RegExp.lastMatch property calculated correctly");
31+
assert.areEqual("def", RegExp['$&'], "RegExp.$& property calculated correctly");
32+
assert.areEqual("def", RegExp.$1, "RegExp.$1 property calculated correctly");
33+
assert.areEqual(1, RegExp.index, "RegExp.index property calculated correctly");
34+
35+
r1.test(s1);
36+
37+
assert.areEqual("abc", RegExp.input, "Stale RegExp.input property should be invalidated by second r1.test(s1)");
38+
assert.areEqual("abc", RegExp['$_'], "Stale RegExp.$_ property should be invalidated by second r1.test(s1)");
39+
assert.areEqual("abc", RegExp.lastMatch, "Stale RegExp.lastMatch should be invalidated by second r1.test(s1)");
40+
assert.areEqual("abc", RegExp['$&'], "Stale RegExp.$& property should be invalidated by second r1.test(s1)");
41+
assert.areEqual("abc", RegExp.$1, "Stale RegExp.$1 should be invalidated by second r1.test(s1)");
42+
assert.areEqual(0, RegExp.index, "Stale RegExp.index property should be invalidated by second r1.test(s1)");
43+
}
44+
},
45+
];
46+
47+
testRunner.runTests(tests, { verbose: WScript.Arguments[0] != "summary" });

‎test/Regex/rlexe.xml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,4 +206,10 @@
206206
<compile-flags>-args summary -endargs</compile-flags>
207207
</default>
208208
</test>
209+
<test>
210+
<default>
211+
<files>bug_OS14763260.js</files>
212+
<compile-flags>-args summary -endargs</compile-flags>
213+
</default>
214+
</test>
209215
</regress-exe>

0 commit comments

Comments
 (0)