Last Comment Bug 771401 - Change PRTime to signed long long
: Change PRTime to signed long long
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: 12 Branch
: All All
: -- normal (vote)
: mozilla17
Assigned To: Makoto Kato [:m_kato]
:
: Nathan Froyd [:froydnj]
Mentors:
Depends on: 711404
Blocks: 769938
  Show dependency treegraph
 
Reported: 2012-07-05 18:28 PDT by Philipp Kewisch [:Fallen]
Modified: 2014-03-23 11:38 PDT (History)
7 users (show)
m_kato: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
fix (8.83 KB, patch)
2012-07-05 21:52 PDT, Makoto Kato [:m_kato]
bobbyholley: review+
benjamin: superreview+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
Details | Diff | Splinter Review

Description Philipp Kewisch [:Fallen] 2012-07-05 18:28:22 PDT
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 2 Makoto Kato [:m_kato] 2012-07-05 21:52:19 PDT
Created attachment 639582 [details] [diff] [review]
fix
Comment 3 Makoto Kato [:m_kato] 2012-07-05 21:57:14 PDT
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) (busy with Stylo) 2012-07-06 00:32:12 PDT
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 [:bsmedberg] 2012-07-10 07:13:06 PDT
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 2012-07-18 10:05:16 PDT
(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] 2012-07-19 01:28:26 PDT
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 9 Ed Morley [:emorley] 2012-07-20 06:47:49 PDT
https://hg.mozilla.org/mozilla-central/rev/e09cf80010c4
Comment 10 Alex Keybl [:akeybl] 2012-07-23 11:55:41 PDT
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] 2012-07-23 13:08:05 PDT
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] 2012-07-23 16:18:00 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/5c9ca801c4c7

Note You need to log in before you can comment on or make changes to this bug.