395773 - Bookmarks backup suggests invalid file name

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

> the default name suggested by the Bookmarks Manager backup function contains
> forward slashes (Bookmarks 09/11/07.html). This can't be saved because the
> slash is the path separator on a Linux/Unix system.
(http://forums.mozillazine.org/viewtopic.php?p=3051402#3051402)

Note that this isn't a Unix-only issue. File names mustn't contain slashes on Windows, too.

My proposal is to fall back to ISO 8601 if the localized date contains slashes.

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch v2

ISO 8601 for real

Comment 2

[(not reading, please use [e-mail] instead)]

Dão, what about just replaceing / with - instead, that way at least YY MM DD will be in the correct order (for the current locale)

Comment 3

[(not reading, please use [e-mail] instead)]

Comment on attachment 280462 [details] [diff] [review]
patch v2

clearing review request, I think replacing / with - (in the date string) would be better.

Comment 4

[(not reading, please use [e-mail] instead)]

if m8 is not out the door, this would be a good one to sneak in, as this feature is new to m8.

Comment 5

[(not reading, please use [e-mail] instead)]

Attachment: patch v3, thanks to dao for getting this started

Comment 6

[Dan Mills [:thunder]]

Thanks a lot for the fix, Seth.
Do you really need the bug # in the code, though? :)

Comment 7

[Dietrich Ayala (:dietrich)]

Comment on attachment 280470 [details] [diff] [review]
patch v3, thanks to dao for getting this started

r=me. however, it'd be nice to add a separator option to nsIScriptableDateFormatter, can you please file a followup for that if you think the idea has merit?

Comment 8

[(not reading, please use [e-mail] instead)]

Comment on attachment 280470 [details] [diff] [review]
patch v3, thanks to dao for getting this started

I can remove the bug # upon checkin, if you'd like.

(I like doing that, as moving code around or changing whitespace makes cvsblame less useful over time.  but maybe that's just me?)

Comment 9

[(not reading, please use [e-mail] instead)]

I missed m8, so m9 (but relnote for m8, per mconnor)

Comment 10

[(not reading, please use [e-mail] instead)]

Dao, my apologies for stealing this from under you.  

I was hoping to sneak something into m8 this morning, but I missed m8.

Comment 11

[(not reading, please use [e-mail] instead)]

> can you please file a followup for that if you think the idea has merit?

see bug #395794, but I think it might be wontfix, but we can follow up there.

Comment 12

[Dão Gottwald [:dao]]

(In reply to comment #2)
> Dão, what about just replaceing / with - instead, that way at least YY MM DD
> will be in the correct order (for the current locale)

That's bad imho, because you can mix up the components too easily. It might work for US dates, but can you ensure that it'll work as well for other locales with slashes and that the result won't be confused with an international or English date format? As a German, something like 07-10-06 causes me headaches. That's why I chose the standard format right away.

Comment 13

[Dietrich Ayala (:dietrich)]

Using "-" is consistent with how we currently name the backups in the bookmarkbackups dir in the profile (going back to Fx2 at least).

Comment 14

[Dão Gottwald [:dao]]

I'm not against using "-" (see attachment 280462 [details] [diff] [review]). I'm against invalidating the local time format.

In fact, the dates in the bookmarkbackups dir seem to match ISO 8601.

Comment 15

[(not reading, please use [e-mail] instead)]

should we fix this bug by doing this:

# note, %1$S is year, %2$S is month, %3$S is day
# don't use : / ? (etc) because those are illegal for some operating systems
bookmarksBackupFilename=Bookmarks %1$S-%2$S-%#$S.html

note, we'd need a localization note to warn the localizer against using any illegal characters even if we don't change bookmarksBackupFilename

as for what dietrich points out, see http://lxr.mozilla.org/seamonkey/source/browser/components/places/src/nsPlacesImportExportService.cpp#2541

PR_FormatTime(timeString, 128, "bookmarks-%Y-%m-%d.html", &nowInfo);

perhaps "bookmarks-%Y-%m-%d.html" should be a string property, with a note like:

# do not localize %Y %m and %d because they are year, month and date.
# do not use : / ? (etc) because those are illegal for some operating systems
autobackupFileName=bookmarks-%Y-%m-%d.html

Comment 16

[(not reading, please use [e-mail] instead)]

> In fact, the dates in the bookmarkbackups dir seem to match ISO 8601.

What about fixing this code to always produce something like:

Bookmarks YYYY-MM-DD.html

we still need a localization note about : / ? (etc)

Comment 17

[(not reading, please use [e-mail] instead)]

Comment on attachment 280470 [details] [diff] [review]
patch v3, thanks to dao for getting this started

clearing approval, review on this patch, based on dao's comments.

Comment 18

[Dão Gottwald [:dao]]

(In reply to comment #16)
> > In fact, the dates in the bookmarkbackups dir seem to match ISO 8601.
> 
> What about fixing this code to always produce something like:
> 
> Bookmarks YYYY-MM-DD.html

Why not use the local date format if it doesn't contain "/"? That's what patch 2 does.

Comment 19

[Phil Ringnalda (:philor)]

I can't find the bug right now, but Axel's vote on whether or not you can expect localizers to know legal filename characters on all (or even all supported) platforms is in a Thunderbird RSS bug, a slightly more gently expressed version of "think again, bucko, not going to happen."

Comment 20

[Axel Hecht [:Pike]]

As a side note, without too much diving into the code in question, the localization of Firefox you're currently running has nothing to do with the encodings of your filesystem.

But yes, that entity should have a localization note. I personally prefer to use 2007-05-03, as the leading year is a globally unique sign of year, month, day, and it sorts nicely in directory listings.

Simon, do you know any date formats that have a leading year other than YYYY-MM-DD?

I'm not so worried about the date format here overall, the file has a chance to be named like the feature. The concept of back-ups itself is geeky enough for folks to remember that it's year-month-date.

The localization note should mention both that %S (why the 1$ in a single argument?) is a date formatted as ..., as well as that this is going to be a file path, so legal characters should be preferred.

Did anybody try what happens in the file dialog when you try to enter an invalid suggestion?

Comment 21

[Dão Gottwald [:dao]]

I have nothing against YYYY-MM-DD. (Well, I proposed it in the first place!) The thing is, the code to use the local format if it doesn't contain "/" is already there -- why change it?

Comment 22

[(not reading, please use [e-mail] instead)]

> Did anybody try what happens in the file dialog when you try to enter an
> invalid suggestion?

on windows, the file picker replaces illegal chars with -.

See http://mxr.mozilla.org/seamonkey/source/widget/src/windows/nsFilePicker.cpp#479

(Thank you my old friend JFD, see bug #100591)

I'm not sure about other platforms.

> The thing is, the code to use the local format if it doesn't contain "/" is
> already there -- why change it?

Again, if the local format uses : or any other illegal char, we'd have the same problem.

FYI:

as for the existing code at http://lxr.mozilla.org/seamonkey/source/browser/components/places/src/nsPlacesImportExportService.cpp#2541,

PR_FormatTime(timeString, 128, "bookmarks-%Y-%m-%d.html", &nowInfo);

see bug #305004

Comment 23

[(not reading, please use [e-mail] instead)]

for this bug, I think we should do:

1) fix the code to produce "Bookmarks YYYY-MM-DD.htm"l (note, not "bookmarks-YYYY-MM-DD.html", which is the format of the auto backup.)

2) add the localizations notes and fix the nit that axel found:

# NOTE TO LOCALIZERS
# do not localize %S, as it a date string of the format YYYY-MM-DD (ISO 8601)
# don't use : / ? (etc) because those are illegal for some operating systems
bookmarksBackupFilename=Bookmarks %S.html

3) add a comment to http://lxr.mozilla.org/seamonkey/source/browser/components/places/src/nsPlacesImportExportService.cpp#2541 about that format being ISO 8601 as well.

dao, what do you think?

Comment 24

[Dão Gottwald [:dao]]

(In reply to comment #22)
> > Did anybody try what happens in the file dialog when you try to enter an
> > invalid suggestion?
> 
> on windows, the file picker replaces illegal chars with -.
> 
> See
> http://mxr.mozilla.org/seamonkey/source/widget/src/windows/nsFilePicker.cpp#479

Is that file picker in use, actually? I get an "illegal file name" alert (in German).

> > The thing is, the code to use the local format if it doesn't contain "/" is
> > already there -- why change it?
> 
> Again, if the local format uses : or any other illegal char, we'd have the same
> problem.

Not an issue with dates, as far as I can tell. The separator always seems to be ".", "-" or "/".

Comment 25

[(not reading, please use [e-mail] instead)]

> Is that file picker in use, actually? I get an "illegal file name" alert (in
> German).

yes, on windows, at least.

I am seeing us convert "Bookmarks 9/11/2007.html" to "Bookmarks 9-11-2007.html"

stack trace:

>	gkwidget.dll!nsFilePicker::SetDefaultString(const nsAString_internal & aString={...})  Line 479	C++
 	xpcom_core.dll!NS_InvokeByIndex_P(nsISupports * that=0x00000007, unsigned int methodIndex=1, unsigned int paramCount=1238420, nsXPTCVariant * params=0x00000000)  Line 102	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=7)  Line 2326 + 0x1e bytes	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=CALL_SETTER)  Line 2326 + 0x1e bytes	C++
 	xpc3250.dll!XPCWrappedNative::SetAttribute(XPCCallContext & ccx={...})  Line 2081 + 0xe bytes	C++
 	xpc3250.dll!XPC_WN_GetterSetter(JSContext * cx=0x04d361e8, JSObject * obj=0x0628cf20, unsigned int argc=1, long * argv=0x06298f4c, long * vp=0x0012e848)  Line 1491 + 0xc bytes	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x04d361e8, unsigned int argc=1, unsigned int flags=2)  Line 1378 + 0x20 bytes	C
 	js3250.dll!js_InternalInvoke(JSContext * cx=0x04d361e8, JSObject * obj=0x0628cf20, long fval=103339904, unsigned int flags=0, unsigned int argc=1, long * argv=0x0012ef9c, long * rval=0x0012ef9c)  Line 1474 + 0x14 bytes	C
 	js3250.dll!js_InternalGetOrSet(JSContext * cx=0x04d361e8, JSObject * obj=0x0628cf20, long id=68511260, long fval=103339904, JSAccessMode mode=JSACC_WRITE, unsigned int argc=1, long * argv=0x0012ef9c, long * rval=0x0012ef9c)  Line 1546 + 0x1f bytes	C
 	js3250.dll!js_NativeSet(JSContext * cx=0x04d361e8, JSObject * obj=0x0628cf20, JSScopeProperty * sprop=0x0626a10c, long * vp=0x0012ef9c)  Line 3490 + 0x32 bytes	C
 	js3250.dll!js_SetProperty(JSContext * cx=0x04d361e8, JSObject * obj=0x0628cf20, long id=68511260, long * vp=0x0012ef9c)  Line 3749 + 0x15 bytes	C
 	js3250.dll!js_Interpret(JSContext * cx=0x04d361e8, unsigned char * pc=0x00dd6b07, long * result=0x0012f0ac)  Line 3865 + 0x147 bytes	C
 	js3250.dll!js_Invoke(JSContext * cx=0x04d361e8, unsigned int argc=1, unsigned int flags=2)  Line 1399 + 0x13 bytes	C
 	js3250.dll!js_InternalInvoke(JSContext * cx=0x04d361e8, JSObject * obj=0x0628ce40, long fval=103337568, unsigned int flags=0, unsigned int argc=1, long * argv=0x06298e50, long * rval=0x0012f234)  Line 1474 + 0x14 bytes	C
 	js3250.dll!JS_CallFunctionValue(JSContext * cx=0x04d361e8, JSObject * obj=0x0628ce40, long fval=103337568, unsigned int argc=1, long * argv=0x06298e50, long * rval=0x0012f234)  Line 4940 + 0x1f bytes	C
 	gklayout.dll!nsJSContext::CallEventHandler(nsISupports * aTarget=0x04e91388, void * aScope=0x04e05080, void * aHandler=0x0628ce60, nsIArray * aargv=0x06297020, nsIVariant * * arv=0x0012f3a8)  Line 1842 + 0x24 bytes	C++
 	gklayout.dll!nsJSEventListener::HandleEvent(nsIDOMEvent * aEvent=0x06296f3c)  Line 224 + 0x67 bytes	C++
 	gklayout.dll!nsEventListenerManager::HandleEventSubType(nsListenerStruct * aListenerStruct=0x04e91488, nsIDOMEventListener * aListener=0x04e91438, nsIDOMEvent * aDOMEvent=0x06296f3c, nsISupports * aCurrentTarget=0x04e91388, unsigned int aPhaseFlags=6)  Line 1096 + 0x12 bytes	C++
 	gklayout.dll!nsEventListenerManager::HandleEvent(nsPresContext * aPresContext=0x043eb288, nsEvent * aEvent=0x0012f718, nsIDOMEvent * * aDOMEvent=0x0012f678, nsISupports * aCurrentTarget=0x04e91388, unsigned int aFlags=6, nsEventStatus * aEventStatus=0x0012f67c)  Line 1216	C++
 	gklayout.dll!nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=6)  Line 207	C++
 	gklayout.dll!nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=6, nsDispatchingCallback * aCallback=0x00000000)  Line 266	C++
 	gklayout.dll!nsEventDispatcher::Dispatch(nsISupports * aTarget=0x04e91388, nsPresContext * aPresContext=0x043eb288, nsEvent * aEvent=0x0012f718, nsIDOMEvent * aDOMEvent=0x00000000, nsEventStatus * aEventStatus=0x0012f760, nsDispatchingCallback * aCallback=0x00000000)  Line 479 + 0x12 bytes	C++
 	gklayout.dll!nsXULElement::PreHandleEvent(nsEventChainPreVisitor & aVisitor={...})  Line 1509 + 0x29 bytes	C++
 	gklayout.dll!nsEventTargetChainItem::PreHandleEvent(nsEventChainPreVisitor & aVisitor={...})  Line 186 + 0x1c bytes	C++
 	gklayout.dll!nsEventDispatcher::Dispatch(nsISupports * aTarget=0x061aa500, nsPresContext * aPresContext=0x043eb288, nsEvent * aEvent=0x0012f934, nsIDOMEvent * aDOMEvent=0x00000000, nsEventStatus * aEventStatus=0x0012f928, nsDispatchingCallback * aCallback=0x00000000)  Line 438	C++
 	gklayout.dll!PresShell::HandleDOMEventWithTarget(nsIContent * aTargetContent=0x061aa500, nsEvent * aEvent=0x0012f934, nsEventStatus * aStatus=0x0012f928)  Line 5772 + 0x1c bytes	C++
 	gklayout.dll!nsXULMenuCommandEvent::Run()  Line 1865	C++
 	xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0012f9e8)  Line 491	C++
 	xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x00bfc390, int mayWait=1)  Line 227 + 0x16 bytes	C++
 	gkwidget.dll!nsBaseAppShell::Run()  Line 154 + 0xc bytes	C++
 	tkitcmps.dll!nsAppStartup::Run()  Line 170 + 0x1c bytes	C++
 	xul.dll!XRE_main(int argc=1, char * * argv=0x00bf8b00, const nsXREAppData * aAppData=0x00bf8ee0)  Line 3069 + 0x25 bytes	C++
 	firefox.exe!main(int argc=1, char * * argv=0x00bf8b00)  Line 153 + 0x12 bytes	C++
 	firefox.exe!__tmainCRTStartup()  Line 586 + 0x19 bytes	C
 	firefox.exe!mainCRTStartup()  Line 403	C
 	kernel32.dll!7c816fd7() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]	

