OS#14763260: Correctly update RegExp.$1 after RegExp.prototype.test matches and used a cached value #4607

boingoing · 2018-01-27T01:57:11Z

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

…atches and used a cached value Regression from chakra-core#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

obastemur · 2018-01-27T02:08:18Z

lib/Runtime/Library/JavascriptRegExpConstructor.cpp

-            Assert(!lastMatch.IsUndefined());
            ScriptContext* scriptContext = this->GetScriptContext();
+            const CharCount lastInputLen = lastInput->GetLength();
+            const wchar_t* lastInputStr = lastInput->GetString();


use WCHAR instead. It refers to char16 on xplat. (why? wchar_t is 4 bytes on some platforms while 2 bytes on others)

I replaced it with char16 since all the APIs here are using char16. Seems this must be fine for xplat.

That is also fine! Thanks

dilijev

Generally LGTM but @akroshg should have a look too.

dilijev · 2018-01-27T03:22:36Z

lib/Runtime/Library/JavascriptRegExpConstructor.cpp

+    {
+        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");


Why these particular checks?

We check similar things in JavascriptRegExpConstructor::SetLastMatch which is close to what we're doing here. Just kept it consistent.

dilijev · 2018-01-27T03:22:55Z

lib/Runtime/Library/JavascriptRegExpConstructor.cpp

+        this->lastPattern = lastPattern;
+        this->lastInput = lastInput;
+        this->lastMatch.Reset();
+        this->reset = true;


Does "reset" in the two lines above have the same meaning?

No. We want to reset the last match to undefined and let the regex constructor know it has to reset it's cache via JavascriptRegExpConstructor::EnsureValues.

What is the difference between reset and invalidateLastMatch?

The existing flag reset is used to indicate we should reset the cache backed-by JavascriptRegExpConstructor::EnsureValues. I added invalidateLastMatch which is used to indicate that there is no value stored in the last match field. This means we need to first calculate the last match before we use it. Ordinarily we assert that the last match field in JavascriptRegExpConstructor::EnsureValues is not undefined.

The reset flag is used in the logic for materializing all of the RegExp['$blah'] properties as actual JavascriptStrings (when needed) rather than input + offset which is faster to compute, and the logic ensures this only happens once. I'm not sure how the term "reset" applies to this logic but this code is apparently very old.

digitalinfinity · 2018-01-29T16:39:54Z

test/Regex/bug_OS14763260.js

+        r2.test(s2);
+        r1.test(s1);
+
+        assert.areEqual("abc", RegExp.$1, "Stale last match should be invalidated by second r1.test(s1)");


Are there other side-effects that regex matching can have? It might be good to add a few more tests.

Sure, I'll add a few more tests.

akroshg · 2018-01-29T18:53:40Z

lib/Runtime/Library/JavascriptRegExpConstructor.h


        Field(UnifiedRegex::RegexPattern*) lastPattern;
        Field(JavascriptString*) lastInput;
+        Field(bool) invalidatedLastMatch; // true if last match must be recalculated before use


nit: move it down with other bool.

akroshg · 2018-01-29T18:56:40Z

lib/Runtime/Library/JavascriptRegExpConstructor.cpp

+            //  - There was a match (test returned true)
+            if (invalidatedLastMatch)
+            {
+                this->lastMatch = RegexHelper::SimpleMatch(scriptContext, pattern, lastInputStr, lastInputLen, 0);


SimpleMatch [](start = 47, length = 11)

could this function be re-entrant and change lastInput by any chance?

I hope not. We don't pass any Var here, just the const char16* so I don't think user can apply getter/setters or use Symbol overrides. Seems as safe as the other operations here.

digitalinfinity · 2018-01-30T00:46:26Z

test/Regex/bug_OS14763260.js


        r1.test(s1);
+
+        assert.areEqual("abc", RegExp.input, "RegExp.input property caclculated correctly");


Nit: spelling of calculated in this file

Thanks, good catch

…r 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

boingoing · 2018-01-30T01:38:51Z

Thanks, everyone

…xp.$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

…y 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

boingoing requested review from MikeHolman, digitalinfinity and dilijev January 27, 2018 02:03

obastemur reviewed Jan 27, 2018

View reviewed changes

Fix MacOS build

7f9822a

dilijev reviewed Jan 27, 2018

View reviewed changes

dilijev requested a review from akroshg January 27, 2018 03:23

digitalinfinity reviewed Jan 29, 2018

View reviewed changes

akroshg reviewed Jan 29, 2018

View reviewed changes

Add test cases

680f4c9

digitalinfinity reviewed Jan 30, 2018

View reviewed changes

kfarnung approved these changes Jan 30, 2018

View reviewed changes

Fix a spelling mistake and add an assert

b98be0e

chakrabot merged commit b98be0e into chakra-core:release/1.8 Jan 30, 2018


		r1.test(s1);

		assert.areEqual("abc", RegExp.input, "RegExp.input property caclculated correctly");

OS#14763260: Correctly update RegExp.$1 after RegExp.prototype.test matches and used a cached value #4607

OS#14763260: Correctly update RegExp.$1 after RegExp.prototype.test matches and used a cached value #4607

Uh oh!

Conversation

boingoing commented Jan 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dilijev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dilijev Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boingoing commented Jan 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dilijev Jan 30, 2018 •

edited

Loading