Last Comment Bug 769938 - calDateTime returns invalid nativeTime property with dates before 1970
: calDateTime returns invalid nativeTime property with dates before 1970
Status: RESOLVED FIXED
:
Product: Calendar
Classification: Client Software
Component: Internal Components (show other bugs)
: Lightning 1.4
: All All
: -- normal (vote)
: 1.6
Assigned To: Matthew Mecca [:mmecca]
:
Mentors:
Depends on: 711404 771401
Blocks: 751821
  Show dependency treegraph
 
Reported: 2012-06-30 11:31 PDT by Matthew Mecca [:mmecca]
Modified: 2012-09-14 14:02 PDT (History)
2 users (show)
philipp: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Fix v1 (1.89 KB, patch)
2012-06-30 12:02 PDT, Matthew Mecca [:mmecca]
philipp: review+
philipp: approval‑calendar‑aurora+
philipp: approval‑calendar‑beta+
Details | Diff | Review
xpcshell test (2.05 KB, patch)
2012-07-09 22:36 PDT, Stefan Sitter
philipp: review+
Details | Diff | Review

Description Matthew Mecca [:mmecca] 2012-06-30 11:31:47 PDT
Dates earlier than 1/1/1970 should return the nativeTime property as a negative 64 bit value, but instead the value is being improperly cast to an unsigned value. This causes invalid date properties to be saved in providers like the Storage provider that use nativeTime to store date values, as in Bug 751821.
Comment 1 Matthew Mecca [:mmecca] 2012-06-30 11:55:34 PDT
I'm no expert here but this seems to me to be happening at the XPCOM boundary:

mNativeTime is defined as a PRTime in calDateTime.h
http://mxr.mozilla.org/comm-central/source/calendar/base/src/calDateTime.h#38

and PRTime is defined as a signed PRInt64 in NSPR
http://mxr.mozilla.org/comm-central/source/mozilla/nsprpub/pr/include/prtime.h#48

but PRTime is defined as an unsigned long long in nsrootidl
http://mxr.mozilla.org/comm-central/source/mozilla/xpcom/base/nsrootidl.idl#33


From what I can tell nothing about this changed in the Lightning 1.4 time frame, so I'm not sure why it causes problems now but didn't before.
Comment 2 Matthew Mecca [:mmecca] 2012-06-30 12:02:22 PDT
Created attachment 638131 [details] [diff] [review]
Fix v1

Changing nativeTime to PRInt64 in calIDateTime.idl seems to fix this, but I'm not sure if this is the best approach or not.
Comment 3 Stefan Sitter 2012-06-30 15:28:48 PDT
The changelog of the mentioned files show that Bug 718488 changed how calIDateTime stores/passes the date. Maybe this change regressed the error?
Comment 4 Stefan Sitter 2012-07-04 15:23:53 PDT
I can confirm the regression range. Executing the following command on shell will produce different result:

> var cd = Components.classes["@mozilla.org/calendar/datetime;1"].createInstance(Components.interfaces.calIDateTime);cd.year=1960;cd.nativeTime;

Thunderbird 11 + Lightning 1.3: nativeTime = -315619200000000
Thunderbird 12 + Lightning 1.4: nativeTime = 18446428454509552000

