Closed Bug 558566 Opened 10 years ago Closed 4 years ago

Restoring a .JSON bookmark file returns "invalid file type error" (extension should be case-insensitive)

Categories

(Firefox :: Bookmarks & History, defect, P3)

3.6 Branch
x86
Windows 7
defect

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed

People

(Reporter: jgronlund, Assigned: mattc65, Mentored)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3

While Restoring a .JSON bookmark file it returns the following error "invalid file type"

You can fix the file very easy, the extension .JSON is by default saved as uppercase. Editing the .JSON to lowercase .json will give a perfect result, and when restoring the new .json lower in FireFox, you will receive a this will overwrite all bookmarks currently there. The message is by design, all bookmarks will be reverted to when you saved the .json..

Reproducible: Always

Steps to Reproduce:
1. Edit and Organise Bookmarks and save with the extension .JSON
2. Save the .JSON file for future restoring of backups
3. Click bookmarks, click Organize, import and backup, click backup, backing up your current .JSON file, 
4. Restore the .JSON file , click Bookmarks, click Organize, import and backup, click restore, choose file, select the .JSON file you just backed up.. You will receive the following error "invalid file type"
Actual Results:  
Restore the .JSON file , click bookmarks,  Organize bookmarks, import and backup, click restore, choose, file, select the .JSON file you just backed up.. You will receive the following error "invalid file type"

Expected Results:  
Normal restoring/restore of bookmarks as of the effective date the current .JSON file was backed up .

To fix this issue, currently known to affect Windows Vista, Windows 7, and very likely Windows XP.. May how no affinity for Mac OS X, Linux, or other Unix and/or Linux distributions..

Renaming the file extension for example.JSON,  to example.json

You will notice the only change is that the file extension is now lowercase, Being uppercase seems to be what breaks it . "is rather trivial, but it works"
Keywords: verifyme
Priority: -- → P3
Whiteboard: Restore .JSON bookmark invalid file type error
Version: unspecified → 3.6 Branch
Keywords: verifyme
Priority: P3 → --
Whiteboard: Restore .JSON bookmark invalid file type error
this can be caused by incompable add-ons, please check if it works in safe mode:
http://kb.mozillazine.org/Safe_Mode
Reported here on unknown Firefox version
http://forums.mozillazine.org/viewtopic.php?f=38&t=1997135

Confirming with today's Firefox 4.0 and Firefox 3.6 nightly, each with a new profile, on Linux.

Library -> Import and Backup -> Restore -> Choose file...

"bookmarks-2010-09-13.JSON" results in "Unsupported file type."
"bookmarks-2010-09-13.json" restores without problem

This should not be case sensitive.
confirmed
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Restoring a .JSON bookmark file returns "invalid file type error" → Restoring a .JSON bookmark file returns "invalid file type error" (extension should be case-insensitive)
Summary: Restoring a .JSON bookmark file returns "invalid file type error" (extension should be case-insensitive) → Restoring a .JSON bookmark file returns "invalid file type error" (extension should be case-insensitive); Bookmark Import from file should check for valid JSON, not if the file ends with .json
The file picker for restoring from file now filters and shows only JSON files, so the .json check can get removed?
Summary: Restoring a .JSON bookmark file returns "invalid file type error" (extension should be case-insensitive); Bookmark Import from file should check for valid JSON, not if the file ends with .json → Restoring a .JSON bookmark file returns "invalid file type error" (extension should be case-insensitive)
Perhaps this could get fixed sometime before SM 3.00.0?
this should be trivial to fix, let's make this a mentored bug

