1300493 - alerts for floating events occur in UTC time

Status: RESOLVED FIXED

(Calendar :: Alarms, defect)

Description

[maaaaaaaaaaaaaartin]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160728204513

Steps to reproduce:

Create an event in current timezone with alarm: the event displays correctly, the alarms shows up at the right time.

Now switch the event to floating ("Local time"), the event stills displays correctly but the alarm shows up two hours late (I'm in Paris, so UTC+2).

Comment 1

[[:MakeMyDay]]

Which timezone do you have configured in preferences > calendar? Are there any messages in the error console when reproducing the issue?

Comment 2

[maaaaaaaaaaaaaartin]

The configured timezone is Europe/Paris.

Apparently I'm getting this error:
> Warning: Use of Mutation Events is deprecated. Use MutationObserver instead.
> Source File: chrome://calendar/content/calendar-daypicker.xml
> Line: 46
Does that help?

Comment 3

[[:MakeMyDay]]

Unfortunately, it doesn't. However, there seems to be a problem with alarms for event with floating time in general, not just when changing from a defined timezone in an existing event.

If you setup a new event (to get it fired immediately, I set it to fire 0 minutes before the event and and event time in the past) with local time and with default timezone applied, the alarm for latter fires immediately, while the former doesn't (tested also with 2 hours offset to UTC with Lighnting 4.7 and 5.3).

Comment 4

[Decathlon]

Attachment: patch

(In reply to [:MakeMyDay] from comment #3)

> If you setup a new event (to get it fired immediately, I set it to fire 0
> minutes before the event and and event time in the past) with local time and
> with default timezone applied, the alarm for latter fires immediately, while
> the former doesn't (tested also with 2 hours offset to UTC with Lighnting
> 4.7 and 5.3).

I can't reproduce in general this behavior unless the start time is set in the past but not too late (less of the UTC offset compared to the local time).

MakeMyDay, could you try if this patch fixes the issue for you?

Comment 5

[[:MakeMyDay]]

Comment on attachment 8790450 [details] [diff] [review]
patch

Thanks for coming up with a patch that quickly. With this applied, it works for me as expected, either for past or future dated events in floating time. Also the originally reported case works for me with this patch.

Comment 6

[maaaaaaaaaaaaaartin]

Awesome! Can't believe it was solved so fast…thanks guys! When do I get a chance to see the patch in mainstream TB (using KUbuntu 14.04)?

Comment 7

[[:MakeMyDay]]

Decathlon, do you still plan to submit a complete patch or should I just add the headers, convert my f+ to r+ and land it?

Comment 8

[Decathlon]

I think it needs a test for this bug. I haven't taken a look yet, mainly because I still didn't find a simply way to try xpcshell tests without building or without using the try-server thousands times (in the past I had problems with timezones in the tests) but I'm going to do it soon.

Comment 9

[Decathlon]

Attachment: patch-v1 with test

Added a test and messages about many tests in the file test_alarmservice.js because without them is very hard to understand what is the test that fails in that file (also useful for bug 1261688).

I pushed this patch on the try-server with the addition of an intentional mistake in the last test of the file in order to have a log with all the messages caused by the tests:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a3dd14f8978869b08aaee150e4ad986693e1a2bd

This bug blocks bug 1261688 (or vice versa) because both the patches modify the same file. I will update one of them right after the other one is pushed.

Comment 10

[[:MakeMyDay]]

Comment on attachment 8802126 [details] [diff] [review]
patch-v1 with test

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

Thanks, r+ with the comments below considered/checked.

::: calendar/test/unit/test_alarmservice.js
@@ +84,5 @@
>                  for (let icalString in this.expectedMap[calId][id]) {
> +                    let expectedAndTitle = this.expectedMap[calId][id][icalString];
> +                    // if no explicit message has been passed, take the item title
> +                    let message = (typeof aMessage == "string") ? aMessage
> +                                                                : expectedAndTitle.title;

let message = aMessage || expectedAndTitle.title;

should be sufficient here.

@@ +89,2 @@
>                      // only alarms expected as fired should exist in our fired alarm map
> +                    do_check_xor(expectedAndTitle.expected != EXPECT_FIRED,

Where is the expected property from here? Shouldn't that be just expectedAndTitle?

@@ +94,2 @@
>                      // only alarms expected as timers should exist in the service's timer map
> +                    do_check_xor(expectedAndTitle.expected != EXPECT_TIMER,

The same here.

Comment 11

[Decathlon]

(In reply to [:MakeMyDay] from comment #10)

> > +                    // if no explicit message has been passed, take the item title
> > +                    let message = (typeof aMessage == "string") ? aMessage
> > +                                                                : expectedAndTitle.title;
> 
> let message = aMessage || expectedAndTitle.title;
> 
> should be sufficient here.

I thought the same too, but when I tried it in the try-server:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1eb45a80975256599354259e25e82a466e1cefee

I found out that when checkExpected() is called here:
https://dxr.mozilla.org/comm-central/source/calendar/test/unit/test_alarmservice.js#348
it generated a message like:

[xpconnect wrapped (nsISupports, calICalendar, calISchedulingSupport, calIOfflineStorage, calISyncWriteCalendar, calICalendarProvider)]; check fired - true == true

so, identifying a non string, in that case, allows to use the item title to generate a useful message and the same when there is no argument passed to chechExpected().



> @@ +89,2 @@
> >                      // only alarms expected as fired should exist in our fired alarm map
> > +                    do_check_xor(expectedAndTitle.expected != EXPECT_FIRED,
> 
> Where is the expected property from here? Shouldn't that be just
> expectedAndTitle?
> 
> @@ +94,2 @@
> >                      // only alarms expected as timers should exist in the service's timer map
> > +                    do_check_xor(expectedAndTitle.expected != EXPECT_TIMER,
> 
> The same here.

Inside the function "expectResult()", the patch substitutes the single expected value with an object "expectedAndTitle" with two properties: "expected", that is the original expected value, and "title", that is the item title to be used as message for identify the item under test. The two elements are then used inside checkExpected() for the test and for the message.

Note that the object allows to generate a different message for each item inside the test addTestItems() 
   https://dxr.mozilla.org/comm-central/source/calendar/test/unit/test_alarmservice.js#172
that wouldn't be possible by passing string messages as argument to checkExpected() because the test stores a lot of expected values for different items (with more recurrences and more alarms for each item as well) inside the array expectResult() and, at the end, the verify of all the stored expected values is done by calling checkExpected() only _once_, see:
https://dxr.mozilla.org/comm-central/source/calendar/test/unit/test_alarmservice.js#172

Maybe there's another way too, but this seems to me the simplest: using directly a string message where it is possible and the item title otherwise.

Comment 12

[Decathlon]

MakeMyDay, has my explanation (in particular the second part) convinced you?

Comment 13

[[:MakeMyDay]]

Sorry for not responding earlier. Yes, that's fine for me, thanks. r=me

Comment 14

[Decathlon]

Attachment: patch-v1 with test - updated

updated patch

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/4cb52bea1cc6