683838 - Different regular expression result since Firefox 7

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Pierre Réveillon]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:6.0.1) Gecko/20100101 Firefox/6.0.1
Build ID: 20110830092941

Steps to reproduce:

This regular expression:
var rg = /(X(?:.(?!X))*?Y)|(Y(?:.(?!Y))*?Z)/g;
var str = "Y aaa X Match1 Y aaa Y Match2 Z";
alert(JSON.stringify(str.match(rg)));


Actual results:

FF<=6: ["X Match1 Y","Y Match2 Z"]
FF>=7: ["Y Match2 Z"]


Expected results:

["X Match1 Y","Y Match2 Z"]

Comment 1

[Alice0775 White]

Regression window(m-c hourly)
Works:
http://hg.mozilla.org/mozilla-central/rev/b69d30cc0b24
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110606 Firefox/7.0a1 ID:20110606110214
Fails:
http://hg.mozilla.org/mozilla-central/rev/3589f8cefd83
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110606 Firefox/7.0a1 ID:20110606132754
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b69d30cc0b24&tochange=3589f8cefd83

Regression window(m-c hourly)
Works:
http://hg.mozilla.org/tracemonkey/rev/fe3c9a76eeae
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110524 Firefox/6.0a1 ID:20110525085637
Fails:
http://hg.mozilla.org/tracemonkey/rev/c8695a65e1e7
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110525 Firefox/6.0a1 ID:20110525145421
Pushlog:
http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=fe3c9a76eeae&tochange=c8695a65e1e7

Suspected bug:
Bug 625600 - Update Yarr import

Comment 2

[Alice0775 White]

Triggered by:
cc36a234d0d6	David Mandelin — Bug 625600: Update Yarr import to WebKit rev 86639, r=cdleary,dvander

Comment 3

[David Mandelin [:dmandelin]]

Affects WebKit trunk. Filed https://bugs.webkit.org/show_bug.cgi?id=67455

Comment 4

[LegNeato]

Sooooo, the webkit bug has had no movement...do we ship Firefox 7 with this regression?

Comment 5

[Asa Dotzler [:asa]]

cdleary, dmandelin, we've got a couple of Yarr regressions still outstanding as we're just about to ship 7. It's not clear how large this regression is and I believe we rolled forward on Yarr to get some security fixes. Should we look at backing it out? What wins did we get that help us weigh the trade-offs of these couple of regressions (this bug and bug 679936).

Comment 6

[David Mandelin [:dmandelin]]

