1304737 - RegExp.test with global or sticky flags does not correctly set RegExp.lastIndex

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Patrick Dähne]

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

[Loic]

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

[Tooru Fujisawa [:arai]]

thank you for pointing it out!
optimization for the trailing ".*" goes wrong.

Comment 3

[Tooru Fujisawa [:arai]]

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

[Tooru Fujisawa [:arai]]

I was wrong.
We have been used non-matchOnly execution for global/sticky test.
Will try adding back equivalent.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: 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

[Hannes Verschore [:h4writer]]

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

[Marcia Knous [:marcia]]

Tracking 52+ for this JS regression.

Comment 8

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3800637757590adbf18a09c18b9351a2d04fa76
Bug 1304737 - Do not ignore trailing .* on matchOnly RegExp execution. r=h4writer

Comment 9

[Gerry Chang [:gchang]]

Track for 51+ as this is JS regression.

Comment 10

[Tooru Fujisawa [:arai]]

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

[Tooru Fujisawa [:arai]]

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

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/f38006377575

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Patrick, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8793914 [details] [diff] [review]
Do not ignore trailing .* on matchOnly RegExp execution.

Fixes a JS regression, Aurora51+, Beta50+

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Don't feel the need to track it now that it's fixed.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/93f470ecdb9b

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2704e3050659

Comment 18

[Patrick Dähne]

It's working now as expected in the Nightly build. Thanks for fixing!

Comment 19

[Saddam Hossain]

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

[Subhrajyoti Sen]

Fixed on Firefox 50.0b3 using Linux Mint 18

Comment 21

[Vibhanshu Chaudhary]

It's working as expected on Firefox 50.0b3 on Windows 10 Pro

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

arai, is this something that might help with the slow script warning issues?

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

It's great to see so many verification points. Thanks everyone!

Comment 24

[Tooru Fujisawa [:arai]]

(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

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Based on comment 21, I will set the corresponding flags.

Comment 26

[Subhrajyoti Sen]

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

[Vibhanshu Chaudhary]

It is verified on Firefox Beta 51.0b3

[testday-20161125]