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

RESOLVED FIXED in Firefox 45

Status

()

P3
normal
RESOLVED FIXED
9 years ago
3 years ago

People

(Reporter: jgronlund, Assigned: mattc65, Mentored)

Tracking

3.6 Branch
Firefox 45
x86
Windows 7
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

9 years ago
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"
(Reporter)

Updated

9 years ago
Keywords: verifyme
Priority: -- → P3
Whiteboard: Restore .JSON bookmark invalid file type error
Version: unspecified → 3.6 Branch

Updated

9 years ago
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)

Comment 5

3 years ago
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]
(Assignee)

Comment 7

3 years ago
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

3 years ago
(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
(Assignee)

Comment 9

3 years ago
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.
(Assignee)

Comment 10

3 years ago
Created attachment 8631862 [details] [diff] [review]
change JSON file type check to lowercase
(Assignee)

Comment 11

3 years ago
Created attachment 8631864 [details] [diff] [review]
Fix  change JSON file type check to lowercase
(Assignee)

Updated

3 years ago
Attachment #8631862 - Attachment is obsolete: true
(Assignee)

Comment 12

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

Comment 13

3 years ago
(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
(Assignee)

Comment 14

3 years ago
(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

3 years ago
(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!
(Assignee)

Updated

3 years ago
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

Comment 17

3 years ago
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)

Updated

3 years ago
Priority: -- → P3

Comment 20

3 years ago
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)

Comment 22

3 years ago
Created attachment 8695882 [details] [diff] [review]
bug558566_fileFormatChange.diff

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

Please review and give feedback :)
Attachment #8695882 - Flags: review?(mak77)

Comment 23

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

Comment 24

3 years ago
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)

Updated

3 years ago
Assignee: nobody → mattc65
Status: NEW → ASSIGNED

Comment 28

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/74b17234e934
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
You need to log in before you can comment on or make changes to this bug.