Closed
Bug 1116600
Opened 10 years ago
Closed 10 years ago
Fix RegExp escaping in PlacesBackups.jsm
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: mak, Assigned: anirudhgp, Mentored)
References
Details
(Whiteboard: [good first bug][lang=js])
Attachments
(1 file, 1 obsolete file)
2.24 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
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 \\.
Assignee | ||
Comment 1•10 years ago
|
||
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)?) ?
Reporter | ||
Comment 2•10 years ago
|
||
(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•10 years ago
|
||
Why not just change these to be RegExp literals?
Reporter | ||
Comment 4•10 years ago
|
||
sure, we could also do that.
Assignee | ||
Comment 5•10 years ago
|
||
What exactly do i do ?
Reporter | ||
Comment 6•10 years ago
|
||
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.
Assignee | ||
Comment 7•10 years ago
|
||
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)?$/; ?
Reporter | ||
Comment 8•10 years ago
|
||
(In reply to anirudh.gp from comment #7)
> Should i change new
> RegExp("\.json(lz4)?$"); to /\.json(lz4)?$/; ?
yes
Assignee: nobody → anirudh.gp
Assignee | ||
Comment 9•10 years ago
|
||
Ok I'll get on it right away. Thanks
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8548793 -
Flags: review?(mak77)
Reporter | ||
Comment 11•10 years ago
|
||
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.
Attachment #8548793 -
Flags: review?(mak77) → review+
Reporter | ||
Comment 12•10 years ago
|
||
Flags: needinfo?(mak77)
Reporter | ||
Comment 13•10 years ago
|
||
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
Attachment #8548793 -
Flags: review+ → review-
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8548793 -
Attachment is obsolete: true
Attachment #8549627 -
Flags: review?(mak77)
Reporter | ||
Comment 15•10 years ago
|
||
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.
Attachment #8549627 -
Flags: review?(mak77) → review+
Reporter | ||
Comment 16•10 years ago
|
||
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(mak77)
Keywords: checkin-needed
Comment 17•10 years ago
|
||
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → mozilla38
You need to log in
before you can comment on or make changes to this bug.
Description
•