Last Comment Bug 683838 - Different regular expression result since Firefox 7
: Different regular expression result since Firefox 7
Status: VERIFIED FIXED
[qa!]
: addon-compat, dev-doc-complete, regression
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 7 Branch
: All All
: -- normal (vote)
: mozilla10
Assigned To: David Mandelin [:dmandelin]
:
Mentors:
: 691172 (view as bug list)
Depends on:
Blocks: 625600 682252 691172
  Show dependency treegraph
 
Reported: 2011-09-01 01:20 PDT by Pierre Réveillon
Modified: 2013-12-27 14:30 PST (History)
20 users (show)
dmandelin: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
affected
+
fixed
+
fixed
+
fixed


Attachments
Patch adapted from Gavin Barraclough (WebKit bug 67455) (3.86 KB, patch)
2011-10-07 18:00 PDT, David Mandelin [:dmandelin]
dmandelin: review+
christian: approval‑mozilla‑aurora+
christian: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Pierre Réveillon 2011-09-01 01:20:24 PDT
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 2011-09-01 02:09:01 PDT
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 2011-09-01 06:26:15 PDT
Triggered by:
cc36a234d0d6	David Mandelin — Bug 625600: Update Yarr import to WebKit rev 86639, r=cdleary,dvander
Comment 3 David Mandelin [:dmandelin] 2011-09-01 17:02:17 PDT
Affects WebKit trunk. Filed https://bugs.webkit.org/show_bug.cgi?id=67455
Comment 4 christian 2011-09-15 13:51:17 PDT
Sooooo, the webkit bug has had no movement...do we ship Firefox 7 with this regression?
Comment 5 Asa Dotzler [:asa] 2011-09-19 15:31:50 PDT
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] 2011-09-19 15:51:13 PDT
(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 christian 2011-09-20 18:15:59 PDT
Ok, sounds like we want to live with this then as the other benefits outweigh this issue. Thanks!
Comment 8 barraclough 2011-10-02 15:47:03 PDT
Patch attached to corresponding WebKit bug.

cheers,
G.
Comment 9 David Mandelin [:dmandelin] 2011-10-07 18:00:16 PDT
Created attachment 565698 [details] [diff] [review]
Patch adapted from Gavin Barraclough (WebKit bug 67455)
Comment 10 David Mandelin [:dmandelin] 2011-10-07 18:01:53 PDT
(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] 2011-10-07 18:04:19 PDT
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 [:bz] 2011-10-07 20:19:27 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-09 07:35:59 PDT
https://hg.mozilla.org/mozilla-central/rev/b9bae20fb35c
Comment 14 Brendan Eich [:brendan] 2011-10-09 20:15:42 PDT
(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 [:bz] 2011-10-09 20:21:11 PDT
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 16 David Mandelin [:dmandelin] 2011-10-10 17:43:01 PDT
*** Bug 691172 has been marked as a duplicate of this bug. ***
Comment 17 christian 2011-10-10 18:15:39 PDT
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] 2011-10-10 18:29:19 PDT
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] 2011-10-10 18:47:03 PDT
http://hg.mozilla.org/releases/mozilla-beta/rev/cbd186fdc8ff
Comment 20 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-10-13 11:05:33 PDT
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] 2011-10-13 17:47:12 PDT
(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] 2011-10-21 10:45:41 PDT
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 christian 2011-10-21 10:54:38 PDT
(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] 2011-10-21 11:42:36 PDT
(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] 2011-10-22 07:25:32 PDT
2 other add-on devs contacted me about regex breakage in Firefox 7.
Comment 26 AndreiD[QA] 2011-10-27 08:18:51 PDT
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 27 David Mandelin [:dmandelin] 2011-11-01 17:43:24 PDT
*** Bug 682252 has been marked as a duplicate of this bug. ***
Comment 28 Eric Shepherd [:sheppy] 2011-12-19 10:19:48 PST
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] 2011-12-20 18:22:45 PST
(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] 2011-12-30 10:21:26 PST
Noted on Firefox 10 for developers.

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