987458 - [B2G][Calendar] Only one alarm will fire and only one notification displays if there are two events with alarms scheduled to fire at the same time

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Calendar, defect)

Description

[Rachel Pribble [:rpribble]]

Description:
If there are two events scheduled at the same time in the calendar and each has a 5 minute alarm, only one of the alarms will fire and the user only sees a notification for one of them.

Repro Steps:
1) Updated Buri to BuildID: 20140324000202
2) Open Calendar
3) Create two separate events on the same day at the same time, both with a 5 minute alarm
4) Observe only one of the alarms fire

Actual:
Only an alarm for one event fires.

Expected:
Alarms for all events always fire.

Environmental Variables:
Device: Buri v1.4 MOZ ril
BuildID: 20140324000202
Gaia: 730670951e40b2317a167fcd07c398bb662d6e87
Gecko: a44f8b39c2c8
Version: 30.0a2
Firmware Version: v1.2-device.cfg

Notes:

Repro frequency: 100%
Link to failed test case: https://moztrap.mozilla.org/manage/case/7581/
See attached: Screenshot

Comment 1

[Rachel Pribble [:rpribble]]

Attachment: OnlyOneAlarmofTwo.png

This issue also reproduces on the Buri v1.3 MOZ ril.

Environmental Variables:
Device: Buri v1.3 MOZ ril
BuildID: 20140324004001
Gaia: f7742fb4929cc57c9f72955ce5cebb8279745ac0
Gecko: e42b778a010f
Version: 28.0
Firmware Version: v1.2-device.cfg

Only one alarm fires when there are two events at the same time on the same day both with 5 minute alarms.

Comment 2

[Rachel Pribble [:rpribble]]

Attachment: log.txt

Comment 3

[Rachel Pribble [:rpribble]]

Note: This issue occurs even if the events are not scheduled at the same time. It was tested with two events at the same time with identical alarms, and two events at different times- one with a 5 minute alarm and one with an hour alarm, both set to fire at the same time- and this repros in both scenarios when there should be consecutive alarms firing.  The user will only see one alarm and will not be shown the other.

Comment 4

[Jason Smith [:jsmith]]

Does this happen on 1.1?

Comment 5

[Josh Schmitt [Joshs]]

(In reply to Jason Smith [:jsmith] from comment #4)
> Does this happen on 1.1?

On 1.1, both alarms fired at the same time (5 minute before event alarm).

Environmental Variables:
Device: Buri 1.1 MOZ
BuildID: 20140318041202
Gaia: 44a2ddf63373f8e95c784faf4ed4d60081699c61
Gecko: 2c70ef07c5b3
Version: 18.0
Firmware Version: V1.2-device.cfg

Comment 6

[Jason Smith [:jsmith]]

I don't think comment 5 completely describes the behavior we need to know here. We need to specifically know:

1. Which calendar notifications appear when the alarm goes off?
2. How many sounds are played when the alarm goes off?

Comment 7

[Josh Schmitt [Joshs]]

(In reply to Jason Smith [:jsmith] from comment #6)
> I don't think comment 5 completely describes the behavior we need to know
> here. We need to specifically know:
> 
> 1. Which calendar notifications appear when the alarm goes off?
> 2. How many sounds are played when the alarm goes off?

1. The first event made will be shown in the notification dropdown
2. Both event sounds are firing
3. In the notification center there are two calender events shown.

Comment 8

[Jason Smith [:jsmith]]

Ok - I can reproduce this on 1.3. Looks like multiple alarm notifications are failing to fire if they happen at the same time.

Comment 9

[Dylan Oliver [:doliver]]

There are very few recent changes to the alarm code -- identifying the regression window here would really help to narrow the search area.

Comment 10

[Harvey]

Last confirmed Working

1.3 Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20131005040201
Gaia: c9090021f7d642bae1db73a1093ab3dbb5078642
Gecko: b5d24ef1eb37
Version: 27.0a1
Firmware Version: v1.2-device.cfg

First confirmed Broken

1.3 Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20131008064334
Gaia: 122ff8c6363227501f4121e5a3892ba41d4c0417
Gecko: 64b497e6f593
Version: 27.0a1
Firmware Version: v1.2-device.cfg

First Broken Gecko Last Working Gaia: Issue reproduces 

Gaia: c9090021f7d642bae1db73a1093ab3dbb5078642
Gecko: 64b497e6f593

Last Working Gecko First Broken Gaia: Issue does not reproduce

Gaia: 122ff8c6363227501f4121e5a3892ba41d4c0417
Gecko: b5d24ef1eb37


Gecko Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b5d24ef1eb37&tochange=64b497e6f593

Note: This window has 3 broken builds that blocked closer regression.
20131006040201, 2013100040201, 20131008040205

Comment 11

[Jason Smith [:jsmith]]

The window above is incorrect. It's referencing build IDs that are part of 2013, not 2014. And 1.3 was on Aurora at this time, not central. Please check again.

Comment 12

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #11)
> The window above is incorrect. It's referencing build IDs that are part of
> 2013, not 2014. And 1.3 was on Aurora at this time, not central. Please
> check again.

Wait disregard - I see this now. This is referencing when 1.3 was on trunk.

Comment 13

[Jason Smith [:jsmith]]

bug 899574 is in the regression range here, which looks like a potential cause of this.

Mike - Can you take a look?

Comment 14

[Miller Medeiros [:millermedeiros]]

I was trying to write an integration test for this to avoid future regressions but since we can't update the system clock (b2g is tied to system) I don't think the solution I came up to test it is reliable enough. (I was creating events that would trigger a notification 10s in the future and checking if the displayed text matched the expected value) - on Travis the execution delay can be bigger than that which could cause intermittent failures.

I'll post it as an attachment for future reference, even tho implementation is brittle.

Comment 15

[Miller Medeiros [:millermedeiros]]

Attachment: brittle calendar test case

I'm not pushing this to the gaia repo since it relies on fast execution (very likely to fail on travis) and is also very tied to the way the event start time is currently handled by the calendar app. But maybe this can help you guys replicate the bug (it's way faster than creating events manually).

one thing I noticed on my tests is that if the gap between the alarms is bigger than 1 second it fires both notifications. But I'm not sure if it happens in 100% of the cases or if it was just a coincidence.

Comment 16

[Michael Henretty [:mikehenrty][:mhenretty]]

Looks like calendar still uses the old desktop notification api, so this is most likely not related to the Notification.get change. I'll still do some investigation though.

Comment 17

[Michael Henretty [:mikehenrty][:mhenretty]]

More information:

This is not a notification or alarm issue. The problem is that two requests to mozApps.getSelf() happen in quick succession, and somehow because of this the second DOMRequest fails to resolve. Looking at the changelog, my guess is that the offending changeset is this [1]. I am building b2g right now with that patch backed out to verify.

1.) https://hg.mozilla.org/mozilla-central/rev/e59ea92c55fc

I think a workaround we could put in place in gaia would be to queue calls to getApps here [2] to that we ensure only 1 request is made at a time. Obviously the ideal solution would be to fix the gecko issue, but in the interest of time we might need to improvise.

2.) https://github.com/mozilla-b2g/gaia/blob/0af32a7834c0bcd26936b8610a79d2c34a374f05/apps/calendar/js/notification.js#L20

Comment 18

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: Github PR

Gareth, this patch works around a gecko problem with sending multiple event reminders in close succession. More generally though, it prevents us from making a mozApps.getSelf() request for every event reminder, which I think is generally good. I know the code is ugly right now, and needs tests, but what are your thoughts of this approach?

Comment 19

[Jason Smith [:jsmith]]

(In reply to Michael Henretty [:mhenretty] from comment #18)
> Created attachment 8402776 [details] [review]
> Github PR
> 
> Gareth, this patch works around a gecko problem with sending multiple event
> reminders in close succession. More generally though, it prevents us from
> making a mozApps.getSelf() request for every event reminder, which I think
> is generally good. I know the code is ugly right now, and needs tests, but
> what are your thoughts of this approach?

Can we get a separate bug on file for gecko bug here?

Comment 20

[Jason Smith [:jsmith]]

No longer going to block on Mozilla-specific 1.3 blockers unless it hits a special exception list, so moving to 1.4? for triage.

Comment 21

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #20)
> No longer going to block on Mozilla-specific 1.3 blockers unless it hits a
> special exception list, so moving to 1.4? for triage.

Vance - Can you check with TCL to see if this is a blocker for them or not?

Comment 22

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to Jason Smith [:jsmith] from comment #21)
> (In reply to Jason Smith [:jsmith] from comment #20)
> > No longer going to block on Mozilla-specific 1.3 blockers unless it hits a
> > special exception list, so moving to 1.4? for triage.
> 
> Vance - Can you check with TCL to see if this is a blocker for them or not?

Hi Jason, this one is not reported by TCL

Comment 23

[Jason Smith [:jsmith]]

(In reply to Vance Chen [:vchen][[e-mail]] from comment #22)
> (In reply to Jason Smith [:jsmith] from comment #21)
> > (In reply to Jason Smith [:jsmith] from comment #20)
> > > No longer going to block on Mozilla-specific 1.3 blockers unless it hits a
> > > special exception list, so moving to 1.4? for triage.
> > 
> > Vance - Can you check with TCL to see if this is a blocker for them or not?
> 
> Hi Jason, this one is not reported by TCL

I know that, but that's not what I'm asking here. I'm asking if TCL sees this as a blocker or not.

Comment 24

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Jason Smith [:jsmith] from comment #19)
> Can we get a separate bug on file for gecko bug here?

Yup, if this is the approach we take (which is to work around in gaia) then I will file a bug on the gecko part. However, if this is not 1.3, this means we have more time, and so fixing this bug from the gecko side is more feasible. I'll await feedback from :gaye and triage decisions before deciding the best way forward.

Comment 25

[Milan Sreckovic [:milan] (needinfo for best results)]

Do we have a problem when the events are at a different time, but the notifications still fire at the same time (e.g., they're an hour apart, but one has a notification an hour before the event, while the other one has it at the time of the event.)

Comment 26

[Preeti Raghunath(:Preeti)]

1.4+

Comment 27

[Gareth Aye [:gaye] (back from PTO)]

Comment on attachment 8402776 [details] [review]
Github PR

I'm Gareth Aye and I approve of this platform workaround. Thanks so much for investigating :mhenretty. We owe you one <3!

Comment 28

[Jason Smith [:jsmith]]

TCL isn't going to IOT block this for 1.3.

Comment 29

[Michael Henretty [:mikehenrty][:mhenretty]]

Ok, given Gareth's feedback, and the fact this is 1.4+, let's land the gaia workaround and file a followup bug about the gecko getSelf() issue.

Comment 30

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8402776 [details] [review]
Github PR

Miller, you wanna have a look?

Comment 31

[Michael Henretty [:mikehenrty][:mhenretty]]

Filed the bug for the gecko issue:
https://bugzilla.mozilla.org/show_bug.cgi?id=994079

Comment 32

[Miller Medeiros [:millermedeiros]]

Comment on attachment 8402776 [details] [review]
Github PR

LGTM. the integration tests really validates that 2 notifications are being displayed. nice work!

PS: travis failed due to "/home/travis/build/mozilla-b2g/gaia/apps/search/test/marionette/places_search_test.js:84:15" (https://travis-ci.org/mozilla-b2g/gaia/jobs/22610830) - which is unrelated to these code changes.

Comment 33

[Michael Henretty [:mikehenrty][:mhenretty]]

master: https://github.com/mozilla-b2g/gaia/commit/a6a328b5c7ae1e497ecafe42e1b3a39f11886a25

Comment 34

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8402776 [details] [review]
Github PR

[Approval Request Comment]
This patch works around a bug in the platform where multiple calls to mozApps.getSelf() in close succession will fail. AFAICT, this bug only affects the calendar app at the moment, so the workaround is applied there. Another bug was filed to fix the platform regression.

[Bug caused by] (feature/regressing bug #):
Bug 915598

[User impact] if declined:
If a user schedules two event reminders for the same time, they will only receive the first reminder.

[Testing completed]:
Added an integration test to prevent further regressions.

[Risk to taking this patch] (and alternatives if risky):
Minor risk to event reminders.

[String changes made]: none.

Comment 35

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: Github PR, 1.4 merge

Unfortunately, the 1.4 uplift caused significant conflict. To be safe, I sent a new pull request. This is working on device, so I'll land when travis goes green.

https://travis-ci.org/mozilla-b2g/gaia/builds/22683931

Comment 36

[Michael Henretty [:mikehenrty][:mhenretty]]

1.4: https://github.com/mozilla-b2g/gaia/commit/4c9c88996da7617f49a0d1a97b23f11f2dbc503f

Comment 37

[Edward Chen[:edchen]]

One thing need to be confirmed, alarm fire just one time even system tray appeared 3 events. Does it match specification?

[Environment]
Gaia      23488b1a45221c17e6a32fdd4c9d0fdbdcf2d021
Gecko     https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/72055108f470
BuildID   20140414164002
Version   28.1
ro.build.version.incremental=eng.cltbld.20140414.200743
ro.build.date=Mon Apr 14 20:07:51 EDT 2014

[Result]
PASS

Comment 38

[Dylan Oliver [:doliver]]

ni? to mhenretty for question in comment #37

Comment 39

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Edward Chen[:edchen] from comment #37)
> One thing need to be confirmed, alarm fire just one time even system tray
> appeared 3 events. Does it match specification?

I don't know what the appropriate behavior is regarding the alarms. I'm not actually sure what it would look like for several alarms to fire at once. I checked the clock app, and it seems if you set two alarms for the same time only one of them gets displayed on screen when it fires, and only one sounds gets played as well. Dylan, do you know the correct UX person to ask about general alarm behavior?

ni? Josh, as comment 7 seems to suggest that two alarms can indeed fire at the same time on 1.1.
Josh, what does it look like when two alarms fire at once? Do you have to dismiss them both separately? Or is it just that two different sounds are playing simultaneously, but they both can be dismissed at once?

Comment 40

[Dylan Oliver [:doliver]]

Ah, I mis-interpreted comment #37. A single alarm with multiple notifications seems acceptable (and preferable) to me. 

Harly is the lead on Calendar UX.

Comment 41

[Josh Schmitt [Joshs]]

(In reply to Michael Henretty [:mhenretty] from comment #39)
> (In reply to Edward Chen[:edchen] from comment #37)
> > One thing need to be confirmed, alarm fire just one time even system tray
> > appeared 3 events. Does it match specification?
> 
> I don't know what the appropriate behavior is regarding the alarms. I'm not
> actually sure what it would look like for several alarms to fire at once. I
> checked the clock app, and it seems if you set two alarms for the same time
> only one of them gets displayed on screen when it fires, and only one sounds
> gets played as well. Dylan, do you know the correct UX person to ask about
> general alarm behavior?
> 
> ni? Josh, as comment 7 seems to suggest that two alarms can indeed fire at
> the same time on 1.1.
> Josh, what does it look like when two alarms fire at once? Do you have to
> dismiss them both separately? Or is it just that two different sounds are
> playing simultaneously, but they both can be dismissed at once?

I assume you mean 1.1 based on my previous comments of comment 4 and comment 7 and in  your comment.

When both alarms fire you get two sounds, only one event is shown in the notifications on screen and both events are shown in the notification center. When both alarms fire the user only sees one at the time of the firing so selecting one will take the user to the calendar app. If the user opens the notification center the user has two events shown that can be selected like normal.

Comment 42

[Harly Hsu[:harly]]

As an UX standpoint, when multiple events with alarms scheduled to fire at the same time, it is OK to only fire a single alarm. However, all the notifications should be displayed for the user to see the events.

Comment 43

[Edward Chen[:edchen]]

Hi Harly,

Thanks for the clarification.