302387 - rdf_FormatDate() generates incorrect timezone information

Status: RESOLVED FIXED

(Core Graveyard :: RDF, defect)

Description

[Todd Agulnick]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6

Under Windows XP, rdf_FormatDate() generates non-abbreviated timzone
specifications which rdf_ParseDate() cannot read. For example, rdf_FormatDate()
generates dates that look like this:

Fri Jul 15 11:07:05 Pacific Daylight Time 2005 +000000

instead of 

Fri Jul 15 11:07:05 PDT 2005 +000000

Reproducible: Always

Steps to Reproduce:
Use XMLSerializer to generate XML for an RDF datastore containing date
information, such as rdf:Bookmarks.


Actual Results:  
See "Details"

Expected Results:  
See "Details"

Comment 1

[Ian McKellar]

I'm seeing this too. It's causing problems in Flock.

The problem seems to be that Windows XP can't provide the abreviated timezone. One solution might be to always save RDF dates in GMT.

Comment 2

[Ian McKellar]

Attachment: This patch modifies rdf_FormatDate to always store GMT time

This should resolve this bug and not introduce any issues on any other platforms. It does change the behaviour subtly - dates are always stored in their GMT form rather than in locale-specific form, but this will actually fix potential other bugs since the date parsing for RDF only understands half a dozen timezones anyway.

Comment 3

[Axel Hecht]

This should fix bug 274567, too.

Do we really need to explicitly specify GMT in the format string?

Comment 4

[O. Atsushi (Torisugari)]

IMHO, we should not use nsIRDFDate anywhere in any product for a while.
For example, fx's download manager is using that interface.
So my firefox claims I've downloaded a nightly build
the day after tomorrow. (Right click -> "Properties")

This is because the browser selializes local date and parsing it as GMT.

Comment 5

[Ian McKellar]

(In reply to comment #3)
> This should fix bug 274567, too.

Ahh yes - looks like it will. It'll also fix behaviour on non-windows platforms in non American/European/Japanese locales, since PR_ParseTimeString doesn't support their timezones: http://lxr.mozilla.org/mozilla1.8/source/nsprpub/pr/src/misc/prtime.c#899
> 
> Do we really need to explicitly specify GMT in the format string?
> 
Hmm, perhaps not - rdf_ParseDate asks PR_ParseTimeString to default to GMT. I will make another patch that omits the GMT string.

Comment 6

[Ian McKellar]

(In reply to comment #4)
> IMHO, we should not use nsIRDFDate anywhere in any product for a while.
> For example, fx's download manager is using that interface.
> So my firefox claims I've downloaded a nightly build
> the day after tomorrow. (Right click -> "Properties")
> 
> This is because the browser selializes local date and parsing it as GMT.
> 
RDF dates are important for display in XUL (I think) but there's at least this bug in their storage, perhaps others.

Comment 7

[Ian McKellar]

Attachment: This version of the patch doesn't store the string "GMT" in the date string and updates the comment to reflect the format of the string

Comment 8

[O. Atsushi (Torisugari)]

(In reply to comment #6) 
> RDF dates are important for display in XUL (I think) but there's at least this
> bug in their storage, perhaps others.

Isn't it risky to change the behavior of an exisiting interface?
It's already widely used all over the gecko-based products.
As a date recorder, you can use RDF int instead and make the node
remember Unix Time value in seconds/miliseconds, though it's ugly
in terms of object-oriented programings...

Comment 9

[Ian McKellar]

(In reply to comment #8)
> (In reply to comment #6) 
>
> Isn't it risky to change the behavior of an exisiting interface?

I don't think this changes the behaviour. This corrects platform-specific and locale-specific bugs in the current implementation. In fact it makes the code match the documentation about the function in comments. The behaviour is backwards-compatible.

> It's already widely used all over the gecko-based products.

It doesn't seem to be used too widely. This bug and change only affect RDF that's serialized to XML, and I think only the download manager (and perhaps the update system) use this inside mozilla products. We use it in Flock for the storage for our favorites (bookmarks) system. This change doesn't affect rdf date literals used by the firefox global history or bookmarks system as they are backed by mork and bookmarks.html respectively, rather than by RDFXML.

> As a date recorder, you can use RDF int instead and make the node
> remember Unix Time value in seconds/miliseconds, though it's ugly
> in terms of object-oriented programings...

Thats actually what we do in other parts of Flock and I'm sure in other parts of the Mozilla products, but for favorites we want to display the RDF in a tree and for that we need to use the date literals (to provide correct display and sorting).

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

Axel, Torisugari. Any decisions related to this bug? It would make our life easier if we could push it.

Comment 11

[Manish Singh]

Ping? I'm trying to get the ball rolling on getting some of our fixes upstream, and this is one of them.

Comment 12

[Axel Hecht [:Pike]]

Comment on attachment 223965 [details] [diff] [review]
This version of the patch doesn't store the string "GMT" in the date string and updates the comment to reflect the format of the string

[e-mail] isn't doing RDF reviews. Please rerequest review from [e-mail]

Comment 13

[Axel Hecht]

Comment on attachment 223965 [details] [diff] [review]
This version of the patch doesn't store the string "GMT" in the date string and updates the comment to reflect the format of the string

r=me, looks good.

I don't have a good tip for a sr.

Comment 14

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 223965 [details] [diff] [review]
This version of the patch doesn't store the string "GMT" in the date string and updates the comment to reflect the format of the string

sr=shaver; sorry this sat so long for such a trivial patch.  In general if you don't know who to ask for super-review, you should grab one of the "catch all" cases listed in http://www.mozilla.org/hacking/reviewers.html -- if they're not the right people, they'll bounce to someone better, but then you don't have to track such things yourself.  I'm sorry nobody gave you that advice earlier.

Comment 15

[Mike Schroepfer]

Todd you have any unit tests to cover this kind of thing?

Comment 16

[Damon Sicore (:damons)]

Comment on attachment 223965 [details] [diff] [review]
This version of the patch doesn't store the string "GMT" in the date string and updates the comment to reflect the format of the string

a1.9+=damons

Comment 17

[Zibi Braniecki [:zbraniecki][:gandalf]]

Checking in rdfutil.cpp;
/cvsroot/mozilla/rdf/base/src/rdfutil.cpp,v  <--  rdfutil.cpp
new revision: 1.62; previous revision: 1.61

Wohoo! Upstream completed ;)
Thanks ian, axel, shaver, damon and everyone involved :))