1116600 - Fix RegExp escaping in PlacesBackups.jsm

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

Some RegExp escaping is wrong or unneded here

http://mxr.mozilla.org/mozilla-central/search?string=new+RegExp&find=PlacesBackups


new RegExp("^bookmarks-([0-9\-]+)(?:_([0-9]+)){0,1}(?:_([a-z0-9=\+\-]{24})){0,1}\.(json(lz4)?)$", "i")

there's no need to escape - if it's at the end of a character class, as well as + inside a character class.
\. should be \\. instead to properly match a "."


new RegExp("\.json(lz4)?$");

ditto for \\.

Comment 1

[Anirudh GP(:anirudhgp)]

Hi i'd like to work on this bug. So for the first reg exp line do we have to change \.(json(lz4)?) to \\.(json(lz4)?) ?

Comment 2

[Marco Bonardo [:mak]]

(In reply to anirudh.gp from comment #1)
> Hi i'd like to work on this bug. So for the first reg exp line do we have to
> change \.(json(lz4)?) to \\.(json(lz4)?) ?

yes, and remove the other escapes in the character classes []

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Why not just change these to be RegExp literals?

Comment 4

[Marco Bonardo [:mak]]

sure, we could also do that.

Comment 5

[Anirudh GP(:anirudhgp)]

What exactly do i do ?

Comment 6

[Marco Bonardo [:mak]]

sorry, let's do what comment 3 suggests, convert new Regexp() to //, then the \. escaping should be right already.
Though you can still remove escaping on "-" and "+" when inside a character class [], since + doesn't need to be escaped there, and - doesn't need to be escaped when it's the last char in the class.

Comment 7

[Anirudh GP(:anirudhgp)]

Hi! Sorry for the late reply. I'll remove the escapes for + and - but i did not understand what i should change new Regexp() to. Should i change new RegExp("\.json(lz4)?$"); to /\.json(lz4)?$/; ?

Comment 8

[Marco Bonardo [:mak]]

(In reply to anirudh.gp from comment #7)
> Should i change new
> RegExp("\.json(lz4)?$"); to /\.json(lz4)?$/; ?

yes

Comment 9

[Anirudh GP(:anirudhgp)]

Ok I'll get on it right away. Thanks

Comment 10

[Anirudh GP(:anirudhgp)]

Attachment: bug-1116600-fix.patch

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 8548793 [details] [diff] [review]
bug-1116600-fix.patch

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

Thanks, it looks good.
I\ll push this to the Try server for you.

Comment 12

[Marco Bonardo [:mak]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bccb42f38ea8

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 8548793 [details] [diff] [review]
bug-1116600-fix.patch

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

::: toolkit/components/places/PlacesBackups.jsm
@@ +28,5 @@
>    () => Components.Constructor("@mozilla.org/file/local;1",
>                                 "nsILocalFile", "initWithPath"));
>  
>  XPCOMUtils.defineLazyGetter(this, "filenamesRegex",
> +  () => /^bookmarks-([0-9-]+)(?:_([0-9]+)){0,1}(?:_([a-z0-9=+-]{24})){0,1}\.(json(lz4)?)$/i;

ah, looks like you wrongly remove the ); here and replaced it with a simple ;
please retore ); on a new line as it was before, it was closing the defineLazyGetter definition

Comment 14

[Anirudh GP(:anirudhgp)]

Attachment: bug-1116600-fix-v2.patch

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 8549627 [details] [diff] [review]
bug-1116600-fix-v2.patch

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

thank you, let me push this again to Try.

Comment 16

[Marco Bonardo [:mak]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b197ec1f7a8

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/fadf6d3cc68c

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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