Closed Bug 1037371 Opened 8 years ago Closed 8 years ago

Notification close event handler not called

Categories

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

defect
Not set
normal

Tracking

(blocking-b2g:2.0+, b2g-v1.3 ?, b2g-v1.3T ?, b2g-v1.4 ?, b2g-v2.0 fixed, b2g-v2.1 fixed)

RESOLVED FIXED
2.0 S6 (18july)
blocking-b2g 2.0+
Tracking Status
b2g-v1.3 --- ?
b2g-v1.3T --- ?
b2g-v1.4 --- ?
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: gerard-majax, Assigned: gerard-majax)

References

Details

(Keywords: regression, Whiteboard: [systemsfe])

Attachments

(2 files, 2 obsolete files)

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
Attached patch Gaia patch exposing the issue (obsolete) — Splinter Review
Attached patch Gecko WIP workaround (obsolete) — Splinter Review
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.
I'm suspecting this may be a regression that I introduced back in bug 963234.
Depends on: 963234
Keywords: regression
Attached file Gaia fix
Fixing by just sending the mozContentNotificationEvent when we should have been doing this.
Attachment #8454354 - Attachment is obsolete: true
Attachment #8454358 - Attachment is obsolete: true
Component: DOM: Events → Gaia::System
Product: Core → Firefox OS
Version: Trunk → unspecified
Comment on attachment 8454405 [details] [review]
Gaia fix

That was my fault after all :)
Attachment #8454405 - Flags: review?(mhenretty)
Attachment #8454405 - Flags: review?(alive)
I'm taking the opportunity to remove executeScript() and use executeAsyncScript().
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)
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.
Will this only happen in automation or can this be triggered in real developer use cases in an app context using onclose?
(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.
(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?
Flags: needinfo?(kgrandon)
Whiteboard: [systemsfe]
Target Milestone: --- → 2.0 S6 (18july)
(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.
Flags: needinfo?(kgrandon)
Comment on attachment 8454405 [details] [review]
Gaia fix

r+ if this fixes the failure.
Attachment #8454405 - Flags: review?(alive) → review+
(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 on attachment 8454405 [details] [review]
Gaia fix

Good catch! I'll also add an integration test for this.
Attachment #8454405 - Flags: review?(mhenretty) → review+
Alex, can you take a look at this test? Locally it fails for me before your patch, and passes after.
Attachment #8455354 - Flags: review?(lissyx+mozillians)
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
blocking-b2g: --- → 2.0?
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 :)
Attachment #8455354 - Flags: review?(lissyx+mozillians) → review+
(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 :)
blocking-b2g: 2.0? → 2.0+
Whiteboard: [systemsfe] → [systemsfe][ETA=7/15]
(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
Yuh oh, I think we are seeing some integration test failures related to this:

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

Investigating.
(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.
You need to log in before you can comment on or make changes to this bug.