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

Status: RESOLVED FIXED

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

Description

[[e-mail]]

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"

Comment 1

[Marco Bonardo [:mak]]

this can be caused by incompable add-ons, please check if it works in safe mode:
http://kb.mozillazine.org/Safe_Mode

Comment 2

[Matthew Turnbull [Bluefang]]

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.

Comment 3

[Marco Bonardo [:mak]]

confirmed

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

The file picker for restoring from file now filters and shows only JSON files, so the .json check can get removed?

Comment 5

[Ed]

Perhaps this could get fixed sometime before SM 3.00.0?

Comment 6

[Marco Bonardo [:mak]]

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

Comment 7

[Matt Coles]

Can I take this, new here and seems like it'd be a nice and easy start to get a feel for mozilla development?

Comment 8

[matthew.lazarow]

(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

Comment 9

[Matt Coles]

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.

Comment 10

[Matt Coles]

Attachment: change JSON file type check to lowercase

Comment 11

[Matt Coles]

Attachment: Fix change JSON file type check to lowercase

Comment 12

[Matt Coles]

Okay after a little confusion with bzexport I think I got what I wanted done.

Comment 13

[matthew.lazarow]

(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

Comment 14

[Matt Coles]

(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.

Comment 15

[matthew.lazarow]

(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!

Comment 16

[Marco Bonardo [:mak]]

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

Comment 17

[Ed]

Works as expected in Linux 2.33

Comment 18

[Marco Bonardo [:mak]]

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")) {

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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.

Comment 20

[Patrick]

Hello. Would love to work on this bug as a first bug. Please advice if it's ok to submit patch

Comment 21

[Marco Bonardo [:mak]]

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.

Comment 22

[Patrick]

Attachment: bug558566_fileFormatChange.diff

Fix - .JSON file extension type to lower case and commit change 

Please review and give feedback :)

Comment 23

[Patrick]

Please not the patch also includes the indentation. Hopefully that can be spotted right away.

Comment 24

[Patrick]

Righ. Got it. Didn't see your previous message.

Comment 25

[Marco Bonardo [:mak]]

no worries.

Comment 26

[Marco Bonardo [:mak]]

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.

Comment 27

[Pulsebot]

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

Comment 28

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/74b17234e934