1139167 - Some birthdays are off by one day in Thunderbird's addressbook

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Christian Hoffmann]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20150224222119

Steps to reproduce:

1) Create a new contact in Thunderbird's addressbook.
2) Set birthday to 1956-08-01.
3) Select the created contact so that its contents are displayed in the bottom pane.

The system timezone seems relevant here- The problem definitely occurs with Europe/Berlin, but I expect other timezones to be affected, too.


Actual results:

The bottom pane shows 1956-07-31 (or the localized version of it)


Expected results:

The bottom pane should show 1956-08-01 (or the localized version of it)

Comment 1

[Christian Hoffmann]

The issue does not happen with all birthdays, only certain years are affected. The date in question must also be before the change to DST in the year for the issue to occur.
I'm not exactly sure what other preconditions have to be met, but I suspect it's due to DST inconsistencies (i.e. some years did not have a DST, some years do).
These lead to the final date being (DATE-1) 23:00:00 or something. So the issue boils down to time accuracy, although only calculating with dates.

Also note that the contact editing view handles this correctly.

The relevant code is in suite/mailnews/addrbook/abCardViewOverlay.js ~254.
The problem can be reproduced in Firefox's JavaScript console by constructing cases with new Date() and .toLocaleDateString(). It's not a JavaScript implementation-specific issue, Chrome behaves the same.

I am working on a patch and report back once I have got something working which should fix the issue in general and not just for me in the Europe/Berlin timezone.

Comment 2

[Magnus Melin [:mkmelin]]

Bug 1122571?

Comment 3

[Thomas D. (:thomas8)]

Christian, it would be really great if you could fix this.

Can you fix the .toLocaleDateString() function itself?

Comment 4

[Christian Hoffmann]

