769938 - calDateTime returns invalid nativeTime property with dates before 1970

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Matthew Mecca [:mmecca]]

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]]

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]]

Attachment: 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 [:ssitter]]

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 [:ssitter]]

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] [:📆][:🧩]]

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 (he/him; ⌚ UTC+1/+2)]

(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] [:📆][:🧩]]

(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 (he/him; ⌚ UTC+1/+2)]

Found the cause; we used to treat 'unsigned long long' as PRInt64. See bug 711404.

Comment 9

[Stefan Sitter [:ssitter]]

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] [:📆][:🧩]]

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 [:ssitter]]

Does existing events < 1970 that were created with the faulty Thunderbird/Lightning version work with the patch applied?

Comment 12

[Matthew Mecca [:mmecca]]

(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 [:ssitter]]

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 [:ssitter]]

Attachment: xpcshell test

Comment 15

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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] [:📆][:🧩]]

Pushed to comm-central changeset af45c977d517

Comment 17

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Backported to releases/comm-aurora changeset 33ab94f81275

Comment 18

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Backported to releases/comm-beta changeset 0fd3cafc7520

Comment 19

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pushed to comm-central changeset 105f391d603d

Comment 20

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Backported to releases/comm-aurora changeset d310591a4249

Comment 21

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Backported to releases/comm-beta changeset 54af1cafd26a

Comment 22

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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]]

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

(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