(In reply to Asa Dotzler [:asa] from comment #5)
> cdleary, dmandelin, we've got a couple of Yarr regressions still outstanding
> as we're just about to ship 7. It's not clear how large this regression is
> and I believe we rolled forward on Yarr to get some security fixes. Should
> we look at backing it out? What wins did we get that help us weigh the
> trade-offs of these couple of regressions (this bug and bug 679936).

See the "Duplicates" and "Blocks" lists in bug 625600. There are a lot of duplicates, but I think the major things fixed by the update were: (a) lots of "regexp too complex" errors and (b) a security bug. There were also the minor issues (c) some hang with NoScript and (d) some incorrect result with backreferences.

Comment 7

[LegNeato]

Ok, sounds like we want to live with this then as the other benefits outweigh this issue. Thanks!

Comment 8

[barraclough]

Patch attached to corresponding WebKit bug.

cheers,
G.

Comment 9

[David Mandelin [:dmandelin]]

Attachment: Patch adapted from Gavin Barraclough (WebKit bug 67455)

Comment 10

[David Mandelin [:dmandelin]]

(In reply to barraclough from comment #8)
> Patch attached to corresponding WebKit bug.
> 
> cheers,
> G.

Thanks!

http://hg.mozilla.org/integration/mozilla-inbound/rev/b9bae20fb35c

Comment 11

[David Mandelin [:dmandelin]]

Comment on attachment 565698 [details] [diff] [review]
Patch adapted from Gavin Barraclough (WebKit bug 67455)

Pretty low risk, really just a 1-line change, so seems like we might want it for Fx9.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Do we want to consider it for 8?  We have at least one duplicate already, and if the patch is safe enough...

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 14

[Brendan Eich [:brendan]]

(In reply to Boris Zbarsky (:bz) from comment #12)
> Do we want to consider it for 8?  We have at least one duplicate already,
> and if the patch is safe enough...

Asa, what do you say?

/be

Comment 15

[Boris Zbarsky [:bzbarsky]]

I should have phrased that better.  It's really a question: Is the patch safe enough to consider for 8 at all?  That's really a question for someone who knows that code.

Comment 17

[LegNeato]

Comment on attachment 565698 [details] [diff] [review]
Patch adapted from Gavin Barraclough (WebKit bug 67455)

I think we'll want this regression fix and the risk is looks to be acceptable. If you disagree and you are worried about landing this in 8beta, please let me know.

Comment 18

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/327f5fdae663

I think it's good for beta too, I'll land it there next.

Comment 19

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/releases/mozilla-beta/rev/cbd186fdc8ff

Comment 20

[u279076]

How does one verify this fix given the testcase in comment 0? Do we simply copy and paste the code in Web Console?

Comment 21

[David Mandelin [:dmandelin]]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #20)
> How does one verify this fix given the testcase in comment 0? Do we simply
> copy and paste the code in Web Console?

Yes, that would work.

Comment 22

[Jorge Villalobos [:jorgev] (he/him)]

Changes to the regular expression engine should be documented, IMO. They should at least have the addon-compat flag, for future reference.

Thanks!

Comment 23

[LegNeato]

(In reply to Jorge Villalobos [:jorgev] from comment #22)
> Changes to the regular expression engine should be documented, IMO. They
> should at least have the addon-compat flag, for future reference.
> 
> Thanks!

Will do in the future, but do we really need to do this for regressions? Just curious if you think it will cause more confusion rather than help.

Comment 24

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Christian Legnitto [:LegNeato] from comment #23)
> (In reply to Jorge Villalobos [:jorgev] from comment #22)
> > Changes to the regular expression engine should be documented, IMO. They
> > should at least have the addon-compat flag, for future reference.
> > 
> > Thanks!
> 
> Will do in the future, but do we really need to do this for regressions?
> Just curious if you think it will cause more confusion rather than help.

I got here through bug 692441, which did affect at least one add-on dev. I'd rather have too many addon-compat flags than too few, so I would prefer that even minor bugs are flagged.

I don't know if sheppy has the same policy about dev-doc-needed, though.

Comment 25

[Brian King [:kinger]]

2 other add-on devs contacted me about regex breakage in Firefox 7.

Comment 26

[AndreiD[QA]]

I tested to see if the issue is still reproducible on Firefox branches, since the patch already landed (comment 19 - beta | comment 18 - aurora | comment 13 - nightly), and got the following results:
Verified Fixed on:
Mozilla/5.0 (Windows NT 6.1; rv:8.0) Gecko/20100101 Firefox/8.0 (Beta5)
Mozilla/5.0 (Windows NT 6.1; rv:9.0a2) Gecko/20111027 Firefox/9.0a2 (Aurora)
Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20111027 Firefox/10.0a1 (Nightly)

*I tested considering the steps in the description: I entered the code lines in the web console. In all cases Firefox displayed the expected results.
Setting this bug as Verified and changing the tracking keyword to qa! since the change is verified on all branches

Comment 28

[Eric Shepherd [:sheppy]]

Is it sufficient to document that "a bug in regular expression handling introduced in Firefox 7 has been fixed" or is something more specific a better idea here?

Comment 29

[David Mandelin [:dmandelin]]

(In reply to Eric Shepherd [:sheppy] from comment #28)
> Is it sufficient to document that "a bug in regular expression handling
> introduced in Firefox 7 has been fixed" or is something more specific a
> better idea here?

That's probably good enough. The actual issue is pretty technical and I can't even remember what it was.

Comment 30

[Eric Shepherd [:sheppy]]

Noted on Firefox 10 for developers.