Comment 26

[Dan Mills [:thunder]]

(In reply to comment #23)
> for this bug, I think we should do:
> 
> 1) fix the code to produce "Bookmarks YYYY-MM-DD.htm"l (note, not
> "bookmarks-YYYY-MM-DD.html", which is the format of the auto backup.)

Ah, that's what v1.1 of the patch in bug 374528 did:

https://bugzilla.mozilla.org/attachment.cgi?id=280157

:-)

I like your idea of putting that string in the properties file!

Comment 27

[Dão Gottwald [:dao]]

(In reply to comment #25)
Yeah, I misread the code -- it processes the default name, but I entered it manually since my system locale is DE. Also not that this doesn't work on Linux or OS X, where FILE_ILLEGAL_CHARACTERS doesn't contain "/".

(In reply to comment #23)
> dao, what do you think?

I'm still in favor of patch v2, which satisfies most users, I think.

Comment 28

[(not reading, please use [e-mail] instead)]

> I'm still in favor of patch v2, which satisfies most users, I think.

dao, one additional reason to favor ISO 8601 is that if you backup many times into a folder, it is easy to spot the "latest" by searching alphabetically.

our current format (ex:  "Bookmarks 9-11-2007") is not good for this reason.

Comment 29

[Dão Gottwald [:dao]]

Attachment: patch v4

Comment 30

[Dão Gottwald [:dao]]

Comment on attachment 280532 [details] [diff] [review]
patch v4

as suggested in comment 23

Comment 31

[Dão Gottwald [:dao]]

Attachment: patch v4.1

removing a superfluous backslash

Comment 32

[(not reading, please use [e-mail] instead)]

Comment on attachment 280533 [details] [diff] [review]
patch v4.1

r=sspitzer

note in two places you misspelled "illagel" (should be "illegal"

(fwiw, I tested from the js console, and (new Date).toLocaleFormat("%Y-%m-%d") generates what we want, specifically, 09 for september, not just 9.

Thanks again, Dao!

Comment 33

[Axel Hecht [:Pike]]

Comment on attachment 280533 [details] [diff] [review]
patch v4.1

<...>
>Index: browser/locales/en-US/chrome/browser/places/places.properties

<...>
>+# %S is the current date string of the format YYYY-MM-DD (ISO 8601)
>+# Don't use any of those characters as they are illegal for some operating systems: /\:*?"<>|

Two comments on the comment, firstly, please use the localization note format, http://developer.mozilla.org/en/docs/Localization_notes. Secondly, the date is coming in already formatted, so the "§($ stuff won't matter that much.

I'd suggest
# LOCALIZATION NOTE (bookmarksBackupFilename) :
# %S will be replaced by the current date in ISO 8601 format, YYYY-MM-DD.
# The resulting string will be suggested as a filename, so make sure that you're
# only using characters legal for file names. Consider falling back to the
# en-US value if you have to use non-ascii characters.

<...>

That might not be the best English, but it should get the message across.

Comment 34

[Axel Hecht [:Pike]]

Dao already requested the litmus test, I'd like to add that we should get this into the l10n-targeted litmus tests, too. AFAICT, this test will be platform dependent as well, and be a functionality test much more than a localization test.

Comment 35

[Axel Hecht [:Pike]]

Phil was probably talking about bug 313525. Dropping a reference to that here.

Comment 36

[Dão Gottwald [:dao]]

Attachment: patch v4.2

comment typo and l10n note fixed

Comment 37

[Marcia Knous [:marcia]]

Litmus Triage Team: Adding tchung because of Comment 34 - l10n request.

Comment 38

[(not reading, please use [e-mail] instead)]

Comment on attachment 280578 [details] [diff] [review]
patch v4.2

carrying over my r=, thanks again Dao!

Comment 39

[Reed Loden [:reed]]

Checking in browser/components/places/content/places.js;
/cvsroot/mozilla/browser/components/places/content/places.js,v  <--  places.js
new revision: 1.102; previous revision: 1.101
done
Checking in browser/components/places/src/nsPlacesImportExportService.cpp;
/cvsroot/mozilla/browser/components/places/src/nsPlacesImportExportService.cpp,v  <--  nsPlacesImportExportService.cpp
new revision: 1.29; previous revision: 1.28
done
Checking in browser/locales/en-US/chrome/browser/places/places.properties;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/places/places.properties,v  <--  places.properties
new revision: 1.28; previous revision: 1.27
done

Comment 40

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

Verified fixed in Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b1) Gecko/2007110903 Firefox/3.0b1 for the beta.

Comment 41

[Aakash Desai [:aakashd]]

It seems like the date numbering aspect of "Export HTML" has been removed. Does this bug need a test case on litmus anymore?

Comment 42

[Marcia Knous [:marcia]]

(In reply to comment #41)
> It seems like the date numbering aspect of "Export HTML" has been removed. Does
> this bug need a test case on litmus anymore?

Still needs a testcase since this is for the bookmark JSON backup, not export HTML.

Comment 43

[Aakash Desai [:aakashd]]

Cool, thanks for the advice, Marcia. The following test cases were updated on litmus for the 3.x test runs in the Library subgroup.

For 3.0,
https://litmus.mozilla.org/show_test.cgi?id=5234

For 3.1,
https://litmus.mozilla.org/show_test.cgi?id=6852

Comment 44

[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