771401 - Change PRTime to signed long long

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Philipp Kewisch [:Fallen][📱📆]]

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.

Comment 1

[Makoto Kato [:m_kato]]

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.

Comment 2

[Makoto Kato [:m_kato]]

Attachment: fix

Comment 3

[Makoto Kato [:m_kato]]

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.

Comment 4

[Bobby Holley (:bholley)]

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.

Comment 5

[Benjamin Smedberg]

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?

Comment 6

[Stefan Sitter [:ssitter]]

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

Comment 7

[Philipp Kewisch [:Fallen][📱📆]]

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

Comment 8

[Makoto Kato [:m_kato]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e09cf80010c4

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e09cf80010c4

Comment 10

[Alex Keybl [:akeybl]]

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.

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5c9ca801c4c7