Note: There are a few cases of duplicates in user autocompletion which are being worked on.

calDateTime returns invalid nativeTime property with dates before 1970

RESOLVED FIXED in 1.6

Status

Calendar
Internal Components
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mmecca, Assigned: mmecca)

Tracking

Lightning 1.4
Dependency tree / graph
Bug Flags:
in-testsuite ?

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Assignee: nobody → matthew.mecca
Blocks: 751821
(Assignee)

Comment 1

5 years ago
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.
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
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.
Attachment #638131 - Flags: review?(philipp)

Comment 3

5 years ago
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

5 years ago
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.
Ms2ger might be able to help us out: see comment 4. Also would you have any concerns with the patch proposed here?
(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.
(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?
Flags: in-testsuite?
Found the cause; we used to treat 'unsigned long long' as PRInt64. See bug 711404.
Depends on: 711404

Comment 9

5 years ago
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?
Depends on: 771401
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.
Attachment #638131 - Flags: review?(philipp)
Attachment #638131 - Flags: review+
Attachment #638131 - Flags: approval-calendar-beta+
Attachment #638131 - Flags: approval-calendar-aurora+

Comment 11

5 years ago
Does existing events < 1970 that were created with the faulty Thunderbird/Lightning version work with the patch applied?
(Assignee)

Comment 12

5 years ago
(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

5 years ago
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

5 years ago
Created attachment 640510 [details] [diff] [review]
xpcshell test
Attachment #640510 - Flags: review?(philipp)
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!
Pushed to comm-central changeset af45c977d517
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.8
Backported to releases/comm-aurora changeset 33ab94f81275
Target Milestone: 1.8 → 1.7
Backported to releases/comm-beta changeset 0fd3cafc7520
Target Milestone: 1.7 → 1.6
Pushed to comm-central changeset 105f391d603d
Target Milestone: 1.6 → 1.8
Backported to releases/comm-aurora changeset d310591a4249
Target Milestone: 1.8 → 1.7
Backported to releases/comm-beta changeset 54af1cafd26a
Target Milestone: 1.7 → 1.6
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 :)
Attachment #640510 - Flags: review?(philipp) → review+
(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?
Target Milestone: 1.6 → ---
(Assignee)

Comment 24

5 years ago
(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
Target Milestone: --- → 1.6
You need to log in before you can comment on or make changes to this bug.