JSON bookmarks backup has localized filename (and can't be easily restored)

VERIFIED FIXED in Firefox 3.1b2

Status

()

P1
normal
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: u81239, Assigned: dietrich)

Tracking

(4 keywords)

unspecified
Firefox 3.1b2
privacy, regression, verified1.9.0.11, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3.5 +
wanted1.9.0.x +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [has patch][has review])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

10 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; nl; rv:1.9) Gecko/2008052906 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; nl; rv:1.9) Gecko/2008052906 Firefox/3.0

Hi,

Using the Dutch localisation, when I look in the bookmarkbackups folder of my profile I see the following files:

bladwijzers-2008-07-16.json
bookmarks-2008-03-13.html

The .html version is a left-over from before the JSON bookmark backups were put into effect. However, this made me notice that the file name changed along with the file format.

It seems that for the JSON version of the bookmarks backup, the localized name is used? (But not a localised date!) That looks like a bug to me. The filename should not contain any locale-specific parts. There is no precedent for that in the rest of the Firefox profile’s filenames, and no good reason to either.

The file name of the JSON file should be the same as the HTML one was. Having a localised part in the filename can lead to unexpected bugs (e.g. does the part that restores a bookmarks backup also use the localised name?).

~Grauw

Reproducible: Always

Steps to Reproduce:
1. Install and use a non-English version of Firefox
2. Go to C:\Users\…\AppData\Roaming\Mozilla\Firefox\Profiles\…\bookmarkbackups
3. Observe how the JSON bookmark backups contain a localised name, e.g. bladwijzers-2008-07-16.json

Comment 1

10 years ago
I guess the even bigger problem is that our own code expects the files to start with "bookmarks-" <http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/utils.js#1621> which prevents them from showing up in the Library...
Status: UNCONFIRMED → NEW
Component: Bookmarks & History → Places
Ever confirmed: true
Flags: wanted1.9.0.x?
Flags: blocking-firefox3.1?
Keywords: regression
QA Contact: bookmarks → places
Summary: JSON bookmarks backup has localized filename → JSON bookmarks backup has localized filename (and can't be easily restored)
(Assignee)

Updated

10 years ago
Flags: wanted1.9.0.x?
Flags: blocking-firefox3.1?
Flags: blocking-firefox3.1+
OS: Windows Vista → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → Firefox 3.1b2

Updated

10 years ago
Blocks: 450363
(Assignee)

Updated

10 years ago
Target Milestone: Firefox 3.1b2 → Firefox 3.1

Updated

10 years ago
Blocks: 448969

Updated

10 years ago
Blocks: 447652
(Assignee)

Comment 2

10 years ago
Created attachment 347095 [details] [diff] [review]
v1

de-localize bookmark backup filenames. also removes an unused string.
Assignee: nobody → dietrich
Attachment #347095 - Flags: review?(mano)

Comment 3

10 years ago
This will leave backup files with a localized name lying around indefinitely. Won't this be a potential privacy risk?
(Assignee)

Updated

10 years ago
Attachment #347095 - Flags: review?(mano)
(Assignee)

Comment 4

10 years ago
Comment on attachment 347095 [details] [diff] [review]
v1

per comment #3, yes. will add code to clean those up.
(Assignee)

Comment 5

10 years ago
Created attachment 347470 [details] [diff] [review]
v2

this displays existing localized and un-localized backups in the menu and removes both per the normal archive-removal policy.
Attachment #347095 - Attachment is obsolete: true
Attachment #347470 - Flags: review?(zeniko)
(Assignee)

Updated

10 years ago
Whiteboard: [has patch][needs review zeniko]

Comment 6

10 years ago
Comment on attachment 347470 [details] [diff] [review]
v2

>+      var rx = new RegExp("^(bookmarks|" + localizedFilenamePrefix + ")-.+\.json$");

AFAICT this will catch all but the mn translation (where there seems to be text after .json). And nobody's been using parentheses and friends, so we should be fine without escaping the prefix.

>+        if (backupName.substr(0, backupFilenamePrefix.length) == backupFilenamePrefix ||
>+            backupName.substr(0, localizedFilenamePrefix.length) == localizedFilenamePrefix) {

Why not use the same RegExp as in places.js for consistency?

>+# Do not change this string!
>+bookmarksArchiveFilename=beluga-%S.json

Please, listen to yourself! ;-)
If you want to make our L10n community aware of this change, rather change the property's name to legacyBookmarksArchiveFilename or similar.

r+=me and thanks with this fixed.
Attachment #347470 - Flags: review?(zeniko) → review+
(Assignee)

Comment 7

10 years ago
(In reply to comment #6)
> >+# Do not change this string!
> >+bookmarksArchiveFilename=beluga-%S.json
> 
> Please, listen to yourself! ;-)
> If you want to make our L10n community aware of this change, rather change the
> property's name to legacyBookmarksArchiveFilename or similar.

Er, that was left in from testing the change, not at all intended as a new l10n notification mechanism :)

http://bit.ly/GYSR
(Assignee)

Comment 8

