Last Comment Bug 1304737 - RegExp.test with global or sticky flags does not correctly set RegExp.lastIndex
: RegExp.test with global or sticky flags does not correctly set RegExp.lastIndex
Status: VERIFIED FIXED
: regression
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 46 Branch
: Unspecified Unspecified
: -- normal (vote)
: mozilla52
Assigned To: Tooru Fujisawa [:arai]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 1207922
  Show dependency treegraph
 
Reported: 2016-09-22 07:37 PDT by Patrick Dähne
Modified: 2016-11-28 04:55 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard: [good first verify]
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
-
verified
+
verified
+
verified


Attachments
Do not ignore trailing .* on matchOnly RegExp execution. (6.43 KB, patch)
2016-09-22 12:37 PDT, Tooru Fujisawa [:arai]
hv1989: review+
rkothari: approval‑mozilla‑aurora+
rkothari: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description User image Patrick Dähne 2016-09-22 07:37:31 PDT
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160916101415

Steps to reproduce:

Open the Web Console and enter the following line:

var r = /f.*/g; r.test('foo'); r.lastIndex;


Actual results:

I get '1' as result.


Expected results:

I expect to get '3' as result.
Comment 1 User image Loic 2016-09-22 10:13:17 PDT
Reg range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4f4ef7073bb53185edc3a208446c687b291abe61&tochange=a0c3bdd0559b936e5b2865783411231181c84d3c

Tooru Fujisawa — Bug 1207922 - Part 1: Self-host RegExp.prototype.{exec,test}. r=till,h4writer
Comment 2 User image Tooru Fujisawa [:arai] 2016-09-22 10:21:03 PDT
thank you for pointing it out!
optimization for the trailing ".*" goes wrong.
Comment 3 User image Tooru Fujisawa [:arai] 2016-09-22 10:46:43 PDT
https://dxr.mozilla.org/mozilla-central/rev/560b2c805bf7bebeb3ceebc495a81b2aa4c0c755/js/src/irregexp/RegExpParser.cpp#1851
>         // Try to strip a trailing '.*' from the RegExp, which as above will
>         // affect the captures but not whether there is a match. Only do this
>         // when there are no other meta characters in the RegExp, so that we
>         // are sure this will not affect how the RegExp is parsed.
>         if (length >= 3 && !HasRegExpMetaChars(chars, length - 2) &&
>             chars[length - 2] == '.' && chars[length - 1] == '*')
>         {
>             length -= 2;
>         }

This affects lastIndex.
also, we cannot use input string's length for lastIndex, as the pattern doesn't match whole string if the string contains newline.
so, this optimization should need to be removed.
Comment 4 User image Tooru Fujisawa [:arai] 2016-09-22 12:04:06 PDT
I was wrong.
We have been used non-matchOnly execution for global/sticky test.
Will try adding back equivalent.
Comment 5 User image Tooru Fujisawa [:arai] 2016-09-22 12:37:14 PDT
Created attachment 8793914 [details] [diff] [review]
Do not ignore trailing .* on matchOnly RegExp execution.

Previously, we were using non-matchOnly execution for test with global/sticky,
to update lastIndex correctly.

https://dxr.mozilla.org/mozilla-esr45/rev/7dedca747e536c65ddbd2f487d0112c88159a6a3/js/src/builtin/RegExp.cpp#808
>     Maybe<ScopedMatchPairs> alternateMatches;
>     if (!reobj->needUpdateLastIndex()) {
>         searchIndex = 0;
>     } else if (!matches) {
>         alternateMatches.emplace(&cx->tempLifoAlloc());
>         matches = &alternateMatches.ref();
>     }
> ...
>     } else if (reobj->needUpdateLastIndex()) {
>         /* Steps 18.a-b. */
>         MOZ_ASSERT(matches && !matches->empty());
>         if (!SetLastIndex(cx, reobj, (*matches)[0].limit))
>             return RegExpRunStatus_Error;
>     }

now, we're returning lastIndex value also on matchOnly execution.

also, bug 1263340 patches changed RegExpBuiltinExec to use internal global and sticky flags, so global and sticky values are known at compile time.

we just need to avoid stripping trailing ".*" for global/sticky case, to make lastIndex value correct.

this change won't affect most case that tests with non-global and non-sticky RegExp.
Comment 6 User image Hannes Verschore [:h4writer] 2016-09-23 03:49:25 PDT
Comment on attachment 8793914 [details] [diff] [review]
Do not ignore trailing .* on matchOnly RegExp execution.