Does anybody know who to ask for xpcom topics? For example it would be useful to know if PRTime should be signed long long in nsrootidl.idl to match the other definitions or not.
Comment 5 Philipp Kewisch [:Fallen] 2012-07-04 16:55:09 PDT
Ms2ger might be able to help us out: see comment 4. Also would you have any concerns with the patch proposed here?
Comment 6 :Ms2ger 2012-07-05 01:47:49 PDT
(In reply to Stefan Sitter from comment #4)
> I can confirm the regression range. Executing the following command on shell
> will produce different result:
> 
> > var cd = Components.classes["@mozilla.org/calendar/datetime;1"].createInstance(Components.interfaces.calIDateTime);cd.year=1960;cd.nativeTime;
> 
> Thunderbird 11 + Lightning 1.3: nativeTime = -315619200000000
> Thunderbird 12 + Lightning 1.4: nativeTime = 18446428454509552000

Strange; I shouldn't have changed any of the code that's hit in that snippet...

> Does anybody know who to ask for xpcom topics? For example it would be
> useful to know if PRTime should be signed long long in nsrootidl.idl to
> match the other definitions or not.

I think so.
Comment 7 Philipp Kewisch [:Fallen] 2012-07-05 02:15:03 PDT
(In reply to :Ms2ger from comment #6)
> (In reply to Stefan Sitter from comment #4)
> > I can confirm the regression range. Executing the following command on shell
> > will produce different result:
> > 
> > > var cd = Components.classes["@mozilla.org/calendar/datetime;1"].createInstance(Components.interfaces.calIDateTime);cd.year=1960;cd.nativeTime;
> > 
> > Thunderbird 11 + Lightning 1.3: nativeTime = -315619200000000
> > Thunderbird 12 + Lightning 1.4: nativeTime = 18446428454509552000
> 
> Strange; I shouldn't have changed any of the code that's hit in that
> snippet...
I'm not specifically saying that you caused this, sorry if that impression came up. I was rather suggesting since you've changed our xpcom stuff before in bug 713633 and that was a very smart patch, you might know enough about xpcom to give us a hint why this happened.

> 
> > Does anybody know who to ask for xpcom topics? For example it would be
> > useful to know if PRTime should be signed long long in nsrootidl.idl to
> > match the other definitions or not.
> 
> I think so.
So would you suggest we just file a bug, or do you know someone else that might know more about this topic?
Comment 8 :Ms2ger 2012-07-05 02:25:27 PDT
Found the cause; we used to treat 'unsigned long long' as PRInt64. See bug 711404.
Comment 9 Stefan Sitter 2012-07-05 09:10:24 PDT
Ok, so PRTime is incorrectly treat as PRUInt64 in xpconnect but should be PRInt64. It just used to work all the time because PRUInt64 was incorrectly treat as PRInt64. Fixing the later issue caused the previous one. Should we file a new bug for xpconnect and use this bug for a temporary workaround in Lightning if required?
Comment 10 Philipp Kewisch [:Fallen] 2012-07-05 18:34:25 PDT
Comment on attachment 638131 [details] [diff] [review]
Fix v1

From what I understand, this will be a safe workaround. I think we should test this for a few central/aurora builds and then push it to beta.

I've filed bug 771401 to fix this upstream, I really would prefer that. Maybe we can backout the change when the dependent bug has been fixed.
Comment 11 Stefan Sitter 2012-07-07 06:32:30 PDT
Does existing events < 1970 that were created with the faulty Thunderbird/Lightning version work with the patch applied?
Comment 12 Matthew Mecca [:mmecca] 2012-07-07 07:12:14 PDT
(In reply to Stefan Sitter from comment #11)
> Does existing events < 1970 that were created with the faulty
> Thunderbird/Lightning version work with the patch applied?

No, those will still have invalid date values. I left Bug 751821 open to track that part of the issue for the Storage calendar.
Comment 13 Stefan Sitter 2012-07-09 21:55:18 PDT
Do you need help with check-in? According to comment 10 this should get testing on central/aurora before it lands on beta. Lightning 1.6 release is due in less than 7 days.
Comment 14 Stefan Sitter 2012-07-09 22:36:30 PDT
Created attachment 640510 [details] [diff] [review]
xpcshell test
Comment 15 Philipp Kewisch [:Fallen] 2012-07-10 03:21:37 PDT
I'll take care of checking this in. Given the release is due and this is pretty major, lets take it as is down to beta. Thanks for the test!
Comment 16 Philipp Kewisch [:Fallen] 2012-07-10 05:37:56 PDT
Pushed to comm-central changeset af45c977d517
Comment 17 Philipp Kewisch [:Fallen] 2012-07-10 05:38:34 PDT
Backported to releases/comm-aurora changeset 33ab94f81275
Comment 18 Philipp Kewisch [:Fallen] 2012-07-10 05:39:08 PDT
Backported to releases/comm-beta changeset 0fd3cafc7520
Comment 19 Philipp Kewisch [:Fallen] 2012-07-10 05:43:22 PDT
Pushed to comm-central changeset 105f391d603d
Comment 20 Philipp Kewisch [:Fallen] 2012-07-10 05:43:50 PDT
Backported to releases/comm-aurora changeset d310591a4249
Comment 21 Philipp Kewisch [:Fallen] 2012-07-10 05:44:23 PDT
Backported to releases/comm-beta changeset 54af1cafd26a
Comment 22 Philipp Kewisch [:Fallen] 2012-07-10 05:45:34 PDT
Comment on attachment 640510 [details] [diff] [review]
xpcshell test

In case you were wondering, the second set of pushes is the unit test I forgot :)
Comment 23 Martin Schröder [:mschroeder] 2012-07-27 10:02:15 PDT
(In reply to Philipp Kewisch [:Fallen] from comment #10)
> Comment on attachment 638131 [details] [diff] [review]
[...]
> I've filed bug 771401 to fix this upstream, I really would prefer that.
> Maybe we can backout the change when the dependent bug has been fixed.

Bug 771401 seems to be fixed on mozilla-central. Is there already a bug report for backing out the workaround?
Comment 24 Matthew Mecca [:mmecca] 2012-08-19 22:06:09 PDT
(In reply to Martin Schröder [:mschroeder] from comment #23)

> Bug 771401 seems to be fixed on mozilla-central. Is there already a bug
> report for backing out the workaround?

Filed Bug 783945

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