10 years ago
(In reply to comment #6)
> (From update of attachment 347470 [details] [diff] [review])
> >+      var rx = new RegExp("^(bookmarks|" + localizedFilenamePrefix + ")-.+\.json$");
> 
> AFAICT this will catch all but the mn translation (where there seems to be text
> after .json).

how about: "^(bookmarks|" + localizedFilenamePrefix + ")-.+\.json"

> And nobody's been using parentheses and friends, so we should be
> fine without escaping the prefix.

are you referring to the "."?
(Assignee)

Comment 9

10 years ago
Created attachment 347562 [details] [diff] [review]
v2.1

fixes comments
Attachment #347470 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Whiteboard: [has patch][needs review zeniko] → [has patch]

Comment 10

10 years ago
(In reply to comment #8)
> how about: "^(bookmarks|" + localizedFilenamePrefix + ")-.+\.json"

That should do it, yes. Next time, please don't let the file extension be localizable... ;-)

> are you referring to the "."?

No, I was referring to the various translations of the word "bookmark".

On second thought, the following could remain localizable, as AFAICT we won't try to automatically restore from these:

-bookmarksBackupFilename=Bookmarks %S.html
-bookmarksBackupFilenameJSON=Bookmarks %S.json

Should we keep these, the file extension should still be moved into code, though.
(Assignee)

Comment 11

10 years ago
Created attachment 347697 [details] [diff] [review]
v2.2

yes, those strings can stay localized, and we'd probably get complaints if we de-localized them. i'm leaving the file extension in for now, since we're long past string freeze.
Attachment #347562 - Attachment is obsolete: true
Attachment #347697 - Flags: approval1.9.1b2?
(Assignee)

Updated

10 years ago
Whiteboard: [has patch] → [has patch][has review]
(Assignee)

Comment 12

10 years ago
Created attachment 347698 [details] [diff] [review]
v2.3

... and this one actually contains the regexp change mentioned above.
Attachment #347697 - Attachment is obsolete: true
Attachment #347698 - Flags: approval1.9.1b2?
Attachment #347697 - Flags: approval1.9.1b2?
Comment on attachment 347698 [details] [diff] [review]
v2.3

a=beltzner
Attachment #347698 - Flags: approval1.9.1b2? → approval1.9.1b2+
Pushed: http://hg.mozilla.org/mozilla-central/rev/8c92737298f6
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 3.1 → Firefox 3.1b2

Comment 15

10 years ago
I'd like to get this backported to the 1.9.0.x branch.

Dietrich, can you check if this patch would work as is or needs real backporting effort?
Flags: wanted1.9.0.x?
Flags: blocking1.9.0.5?
Just to make things clear: if I'm reading to code properly - we should not de-localize bookmarksArchiveFilename if already localized, because Firefox won't find localized backups then. If so, you should definetely mention this in the localization note.
Wanted for branch, but definitely not blocking the release whose codefreeze is in a couple days. We'll want trunk baking time and branch tests for this.
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.5?
Keywords: privacy
(Assignee)

Updated

10 years ago
Attachment #347698 - Flags: review?(mano)
(Assignee)

Comment 18

10 years ago
Comment on attachment 347698 [details] [diff] [review]
v2.3

grr, i just realized i got initial review from simon, and no review from a valid browser or toolkit peer.
(Assignee)

Comment 19

10 years ago
(In reply to comment #16)
> Just to make things clear: if I'm reading to code properly - we should not
> de-localize bookmarksArchiveFilename if already localized, because Firefox
> won't find localized backups then. If so, you should definetely mention this in
> the localization note.

the latest version of the patch finds both localized and non-localized files in all scenarios (menu population, getting latest backup, etc), but only generates non-localized files.
Comment on attachment 347698 [details] [diff] [review]
v2.3

r=valid-something.
Attachment #347698 - Flags: review?(mano) → review+
Keywords: fixed1.9.1

Updated

10 years ago
Duplicate of this bug: 457665
verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; es-ES; rv:1.9.1b2) Gecko/20081201 Firefox/3.1b2
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1

Updated

10 years ago
Duplicate of this bug: 481157
Comment on attachment 347698 [details] [diff] [review]
v2.3

i think this should be fixed for 3.0.9, since actually we save backups with localized names and go looking for unlocalized names.
Attachment #347698 - Flags: approval1.9.0.9?
Attachment #347698 - Flags: approval1.9.0.9? → approval1.9.0.10?
Attachment #347698 - Flags: approval1.9.0.10? → approval1.9.0.10+
Comment on attachment 347698 [details] [diff] [review]
v2.3

Approved for 1.9.0.10, a=dveditz for release-drivers
Can we get automated tests for this on all branches and trunk?
Flags: in-testsuite?
(Assignee)

Comment 27

10 years ago
i have a 1.9.0 patch for this, will check in when the tree is clean
(Assignee)

Comment 28

10 years ago
Checking in browser/components/places/content/places.js;
/cvsroot/mozilla/browser/components/places/content/places.js,v  <--  places.js
new revision: 1.168; previous revision: 1.167
done
Checking in toolkit/components/places/src/utils.js;
/cvsroot/mozilla/toolkit/components/places/src/utils.js,v  <--  utils.js
new revision: 1.25; previous revision: 1.24
done
Checking in toolkit/locales/en-US/chrome/places/places.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/places/places.properties,v  <--  places.properties
new revision: 1.8; previous revision: 1.7
done
Keywords: fixed1.9.0.10
Verified for 1.9.0.11 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; de; rv:1.9.0.11pre) Gecko/2009051304 GranParadiso/3.0.11pre. This is fixed now. New names are non-localized but the older, localized, files are viewable as options in the Bookmarks Library.
Keywords: fixed1.9.0.11 → verified1.9.0.11

Updated

9 years ago
Blocks: 508265

Updated

9 years ago
No longer blocks: 508265
Depends on: 508265
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.