90631 - Globally remove PR_ParseTimeString()

Status: RESOLVED WONTFIX

(Core Graveyard :: Tracking, defect)

Description

[NilsE]

This affects all platforms. This is a kind of meta bug.

Summary:

The NSPR function PR_ParseTimeString() converts a time string into
"ticks since epoch". It's broken in the sense that only a subset of all
timezones are supported and the function silently returns a bogus value
if a timezone is not recognized. According to larryh, this function is
being considered "deprecated" and will not be fixed. See bug 74847 for a
detailed discussion. Therefore, PR_ParseTimeString() must be removed
from Seamonkey.


Description:

The silent failure gives rise to class of subtle problems that won't be
observed by Netscape engineers as all US timezones are supported. An
example of such a bug is bug 73029. As of 07/13/2001, this function is
being used in 18 files within Seamonkey. See

http://lxr.mozilla.org/seamonkey/ident?i=PR_ParseTimeString

It is quite likely that this is the source of a number of bugs.
This bug should be marked "fixed" after all occurrences have been
removed/replaced.

How to fix?

As PR_ParseTimeString() will not be fixed and the reason for its
deprecation is unknown, the function can only be avoided. There appears
to be no "NSPR official" replacement/workaround. There are two
possibilities of how to proceed:

1. Find some external function/library (compatible with MPL) that does
   the trick. glibc provides strptime(), which could be used to write a
   replacement. Python and other scripting languages have something
   similar. But it's probably hard to find a portable third party
   function that works on all Mozilla platforms and there's the problem
   of adding another software dependence. 

2. Avoid time strings altogether. For internal use, PRTime (ticks since
   epoch) should be sufficient. For output, a PRTime to string
   conversion by means of PR_ExplodeTime should be by far easier than the
   reverse direction (as performed by PR_Parse_TimeString()). The only
   part I couldn't find is a PRExplodedTime_to_String conversion
   function which should be trivial to write.

Personally I would suggest option 2.


Affected modules include: 

extensions/cookie, extensions/p3p, intl/locale, modules/libjar,
modules/plugin, netwerk, xpcom, xpfe, mailnews.
(This is why I filed the bug as Mozilla/General; don't know better. 
Sorry, Asa ;-) )

(Right now, I don't know of any bugs where PR_Parse_TimeString() is the
culprit that have a severity higher than "normal". But that doesn't mean
they don't exist ;-) Anyway, that's why this bug is set to "normal".)

Comment 1

[timeless]

tracking is better than asa...

testParseTimeString() is interesting ...

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Added some other folks who are likely to have useful input on the direction to
take.  Another conceivable option is that since NSPR is abandoning
PR_ParseTimeString, perhaps it could be moved to an i18n-related module and
maintained there (similar to what happened with PLEvents).

Comment 3

[NilsE]

Attachment: debug msgs for mozilla/nsprpub/pr/src/misc/prtime.c

Comment 4

[NilsE]

If you follow Dan's suggestion of re-using PR_ParseTimeString within
i18n, the question is if and how the function could be corrected.

Two issues:
1) deal with all/most time zones
2) indicate error(assertion?) if unknown timezone is encountered

Using the above patch, I've done some tests:

- Browsing: 
  Web servers seem to send only GMT. At least during browsing I always
  encountered GMT times as input, such as:
PR_ParseTimeString called with: Thu, 19 Jul 2001 14:57:45 GMT. Recognized time:
14:57 GMT


- Mail/News:
  Here we obviously get wrong results. Funny thing is, the times were
  recognized as GMT and not converted to GMT because the parsed timezone 
  was unknown:
PR_ParseTimeString called with: Tue, 17 Jul 2001 07:19:18 +0200. Recognized
time: 7:19 GMT
PR_ParseTimeString called with: Thu, 19 Jul 2001 14:47:44 +0200 (CEST).
Recognized time: 14:47 GMT

I didn't observe other callees so far.

So, in order to re-use the function, someone has to have a closer look
at it, fix it and do some tests to ensure correctness (the thing is >600
lines).

Dan: did you notice that the section "OLD COMPATIBILITY FUNCTIONS" in
prtime.c only starts right *after* PR_ParseTimeString? ;-)

Comment 5

[nhottanscp]

I just want to clarify this is not supposed to parse localized strings. For
example, "Tue", "Jul" may be localized depends on the user's locale. Parsing
localized date/time string is not easy.

Comment 6

[Cormac F]

Adding meta keyword - Sorry for the spam!

Comment 7

[chris hofmann]

wonder if a tree sweeping hacker like dougt should own this hairball.

Comment 8

[Doug Turner (:dougt)]

this isn't that big of a change.  I can work on it, but it is going to be after
the peformance and API work I am doing.  

Comment 9

[Judson Valeski]

-> dougt

Comment 10

[Doug Turner (:dougt)]

This is not a high priority.

Comment 11

[Doug Turner (:dougt)]

NSPR is taking fixes.  

A couple of things that you should understand:

1. NSPR won't be able to understand all the time zone abbreviations.
In particular, some time zone abbreviations are fundamentally
ambiguous.  For example, CST is both (US) Central Standard
Time and China Standard Time.

2. NSPR won't be able to parse localized time strings.

3. NSPR can only review submitted patches and the team won't be able to
implement the requested enhancements.

Based on this, please open up a new bug against NSPR.  If you are unable to
create a patch for NSPR, please assign the bug to me.  When I have the time, I
will try to fix it.

Comment 12

[Wan-Teh Chang]

Nils,

The three things that Doug said in comment #11 are
actually my words.  I will try to support the
PR_ParseTimeString function under those three
conditions.  If the conditions are acceptable,
please reopen the NSPR bugs you filed before.

Thanks.

Comment 13

[NilsE]

Nice to see this revisited. This is my take:

1. While doing QA, I found this NSPR function that is broken, supposed to be
deprecated, and according to larryh will not be fixed. His reasoning is similar
to dougt's point 1) in comment #11. I agree that ambiguous time zone naming
conventions render this function useless beyond repair. So far, I haven't talked
about localization which would make matters even worse.

2. Therefore, I agree with the decision not to maintain PR_ParseTimeString() as
is would be impossible to fix it anyway. The only way to prevent bugs based on
this is to remove the function. In bug 73029 darin found a way to avoid this
function.

3. I don't understand dougt's point 3) as I'm not requesting an enhancement. I
think it's it's better to avoid PR_ParseTimeString() than trying unsuccessfully
to enhance/fix it.

I'm not in the position to provide patch that's removing it globally because I
just haven't got the expertise in all these different areas of Mozilla where the
function is being used. I'm also not providing a patch to fix
PR_ParseTimeString() (or reopening bug 74847 for that matter) because I now
think it can't be done reliably.