Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

VERIFIED FIXED in Firefox 50

Status

()

Core
JavaScript Engine
VERIFIED FIXED
10 months ago
8 months ago

People

(Reporter: Patrick Dähne, Assigned: arai)

Tracking

({regression})

46 Branch
mozilla52
regression
Points:
---

Firefox Tracking Flags

(firefox49 wontfix, firefox50- verified, firefox51+ verified, firefox52+ verified)

Details

Attachments

(1 attachment)

(Reporter)

Description

10 months ago
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

10 months ago
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
Blocks: 1207922
Component: Untriaged → JavaScript Engine: JIT
Flags: needinfo?(arai.unmht)
Keywords: regression
Product: Firefox → Core
Version: 49 Branch → 46 Branch
(Assignee)

Comment 2

10 months ago
thank you for pointing it out!
optimization for the trailing ".*" goes wrong.
Assignee: nobody → arai.unmht
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 3

10 months ago
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.
Flags: needinfo?(arai.unmht)

Updated

10 months ago
status-firefox49: --- → affected
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → affected
tracking-firefox50: --- → ?
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
(Assignee)

Comment 4

10 months ago
I was wrong.
We have been used non-matchOnly execution for global/sticky test.
Will try adding back equivalent.
(Assignee)

Comment 5

10 months ago
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.
Attachment #8793914 - Flags: review?(hv1989)
(Assignee)

Updated

10 months ago
Component: JavaScript Engine: JIT → JavaScript Engine
Summary: RegExp.test does not correctly set RegExp.lastIndex → RegExp.test with global or sticky flags does not correctly set RegExp.lastIndex
Comment on attachment 8793914 [details] [diff] [review]
Do not ignore trailing .* on matchOnly RegExp execution.

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

Looks good.
Attachment #8793914 - Flags: review?(hv1989) → review+
Tracking 52+ for this JS regression.
tracking-firefox52: ? → +
(Assignee)

Comment 8

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

Comment 9

10 months ago
Track for 51+ as this is JS regression.
tracking-firefox51: ? → +
(Assignee)

Comment 10

10 months ago
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
Attachment #8793914 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 11

10 months ago
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
Attachment #8793914 - Flags: approval-mozilla-beta?

Comment 12

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f38006377575
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Comment 13

10 months ago
Hi Patrick, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(pdaehne)

Comment 14

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

Fixes a JS regression, Aurora51+, Beta50+
Attachment #8793914 - Flags: approval-mozilla-beta?
Attachment #8793914 - Flags: approval-mozilla-beta+
Attachment #8793914 - Flags: approval-mozilla-aurora?
Attachment #8793914 - Flags: approval-mozilla-aurora+

Comment 15

10 months ago
Don't feel the need to track it now that it's fixed.
tracking-firefox50: ? → -

Comment 16

10 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/93f470ecdb9b
status-firefox51: affected → fixed

Comment 17

10 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/2704e3050659
status-firefox50: affected → fixed
(Reporter)

Comment 18

10 months ago
It's working now as expected in the Nightly build. Thanks for fixing!
(Reporter)

Updated

10 months ago
Flags: needinfo?(pdaehne)
QA Whiteboard: [good first verify]

Comment 19

10 months ago
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

10 months ago
Fixed on Firefox 50.0b3 using Linux Mint 18

Comment 21

10 months ago
It's working as expected on Firefox 50.0b3 on Windows 10 Pro
arai, is this something that might help with the slow script warning issues?
Flags: needinfo?(arai.unmht)

Comment 23

10 months ago
It's great to see so many verification points. Thanks everyone!
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
(Assignee)

Comment 24

10 months ago
(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.
Flags: needinfo?(arai.unmht)
Based on comment 21, I will set the corresponding flags.
status-firefox50: fixed → verified
status-firefox49: affected → wontfix

Comment 26

8 months ago
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

8 months ago
It is verified on Firefox Beta 51.0b3

[testday-20161125]
status-firefox51: fixed → verified
You need to log in before you can comment on or make changes to this bug.