Review of attachment 8793914 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.
Comment 7 User image Marcia Knous [:marcia - use ni] 2016-09-23 13:05:48 PDT
Tracking 52+ for this JS regression.
Comment 8 User image Tooru Fujisawa [:arai] 2016-09-23 13:11:35 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3800637757590adbf18a09c18b9351a2d04fa76
Bug 1304737 - Do not ignore trailing .* on matchOnly RegExp execution. r=h4writer
Comment 9 User image Gerry Chang [:gchang] 2016-09-26 01:34:58 PDT
Track for 51+ as this is JS regression.
Comment 10 User image Tooru Fujisawa [:arai] 2016-09-26 01:58:29 PDT
Comment on attachment 8793914 [details] [diff] [review]
Do not ignore trailing .* on matchOnly RegExp execution.

looks like m-c merge is taking time.
requesting approval in advance.

same patch is applicable to mozilla-aurora.

Approval Request Comment
> [Feature/regressing bug #]
bug 1207922

> [User impact if declined]
Wrong JavaScript behavior.

> [Describe test coverage new/current, TreeHerder]
tested in m-i

> [Risks and why]
low.  this patch disables unexpected optimization for specific case.
it does fallback to normal path.

> [String/UUID change made/needed]
None
Comment 11 User image Tooru Fujisawa [:arai] 2016-09-26 02:12:22 PDT
Comment on attachment 8793914 [details] [diff] [review]
Do not ignore trailing .* on matchOnly RegExp execution.

same patch is applicable to mozilla-beta.

Approval Request Comment
> [Feature/regressing bug #]
bug 1207922

> [User impact if declined]
Wrong JavaScript behavior.

> [Describe test coverage new/current, TreeHerder]
tested in m-i

> [Risks and why]
low.  this patch disables unexpected optimization for specific case.
it does fallback to normal path.

> [String/UUID change made/needed]
None
Comment 12 User image Iris Hsiao [:ihsiao] 2016-09-26 04:05:29 PDT
https://hg.mozilla.org/mozilla-central/rev/f38006377575
Comment 13 User image Ritu Kothari (:ritu) 2016-09-26 12:32:11 PDT
Hi Patrick, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Comment 14 User image Ritu Kothari (:ritu) 2016-09-26 15:40:52 PDT
Comment on attachment 8793914 [details] [diff] [review]
Do not ignore trailing .* on matchOnly RegExp execution.

Fixes a JS regression, Aurora51+, Beta50+
Comment 15 User image Ritu Kothari (:ritu) 2016-09-26 15:41:47 PDT
Don't feel the need to track it now that it's fixed.
Comment 17 User image Carsten Book [:Tomcat] 2016-09-27 00:58:28 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/2704e3050659
Comment 18 User image Patrick Dähne 2016-09-28 02:33:05 PDT
It's working now as expected in the Nightly build. Thanks for fixing!
Comment 19 User image Saddam Hossain 2016-09-30 02:43:33 PDT
I have reproduced this Bug on Nightly 52.0a1 (2016-09-22)  on Windows 10, 64 Bit!

The bug's fix is now verified on latest  Nightly 52.0a1

Nightly  52.0a1:
Build ID	   : 20160929030426
User Agent 	   : Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:52.0) Gecko/20100101 Firefox/52.0

[testday-20160930]
Comment 20 User image Subhrajyoti Sen 2016-09-30 02:48:23 PDT
Fixed on Firefox 50.0b3 using Linux Mint 18
Comment 21 User image Vibhanshu Chaudhary 2016-09-30 03:36:05 PDT
It's working as expected on Firefox 50.0b3 on Windows 10 Pro
Comment 22 User image Liz Henry (:lizzard) (needinfo? me) 2016-09-30 10:12:37 PDT
arai, is this something that might help with the slow script warning issues?
Comment 23 User image Ritu Kothari (:ritu) 2016-09-30 10:28:28 PDT
It's great to see so many verification points. Thanks everyone!
Comment 24 User image Tooru Fujisawa [:arai] 2016-09-30 10:59:58 PDT
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #22)
> arai, is this something that might help with the slow script warning issues?

No, not related.
Comment 25 User image Iulia Cristescu, QA [:IuliaC] 2016-10-03 23:28:13 PDT
Based on comment 21, I will set the corresponding flags.
Comment 26 User image Subhrajyoti Sen 2016-11-25 04:51:59 PST
The fix is verified on Beta 51.0b3

Build ID     :20161124073320
User Agenet  :Mozilla/5.0 (Windows NT 10.0; rv:51.0) Gecko/20100101 Firefox/51.0
Comment 27 User image Vibhanshu Chaudhary 2016-11-25 06:16:21 PST
It is verified on Firefox Beta 51.0b3

[testday-20161125]

Note You need to log in before you can comment on or make changes to this bug.