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

Status: VERIFIED FIXED

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

Description

[u81239]

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

[Simon Bünzli]

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

Comment 2

[Dietrich Ayala (:dietrich)]

Attachment: v1

de-localize bookmark backup filenames. also removes an unused string.

Comment 3

[Simon Bünzli]

This will leave backup files with a localized name lying around indefinitely. Won't this be a potential privacy risk?

Comment 4

[Dietrich Ayala (:dietrich)]

Comment on attachment 347095 [details] [diff] [review]
v1

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

Comment 5

[Dietrich Ayala (:dietrich)]

Attachment: v2

this displays existing localized and un-localized backups in the menu and removes both per the normal archive-removal policy.

Comment 6

[Simon Bünzli]

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.

Comment 7

[Dietrich Ayala (:dietrich)]

(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

Comment 8

[Dietrich Ayala (:dietrich)]

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

Comment 9

[Dietrich Ayala (:dietrich)]

Attachment: v2.1

fixes comments

Comment 10

[Simon Bünzli]

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

Comment 11

[Dietrich Ayala (:dietrich)]

Attachment: 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.

Comment 12

[Dietrich Ayala (:dietrich)]

Attachment: v2.3

... and this one actually contains the regexp change mentioned above.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

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

a=beltzner

Comment 14

[Dave Townsend [:mossop]]

Pushed: http://hg.mozilla.org/mozilla-central/rev/8c92737298f6

Comment 15

[Axel Hecht [:Pike]]

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?

Comment 16

[Vlado Valastiak [:wladow]]

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.

Comment 17

[Daniel Veditz [:dveditz]]

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.

Comment 18

[Dietrich Ayala (:dietrich)]

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.

Comment 19

[Dietrich Ayala (:dietrich)]

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

[Asaf Romano (gone)]

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

r=valid-something.

Comment 22

[Tracy Walker [:tracy]]

verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; es-ES; rv:1.9.1b2) Gecko/20081201 Firefox/3.1b2

Comment 24

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 25

[Daniel Veditz [:dveditz]]

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

Approved for 1.9.0.10, a=dveditz for release-drivers

Comment 26

[Samuel Sidler (old account; do not CC)]

Can we get automated tests for this on all branches and trunk?

Comment 27

[Dietrich Ayala (:dietrich)]

i have a 1.9.0 patch for this, will check in when the tree is clean

Comment 28

[Dietrich Ayala (:dietrich)]

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

Comment 29

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 30

[Gervase Markham [:gerv]]

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