The code checking this is
http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/places.js#507
should be enough to add a couple .toLowerCase
Mentor: mak77
Whiteboard: [good first bug][lang=js]
Can I take this, new here and seems like it'd be a nice and easy start to get a feel for mozilla development?
(In reply to Matt Coles from comment #7)
> Can I take this, new here and seems like it'd be a nice and easy start to
> get a feel for mozilla development?

Hey Matt, Are you able to assign a task to yourself? I'm new here as well but couldn't reassign because it just wants to take me to a profile or email. Reading through the getting started guide I'm not seeing anything indicating that it's common to have to ask first but maybe I missed something?  Anyway I was tinkering with it and did get a build to work and actually felt conflicted by a couple solutions (trying to figure out which way was better for performance...if it even matters). If you want to take this please do and I'd be happy to share / compare notes and then I'll pickup something else when I finally figure out how to assign myself something :P
Haven't been able to assign myself anything - not sure its possible, but as far as I'm aware the solution is very simple as above and I've got the build working with(In reply to matthew.lazarow from comment #8)
> (In reply to Matt Coles from comment #7)
> > Can I take this, new here and seems like it'd be a nice and easy start to
> > get a feel for mozilla development?
> 
> Hey Matt, Are you able to assign a task to yourself? I'm new here as well
> but couldn't reassign because it just wants to take me to a profile or
> email. Reading through the getting started guide I'm not seeing anything
> indicating that it's common to have to ask first but maybe I missed
> something?  Anyway I was tinkering with it and did get a build to work and
> actually felt conflicted by a couple solutions (trying to figure out which
> way was better for performance...if it even matters). If you want to take
> this please do and I'd be happy to share / compare notes and then I'll
> pickup something else when I finally figure out how to assign myself
> something :P

No I'm not able to assign myself to it, and I've got a working build and I'm making a patch now, the solution seems very simple though, simply adding .toLowerCase to the file paths in the if statement on line #507, not sure what else you would do but I'm all ears.
Attachment #8631862 - Attachment is obsolete: true
Okay after a little confusion with bzexport I think I got what I wanted done.
(In reply to Matt Coles from comment #9)
> Haven't been able to assign myself anything - not sure its possible, but as
> far as I'm aware the solution is very simple as above and I've got the build
> working with(In reply to matthew.lazarow from comment #8)
> > (In reply to Matt Coles from comment #7)
> > > Can I take this, new here and seems like it'd be a nice and easy start to
> > > get a feel for mozilla development?
> > 
> > Hey Matt, Are you able to assign a task to yourself? I'm new here as well
> > but couldn't reassign because it just wants to take me to a profile or
> > email. Reading through the getting started guide I'm not seeing anything
> > indicating that it's common to have to ask first but maybe I missed
> > something?  Anyway I was tinkering with it and did get a build to work and
> > actually felt conflicted by a couple solutions (trying to figure out which
> > way was better for performance...if it even matters). If you want to take
> > this please do and I'd be happy to share / compare notes and then I'll
> > pickup something else when I finally figure out how to assign myself
> > something :P
> 
> No I'm not able to assign myself to it, and I've got a working build and I'm
> making a patch now, the solution seems very simple though, simply adding
> .toLowerCase to the file paths in the if statement on line #507, not sure
> what else you would do but I'm all ears.

Hey Matt, I did that first but then looked at it and thought because its an inclusive 'And' it means we're never going to shortcut and both conditions would have to be checked. So because of that just adding ToLowerCase on both conditions feels redundant. So I was torn then between if it's actually better to save the ToLowerCase result to a new variable, and do the ends with condition check on that instead.  I do a lot of C# at work and ran into a similar condition before and was shocked because a simple change like that on an file parser we built ended up giving us amazing gains in performance. Ever since I always pause and try to reconsider a lot of string manipulation statements like this one. It probably really doesn't matter given its just importing bookmarks huh :P
(In reply to matthew.lazarow from comment #13)
> (In reply to Matt Coles from comment #9)
> > Haven't been able to assign myself anything - not sure its possible, but as
> > far as I'm aware the solution is very simple as above and I've got the build
> > working with(In reply to matthew.lazarow from comment #8)
> > > (In reply to Matt Coles from comment #7)
> > > > Can I take this, new here and seems like it'd be a nice and easy start to
> > > > get a feel for mozilla development?
> > > 
> > > Hey Matt, Are you able to assign a task to yourself? I'm new here as well
> > > but couldn't reassign because it just wants to take me to a profile or
> > > email. Reading through the getting started guide I'm not seeing anything
> > > indicating that it's common to have to ask first but maybe I missed
> > > something?  Anyway I was tinkering with it and did get a build to work and
> > > actually felt conflicted by a couple solutions (trying to figure out which
> > > way was better for performance...if it even matters). If you want to take
> > > this please do and I'd be happy to share / compare notes and then I'll
> > > pickup something else when I finally figure out how to assign myself
> > > something :P
> > 
> > No I'm not able to assign myself to it, and I've got a working build and I'm
> > making a patch now, the solution seems very simple though, simply adding
> > .toLowerCase to the file paths in the if statement on line #507, not sure
> > what else you would do but I'm all ears.
> 
> Hey Matt, I did that first but then looked at it and thought because its an
> inclusive 'And' it means we're never going to shortcut and both conditions
> would have to be checked. So because of that just adding ToLowerCase on both
> conditions feels redundant. So I was torn then between if it's actually
> better to save the ToLowerCase result to a new variable, and do the ends
> with condition check on that instead.  I do a lot of C# at work and ran into
> a similar condition before and was shocked because a simple change like that
> on an file parser we built ended up giving us amazing gains in performance.
> Ever since I always pause and try to reconsider a lot of string manipulation
> statements like this one. It probably really doesn't matter given its just
> importing bookmarks huh :P

Uh, last I checked javascript won't evaluate both conditions if the first one is false.
(In reply to Matt Coles from comment #14)
> (In reply to matthew.lazarow from comment #13)
> > (In reply to Matt Coles from comment #9)
> > > Haven't been able to assign myself anything - not sure its possible, but as
> > > far as I'm aware the solution is very simple as above and I've got the build
> > > working with(In reply to matthew.lazarow from comment #8)
> > > > (In reply to Matt Coles from comment #7)
> > > > > Can I take this, new here and seems like it'd be a nice and easy start to
> > > > > get a feel for mozilla development?
> > > > 
> > > > Hey Matt, Are you able to assign a task to yourself? I'm new here as well
> > > > but couldn't reassign because it just wants to take me to a profile or
> > > > email. Reading through the getting started guide I'm not seeing anything
> > > > indicating that it's common to have to ask first but maybe I missed
> > > > something?  Anyway I was tinkering with it and did get a build to work and
> > > > actually felt conflicted by a couple solutions (trying to figure out which
> > > > way was better for performance...if it even matters). If you want to take
> > > > this please do and I'd be happy to share / compare notes and then I'll
> > > > pickup something else when I finally figure out how to assign myself
> > > > something :P
> > > 
> > > No I'm not able to assign myself to it, and I've got a working build and I'm
> > > making a patch now, the solution seems very simple though, simply adding
> > > .toLowerCase to the file paths in the if statement on line #507, not sure
> > > what else you would do but I'm all ears.
> > 
> > Hey Matt, I did that first but then looked at it and thought because its an
> > inclusive 'And' it means we're never going to shortcut and both conditions
> > would have to be checked. So because of that just adding ToLowerCase on both
> > conditions feels redundant. So I was torn then between if it's actually
> > better to save the ToLowerCase result to a new variable, and do the ends
> > with condition check on that instead.  I do a lot of C# at work and ran into
> > a similar condition before and was shocked because a simple change like that
> > on an file parser we built ended up giving us amazing gains in performance.
> > Ever since I always pause and try to reconsider a lot of string manipulation
> > statements like this one. It probably really doesn't matter given its just
> > importing bookmarks huh :P
> 
> Uh, last I checked javascript won't evaluate both conditions if the first
> one is false.

**** you're right it will short circuit there; dang '!' got me again. Happy coding!
Attachment #8631864 - Flags: review?(mak77)
sorry for late, I was away for a couple days, I will look at this shortly.

I hope you found all the documentation needed to start contributing, most is linked from this page https://developer.mozilla.org/en-US/docs/Introduction
You can reach the firefox team in the #fx-team channel, and new contributors chat in #introduction
Works as expected in Linux 2.33
Comment on attachment 8631864 [details] [diff] [review]
Fix  change JSON file type check to lowercase

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

Please fix the commit message so it's like:

Bug 558566 - Restoring bookmarks from a backup should case-insensitively check the file extension. r=mak

::: browser/components/places/content/places.js
@@ +503,5 @@
>     * Restores bookmarks from a JSON file.
>     */
>    restoreBookmarksFromFile: function PO_restoreBookmarksFromFile(aFilePath) {
>      // check file extension
> +    if (!aFilePath.toLowerCase().endsWith("json") && !aFilePath.toLowerCase().endsWith("jsonlz4"))  {

please reindent as

if (!aFilePath.toLowerCase().endsWith("json") &&
    !aFilePath.toLowerCase().endsWith("jsonlz4")) {
Attachment #8631864 - Flags: review?(mak77) → review+
Hi Matt, your patch passed review. Can you change the commit message and indentation like in mentioned in comment 18 and attach the new patch here, please? After that, it should be ready for checkin.
Flags: needinfo?(mattc65)
Priority: -- → P3
Hello. Would love to work on this bug as a first bug. Please advice if it's ok to submit patch
Hi, we already have a reviewed patch here that just needs to be finalized, I will just do that and land.
Please look for another mentored bug that needs help.
Flags: needinfo?(mak77)
Attached patch bug558566_fileFormatChange.diff (obsolete) — Splinter Review
Fix - .JSON file extension type to lower case and commit change 

Please review and give feedback :)
Attachment #8695882 - Flags: review?(mak77)
Please not the patch also includes the indentation. Hopefully that can be spotted right away.
Righ. Got it. Didn't see your previous message.
no worries.
Flags: needinfo?(mattc65)
Flags: needinfo?(mak77)
Comment on attachment 8695882 [details] [diff] [review]
bug558566_fileFormatChange.diff

I'm sorry, this is not what the review comment was requesting...
I can help you in a separate mentored bug if you found an unowned one that looks interesting to you.

I will just directly take care of landing the previous fix with my comments addressed on top.
Attachment #8695882 - Attachment is obsolete: true
Attachment #8695882 - Flags: review?(mak77)
Assignee: nobody → mattc65
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/74b17234e934
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
You need to log in before you can comment on or make changes to this bug.