Change PRTime to signed long long

RESOLVED FIXED in Firefox 16

Status

()

Core
XPCOM
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Fallen, Assigned: m_kato)

Tracking

({regression})

12 Branch
mozilla17
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox16 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
Calendar has experienced regression bug 769938 due to the patch for bug 711404, which fixes how xpconnect handles PRUint64. While the patch for bug 711404 is technically correct, we need to find a solution for PRTime:

The problem that Calendar is experiencing, is that dates before 1970, which should have a negative native time, now underflow and mess up the date.

I propose to change PRTime to long long, since dates before 1970 are currently much more common than those after 2038.
(Assignee)

Comment 1

5 years ago
PRTime in NSPR is defined as PRInt64. (https://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/prtime.h#48).  But it is defined as unsigned long long in nosroot.idl (https://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsrootidl.idl#33).  It is confusion.

On CVS log, no reason exists... http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/xpcom/base&command=DIFF_FRAMESET&file=nsrootidl.idl&rev2=1.6&rev1=1.5.
(Assignee)

Comment 2

5 years ago
Created attachment 639582 [details] [diff] [review]
fix
(Assignee)

Comment 3

5 years ago
Comment on attachment 639582 [details] [diff] [review]
fix

By bug 711404, we can handle PRUint64 on xpconnect correctly.  But Although PRTime in NSPR is PRInt64, PRTime in nsroot.idl is defined as unsigned long long.

So, we should change PRTime as long long.
Attachment #639582 - Flags: superreview?(benjamin)
Attachment #639582 - Flags: review?(bobbyholley+bmo)
Comment on attachment 639582 [details] [diff] [review]
fix

Please add a/some comment(s) explaining why we're testing timeProperty as -1. From a first glance, it's confusing.
Attachment #639582 - Flags: review?(bobbyholley+bmo) → review+

Updated

5 years ago
Assignee: nobody → m_kato
Comment on attachment 639582 [details] [diff] [review]
fix

This is correct because PRTime is actually this in prtime.h, right?

typedef PRInt64 PRTime;

And so we're just making the IDL typedef match the correct C++ typedef?
Attachment #639582 - Flags: superreview?(benjamin) → superreview+

Comment 6

5 years ago
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)

You are correct. PRTime should be PRInt64 but is defined as PRUint64 in xpconnect by mistake. 

This was not observed as a problem in mozilla11 and older platform releases because PRUint64 was processed as PRInt64 in xpconnect by mistake and therefore PRTime worked as expected.

The later mistake was fixed with Bug 711404 but unfortunately this fix regressed and broke PRTime in mozilla12 and newer platform releases.
Keywords: regression
Version: unspecified → 12 Branch
(Reporter)

Comment 7

5 years ago
Comment on attachment 639582 [details] [diff] [review]
fix

I'd appreciate if this patch could also be pushed to aurora and beta.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 711404
User impact if declined: Consumers of PRTime with dates before 1970 could be mistakingly placed far in the future, which might cause wrong dates to be placed in databases, etc.
Risk to taking this patch (and alternatives if risky): Low risk, as PRTime has been (mistakingly) correct before Mozilla 12 all along.
String or UUID changes made by this patch: nsIXPCTestObjectReadOnly, nsIXPCTestObjectReadWrite
Attachment #639582 - Flags: approval-mozilla-beta?
Attachment #639582 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e09cf80010c4
Target Milestone: --- → mozilla17
(Assignee)

Updated

5 years ago
Flags: in-testsuite+

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/e09cf80010c4
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Comment on attachment 639582 [details] [diff] [review]
fix

[Triage Comment]
Given the fact that bug 711404 has been in the product since 12 (without obvious impact), I don't think we need to rush this past Aurora.
Attachment #639582 - Flags: approval-mozilla-beta?
Attachment #639582 - Flags: approval-mozilla-beta-
Attachment #639582 - Flags: approval-mozilla-aurora?
Attachment #639582 - Flags: approval-mozilla-aurora+
(Reporter)

Comment 11

5 years ago
I'd appreciate if someone other than me could take care of the transplant, as I don't have the mozilla-aurora repository set up.
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-aurora/rev/5c9ca801c4c7
status-firefox16: --- → fixed
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.