(In reply to Thomas D. from comment #3)
> Can you fix the .toLocaleDateString() function itself?
As posted in my comment to bug #1122571 (thanks for pointing it out; it didn't turn up in my search), I think .toLocaleDateString() behaves according to the spec, so in my opinion nothing can be fixed there.

We are working with dates and not datetimes here, but JavaScript only provides a time-aware datatype for us. Therefore, I think the best approximation to "date only" would be the usage of UTC-only to cancel out any timezone or DST difficulties.
This is what I am trying to work on.

For this to work, both the timestamp and the result have to be computed in UTC.

The following snippet seems to do the trick:

new Date(Date.UTC(1959,4,2)).toLocaleDateString(undefined, {timeZone: "UTC"})

In fact, this does work for all dates I've tested. However, I am going to test this a bit more systematically before turning this into a patch.
The only ugly bit about this snippet is the "undefined" for the list of requested locales. I am passing this in order to retain the current behaviour. Passing an empty list also works, passing null or an empty string doesn't.

I will report back with either a working patch or any problems discovered during testing.

Comment 5

[Christian Hoffmann]

As promised, I have written a test script in bash/nodejs which iterates over all timezones and all days in the years between 1900 and 2099 and compares the input date with the output of the current Thunderbird logic and my proposed changed Thunderbird logic.

The results confirm that the output date matches the input date with the proposed logic, while the current logic fails this test.
Please note that the test uses .toLocaleString() and not .toLocaleDateString() which Thunderbird uses. The reason for this change is that this allows to catch miscalculations even if the date is rendered correctly, but the hour isn't. I do realize that nodejs is v8 and not Gecko, but as explained above I expect this to be implementation-independent.

$ bash mozbug-1139167-test.sh | grep -P 'TZ|new=false'
# only returns a debug line for each of the 417 timezones listed on my system

Attached are patches against trunk and Thunderbird 31 (I'm running arch and it's the stable version there). A quick check in Thunderbird 31 on my system confirms that the patch is working as expected.

It would be great if someone could give feedback on this patch and/or could integrate it into the official sources.

Comment 6

[Christian Hoffmann]

Attachment: Patch against comm-central trunk

Comment 7

[Christian Hoffmann]

Attachment: Patch against thunderbird-31.5.0 sources

Comment 8

[Christian Hoffmann]

Attachment: Test script to validate the proposed logic change (requires nodejs)

Comment 9

[Ian Neal]

You just need to pick a reviewer now. https://wiki.mozilla.org/Modules/MailNews_Core suggests the likes of standard8 and Neil Rashbrook ([e-mail]) but there are others.

Comment 10

[Christian Hoffmann]

Attachment: Patch against comm-central trunk

Comment 11

[Christian Hoffmann]

Attachment: Patch against thunderbird-31.5.0 sources

Comment 12

[Christian Hoffmann]

Comment on attachment 8575128 [details] [diff] [review]
Patch against comm-central trunk

Updated both patches to avoid dropping the fix for bug #522642.

Mark, I'm choosing you as you are listed for both addressbook and i18n. Could you review/commit the patch?
Thanks in advance.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 8575128 [details] [diff] [review]
Patch against comm-central trunk

This doesn't need specific i18n review, so passing it across to Mike.

Comment 14

[:aceman]

Does this also cover the problem that if you enter a birthday of 29/2/2000 (29th Feb), it shows correctly but as soon as you reopen editing the card, it gets modified to 1/3/2000 (3rd Mar).
Or is that actually a problem?

Comment 16

[Christian Hoffmann]

(In reply to :aceman from comment #14)
> Does this also cover the problem that if you enter a birthday of 29/2/2000
> (29th Feb), it shows correctly but as soon as you reopen editing the card,
> it gets modified to 1/3/2000 (3rd Mar).
> Or is that actually a problem?
No, sadly not, but it was not the intention of the patch to fix it, I was not even aware of this problem. I'll be happy to try my luck on this one as well, but as this is a different issue in a different dialogue I suggest that this is handled in a seperate bug report.

Btw, if someone has any input on why we see so many duplicate bug reports for this issue (#1139167) just now (2015), that would be great. Maybe just a coincidence, but I found it surprising.

Comment 18

[hsv]

Why (in Thunderbird) use such a mechanism for a birthday, a simple past event? Especially since Tb allows entering only the year, or only the day of year, it would be better to record only a threesome, not a 'time_t' or broader equivalent.
Otherwise users would be required to enter the longitude or zone of the place where the celebree was born, and the hour of birth.
Such things survive calendar-change: George Washington was born on February 11, but, because during his lifetime England & her colonies switched to the Gregorian calendar, its anniversary is celebrated on February 22. In the event of a further calendar reform, the notion the eleven month s eleventh day s eleventh hour, in reference to the western-front armistice of WWI, will not be translated, although its anniversary surely will be.

Comment 19

[Christian Hoffmann]

(In reply to hsv from comment #18)
> Why (in Thunderbird) use such a mechanism for a birthday, a simple past
> event? Especially since Tb allows entering only the year, or only the day of
> year, it would be better to record only a threesome, not a 'time_t' or
> broader equivalent.
While I agree with this, I also see technical reasons for the way this was implemented: The stored date is supposed to be formattable in the user's locale and all the formatting logic works on the single, given data type -- a JavaScript Date object which happens to also keep time information.
I also assume that storage in 3rd party formats such as VCards needs to happen in a specified format.

While invention of an appropriate Date-only data type along with the necessary formatters is certainly not impossible, it may be connected with a significant amount of work which is IMO out of the scope of a simple bug fix.

Comment 20

[Kent James (:rkent)]

Mike, are you going to be able to look at this soon?

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8575128 [details] [diff] [review]
Patch against comm-central trunk

This looks OK to me by inspection, but somebody needs to review the suite bits.

Comment 22

[Christian Hoffmann]

Comment on attachment 8575128 [details] [diff] [review]
Patch against comm-central trunk

Alright. Unfortunately I'm unsure how to request review feedback from a second reviewer in the bug's flags.

Mark, you are listed as primary SeaMonkey Address book owner; I'm re-cc'ing you as you probably already invested some time by initially reading this bug. Would be great if you could have a second look at it regarding SeaMonkey.

I would also be fine with dropping the suite bits from the patch for now, but I thought that it makes sure to keep the code bases aligned.

Comment 23

[:aceman]

Comment on attachment 8575128 [details] [diff] [review]
Patch against comm-central trunk

I think listing standard8 as the owner of Seamonkey AB is a longstanding bug. I think IanN will be a more active reviewer for this.

You can request review from another user via the additional review field that crops up at the bottom of the flags list after you already have a reviewer on a patch. It is marked as "addl.".
When there are no reviewers yet, you can put multiple people into the first review field that is in the flags list.

Comment 24

[:aceman]

Comment on attachment 8575129 [details] [diff] [review]
Patch against thunderbird-31.5.0 sources

Review of attachment 8575129 [details] [diff] [review]:
-----------------------------------------------------------------

I am not sure we can take something like this for TB31. But there may be a chance of it getting into TB38.x. Maybe the trunk version applies there too.

Comment 25

[Christian Hoffmann]

aceman, thanks for the bug wrangling / advice.

As far as I understand, this is now awaiting SeaMonkey review and decisions about the target branches. Please let me know if there is something I am supposed to do here at this stage.

Comment 26

[Ian Neal]

Comment on attachment 8575128 [details] [diff] [review]
Patch against comm-central trunk

r=me

just a small note, it is best to provide patches with a context of 8 and set showfunc to true, so in your .hgrc have something like:
[defaults]
diff = -U 8 -p
qdiff = -U 8 -p

[diff]
git = true
showfunc = true
unified = 8

Comment 27

[Philip Chee]

Comment on attachment 8575128 [details] [diff] [review]
Patch against comm-central trunk

a=me for SeaMonkey CLOSED TREE

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Thank you for debugging the issue and writing the patch. It's been uploaded as https://hg.mozilla.org/comm-central/rev/17a6a5c2d295

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Comment on attachment 8575128 [details] [diff] [review]
Patch against comm-central trunk

[Approval Request Comment]
Regression caused by (bug #): ?
User impact if declined: Addressbook can show a birthday off by one day
Testing completed (on c-c, etc.): Not yet
Risk to taking this patch (and alternatives if risky): ?

Comment 30

[Kent James (:rkent)]

Comment on attachment 8575128 [details] [diff] [review]
Patch against comm-central trunk

https://hg.mozilla.org/releases/comm-aurora/rev/aed081047c5e
https://hg.mozilla.org/releases/comm-beta/rev/3ca0490d9fc9