Closed Bug 683838 Opened 13 years ago Closed 13 years ago

Different regular expression result since Firefox 7

Categories

(Core :: JavaScript Engine, defect)

7 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla10
Tracking Status
firefox7 + affected
firefox8 + fixed
firefox9 + fixed
firefox10 + fixed

People

(Reporter: pierre, Assigned: dmandelin)

References

Details

(Keywords: addon-compat, dev-doc-complete, regression, Whiteboard: [qa!])

Attachments

(1 file)

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"]
OS: Linux → All
Hardware: x86_64 → All
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
Blocks: 625600
Keywords: regression
Triggered by:
cc36a234d0d6	David Mandelin — Bug 625600: Update Yarr import to WebKit rev 86639, r=cdleary,dvander
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: js-triage-needed
Assignee: general → cdleary
Affects WebKit trunk. Filed https://bugs.webkit.org/show_bug.cgi?id=67455
Sooooo, the webkit bug has had no movement...do we ship Firefox 7 with this regression?
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).
(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.
Ok, sounds like we want to live with this then as the other benefits outweigh this issue. Thanks!
Patch attached to corresponding WebKit bug.

cheers,
G.
Blocks: 691172
Assignee: cdleary → dmandelin
Whiteboard: js-triage-needed → js-triage-done
(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
Status: NEW → ASSIGNED
Whiteboard: js-triage-done → [inbound]
Flags: in-testsuite+
Whiteboard: [inbound]
Target Milestone: --- → mozilla10
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.
Attachment #565698 - Flags: approval-mozilla-aurora?
Do we want to consider it for 8?  We have at least one duplicate already, and if the patch is safe enough...
https://hg.mozilla.org/mozilla-central/rev/b9bae20fb35c
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
(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
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 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.
Attachment #565698 - Flags: approval-mozilla-beta+
Attachment #565698 - Flags: approval-mozilla-aurora?
Attachment #565698 - Flags: approval-mozilla-aurora+
http://hg.mozilla.org/releases/mozilla-aurora/rev/327f5fdae663

I think it's good for beta too, I'll land it there next.
How does one verify this fix given the testcase in comment 0? Do we simply copy and paste the code in Web Console?
Whiteboard: [qa?]
(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.
Whiteboard: [qa?] → [qa+]
Changes to the regular expression engine should be documented, IMO. They should at least have the addon-compat flag, for future reference.

Thanks!
(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.
(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.
2 other add-on devs contacted me about regex breakage in Firefox 7.
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
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]
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?
(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.
Noted on Firefox 10 for developers.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: