1037371 - Notification close event handler not called

Status: RESOLVED FIXED

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

Description

[Alexandre LISSY :gerard-majax]

While having a look at the intermittent bug 1010415, I wanted to improve the gaia integration test suite and make it less race-risky by using events listener.

I noticed that 'show' event gets dispatched correctly, but 'close' was not. I have no idea why this was never caught by our current mochitests.

So far my current understanding is the following:
 - on .close(), in Notification.cpp we get into Notification::CloseInternal() and we call CloseAlert() on the alertService
 - this gets handled in b2g/components/AlertsService.js where we send an |alert-notification-close| message to AlertsHelper
 - AlertsHelper received it and calls this.closeAlert()
 - AlertsHelper.closeAlert() sends a MozChromeNotificationEvent to Gaia

Comment 1

[Alexandre LISSY :gerard-majax]

Attachment: Gaia patch exposing the issue

Comment 2

[Alexandre LISSY :gerard-majax]

Attachment: Gecko WIP workaround

This Gecko patch is a WIP workaround that does the job, but as far as I can say we should be getting desktop-notification-close back from Gaia.

Comment 3

[Alexandre LISSY :gerard-majax]

I'm suspecting this may be a regression that I introduced back in bug 963234.

Comment 4

[Alexandre LISSY :gerard-majax]

Attachment: Gaia fix

Fixing by just sending the mozContentNotificationEvent when we should have been doing this.

Comment 5

[Alexandre LISSY :gerard-majax]

Comment on attachment 8454405 [details] [review]
Gaia fix

That was my fault after all :)

Comment 6

[Alexandre LISSY :gerard-majax]

I'm taking the opportunity to remove executeScript() and use executeAsyncScript().

Comment 7

[Alexandre LISSY :gerard-majax]

It's all clean locally, tested with OOP Mulet:

  Notification.get():
    ✓ promise is fulfilled 
    ✓ promise returns a new notification (55ms)
    ✓ get works with tag option (53ms)
    ✓ should work across domains (5184ms)
    ✓ notifications should persist even after closing app (6615ms)
    ✓ bug 931307, empty title should not cause crash (64ms)

  Notification events
    ✓ click event starts application (5935ms)
    ✓ close event removes notification (5607ms)
    ✓ click event on resent notification starts application (6409ms)
    ✓ close event removes resent notification (7062ms)

  mozChromeNotifications:
    ✓ Checking mozResendAllNotifications API (112ms)
    ✓ Sending no notification, resends none (3723ms)
    ✓ Sending one notification, resends one (3499ms)
    ✓ Sending two notifications, resends two (3712ms)
    ✓ Sending two notifications, close one, resends one (3872ms)
    ✓ Sending one notification, remove from tray, resend (4955ms)


  16 passing (2m)

Comment 8

[Alexandre LISSY :gerard-majax]

One unrelated failure on Gaia-Try: https://tbpl.mozilla.org/?rev=bf2c350dfdd36bcda34fb76d30dfb5d880e09d13&tree=Gaia-Try

I triggered 30 jobs of integration tests to check if maybe we fixed bug 1010415.

Comment 9

[Jason Smith [:jsmith]]

Will this only happen in automation or can this be triggered in real developer use cases in an app context using onclose?

Comment 10

[Alexandre LISSY :gerard-majax]

(In reply to Jason Smith [:jsmith] from comment #9)
> Will this only happen in automation or can this be triggered in real
> developer use cases in an app context using onclose?

I'm not sure. For now, given the lack of reports, I'd say that this only happens on automation.

Comment 11

[Alexandre LISSY :gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #8)
> One unrelated failure on Gaia-Try:
> https://tbpl.mozilla.org/
> ?rev=bf2c350dfdd36bcda34fb76d30dfb5d880e09d13&tree=Gaia-Try
> 
> I triggered 30 jobs of integration tests to check if maybe we fixed bug
> 1010415.

This try was wrong, I forgot to remove the files from the exclusion list. This one makes sure it's the case: https://tbpl.mozilla.org/?rev=ccd987879afa6a759d2d43423bd4d5325be50d3f&tree=Gaia-Try

It shows green, and checking the full log shows that we executes the tests. I've retriggered a set of 50 jobs for Gaia Integration tests, to make sure.

Kevin, do you have figures regarding the intermittence?

Comment 12

[Kevin Grandon :kgrandon]

(In reply to Alexandre LISSY :gerard-majax from comment #11)
> (In reply to Alexandre LISSY :gerard-majax from comment #8)
> > One unrelated failure on Gaia-Try:
> > https://tbpl.mozilla.org/
> > ?rev=bf2c350dfdd36bcda34fb76d30dfb5d880e09d13&tree=Gaia-Try
> > 
> > I triggered 30 jobs of integration tests to check if maybe we fixed bug
> > 1010415.
> 
> This try was wrong, I forgot to remove the files from the exclusion list.
> This one makes sure it's the case:
> https://tbpl.mozilla.org/
> ?rev=ccd987879afa6a759d2d43423bd4d5325be50d3f&tree=Gaia-Try
> 
> It shows green, and checking the full log shows that we executes the tests.
> I've retriggered a set of 50 jobs for Gaia Integration tests, to make sure.
> 
> Kevin, do you have figures regarding the intermittence?

I've disabled a bunch of those tests that failed yesterday, sorry for the noise on your run. This test felt intermittent around ~5% of the time, but I don't have exact numbers.

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8454405 [details] [review]
Gaia fix

r+ if this fixes the failure.

Comment 14

[Alexandre LISSY :gerard-majax]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #13)
> Comment on attachment 8454405 [details] [review]
> Gaia fix
> 
> r+ if this fixes the failure.

That does fix a bug, but saldy Gaia-Try failures seems to be unrelated :(

Comment 15

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

Comment on attachment 8454405 [details] [review]
Gaia fix

Good catch! I'll also add an integration test for this.

Comment 16

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

Attachment: Gaia PR, integration test for close handler

Alex, can you take a look at this test? Locally it fails for me before your patch, and passes after.

Comment 17

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

I suggest we block on this. There are a couple of places inside of gaia where we use notification close handlers (like [1]), and also we want any 3rd-party apps to be able to use this ability too. This is a low-risk, high-reward patch that is already r+'ed.

1.) https://github.com/mozilla-b2g/gaia/blob/10b286513fe6719d739620a7a9153cda34b63c11/apps/system/js/voicemail.js#L145

Comment 18

[Alexandre LISSY :gerard-majax]

Comment on attachment 8455354 [details] [review]
Gaia PR, integration test for close handler

Simple and clear test case, there's not much to argue about :). Let's wait for Gaia Try result :)

Comment 19

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

(In reply to Alexandre LISSY :gerard-majax from comment #18)
> Comment on attachment 8455354 [details] [review]
> Gaia PR, integration test for close handler
> 
> Simple and clear test case, there's not much to argue about :). Let's wait
> for Gaia Try result :)

Well, we also need your patch to land first too :)

Comment 20

[Alexandre LISSY :gerard-majax]

(In reply to Michael Henretty [:mhenretty] from comment #19)
> (In reply to Alexandre LISSY :gerard-majax from comment #18)
> > Comment on attachment 8455354 [details] [review]
> > Gaia PR, integration test for close handler
> > 
> > Simple and clear test case, there's not much to argue about :). Let's wait
> > for Gaia Try result :)
> 
> Well, we also need your patch to land first too :)

Just rebased to not remove the tbpl test exclusion list

Comment 21

[Alexandre LISSY :gerard-majax]

Only intermittents: https://tbpl.mozilla.org/?rev=48a4c5a22be97c2c0ef0d1a4df03fbdc2ed2dc4e&tree=Gaia-Try

Comment 22

[Alexandre LISSY :gerard-majax]

https://github.com/mozilla-b2g/gaia/commit/49644a1e836356a4096bf2fda8b3b19a8f113d56
https://github.com/mozilla-b2g/gaia/commit/b64cb6f3f2a47582565b3fb06203477fb45c9d5a

Comment 23

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

Yuh oh, I think we are seeing some integration test failures related to this:

https://tbpl.mozilla.org/?tree=Gaia-Try&rev=28b72374cdd5

Investigating.

Comment 24

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

(In reply to Michael Henretty [:mhenretty] from comment #23)
> Yuh oh, I think we are seeing some integration test failures related to this:
> 
> https://tbpl.mozilla.org/?tree=Gaia-Try&rev=28b72374cdd5
> 
> Investigating.

Nm, the above failures were related to bug 1035430.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/d24d1744f8548a4b44280641dd77a7bd43238320
v2.0: https://github.com/mozilla-b2g/gaia/commit/cae98909946319d2b0a9436be21df64b8975c9d0