Closed Bug 1006537 Opened 6 years ago Closed 5 years ago

System message not sent when application is killed and user taps on notification

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- fixed

People

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

References

Details

(Keywords: regression, Whiteboard: [systemsfe])

Attachments

(1 file)

While adding more tests coverage to bug 967475, I noticed that we regressed sending system message after tapping a notification when the application is not running.

Michael, this is code path I don't clearly understand.

We are able to send the app-notification-return message in http://dxr.mozilla.org/mozilla-central/source/b2g/components/AlertsHelper.jsm#131 to http://dxr.mozilla.org/mozilla-central/source/b2g/components/AlertsService.js#130

I understand that we should be triggering system message code path if the application is not running anymore. So far, I do see this behavior when the notification is resent after reboot (bug 967475) but if I send a notification, kill the app, then tap on it, it does not go through the system message code path and instead tries to trigger the observer (which seems to be still running).

Below are some logs.

=*= AlertsHelper.jsm : Notification event: desktop-notification-click for app://uitest.gaiamobile.org/manifest.webapp#notag:{3f3c4a2a-033d-40a8-b36d-7560b024ccde}
=*= AlertsHelper.jsm : handleNotificationEvent: uid=app://uitest.gaiamobile.org/manifest.webapp#notag:{3f3c4a2a-033d-40a8-b36d-7560b024ccde}
=*= AlertsHelper.jsm : handleNotificationEvent: uid=app://uitest.gaiamobile.org/manifest.webapp#notag:{3f3c4a2a-033d-40a8-b36d-7560b024ccde} topic=alertclickcallback
=*= AlertsHelper.jsm : handleNotificationEvent: uid=app://uitest.gaiamobile.org/manifest.webapp#notag:{3f3c4a2a-033d-40a8-b36d-7560b024ccde} topic=alertclickcallback return mm=[object ChromeMessageSender] target=app://uitest.gaiamobile.org/index.html
=*= AlertsService.js : receiveMessage: aMessage.name=app-notification-return
=*= AlertsService.js : receiveMessage: aMessage.name=app-notification-return uid=app://uitest.gaiamobile.org/manifest.webapp#notag:{3f3c4a2a-033d-40a8-b36d-7560b024ccde}
=*= AlertsService.js : receiveMessage: aMessage.name=app-notification-return uid=app://uitest.gaiamobile.org/manifest.webapp#notag:{3f3c4a2a-033d-40a8-b36d-7560b024ccde} topic=alertclickcallback
=*= AlertsService.js : receiveMessage: aMessage.name=app-notification-return uid=app://uitest.gaiamobile.org/manifest.webapp#notag:{3f3c4a2a-033d-40a8-b36d-7560b024ccde} topic=alertclickcallback observer:[xpconnect wrapped nsIObserver] observe:function observe() {
    [native code]
}
NotificationObserver::Observe => alertclickcallback
Flags: needinfo?(mhenretty)
blocking-b2g: --- → 2.0?
Is it expected that the observer is still valid after the application has been killed ?
This only happens when runnig non OOP build. With a B2G Desktop with OOP, the observer seems to correctly disappears and we properly send system message:

=*= AlertsHelper.jsm : Notification event: desktop-notification-click for app://uitest.gaiamobile.org/manifest.webapp#notag:{4e49f8f3-7a67-409a-85d7-fb1e72fba035}
=*= AlertsHelper.jsm : handleNotificationEvent: uid=app://uitest.gaiamobile.org/manifest.webapp#notag:{4e49f8f3-7a67-409a-85d7-fb1e72fba035}
=*= AlertsHelper.jsm : handleNotificationEvent: uid=app://uitest.gaiamobile.org/manifest.webapp#notag:{4e49f8f3-7a67-409a-85d7-fb1e72fba035} topic=alertclickcallback
=*= AlertsHelper.jsm : handleNotificationEvent: uid=app://uitest.gaiamobile.org/manifest.webapp#notag:{4e49f8f3-7a67-409a-85d7-fb1e72fba035} topic=alertclickcallback return mm=[object ChromeMessageSender] target=app://uitest.gaiamobile.org/index.html
=*= AlertsHelper.jsm : handleNotificationEvent: uid=app://uitest.gaiamobile.org/manifest.webapp#notag:{4e49f8f3-7a67-409a-85d7-fb1e72fba035} topic=alertclickcallback systemMessage
Removing the blocking flag since it's only in non OOP
blocking-b2g: 2.0? → ---
We should block as various apps might run inproc but still send notifications (e.g. 1.3t tarako builds).

Also, if the cause is in Gecko we should move the component.
blocking-b2g: --- → 2.0?
Component: Gaia::System → General
Whiteboard: [systemfe]
FYI, I've rechecked the status of this regarding recent mulet notifications fixes. Checking out 26aa525998c78101a56a990b75c0b7da9a8a29bb (from git.mozilla.org), rebuilding, running the new gaia integration test that exposes the bug, I'm reproducing this when B2G Desktop is built without OOP enabled. Once OOP is enabled, it is also working. So I don't think it's due to bug 963234.
Component: General → DOM: Device Interfaces
Product: Firefox OS → Core
Whiteboard: [systemfe] → [systemsfe]
Sounds like the observer is only cleaned up when the process itself is killed, which doesn't happen with non-OOP or certain Tarako apps. Alexandre, you mentioned on IRC the possibility of checking if the window object was present before notifying the observer. This sounds like a workable solution, but then we would still have these "dead" observers sticking around. Were you able to investigate having the notification object itself be able to unregister the observer in it's destructor?
Flags: needinfo?(mhenretty)
(In reply to Michael Henretty [:mhenretty] from comment #6)
> Sounds like the observer is only cleaned up when the process itself is
> killed, which doesn't happen with non-OOP or certain Tarako apps. Alexandre,
> you mentioned on IRC the possibility of checking if the window object was
> present before notifying the observer. This sounds like a workable solution,
> but then we would still have these "dead" observers sticking around. Were
> you able to investigate having the notification object itself be able to
> unregister the observer in it's destructor?

Partly: I had a look at other components in Notification.cpp, especially NotificationStorageCallback, and I saw it uses a bunch of macros to give collection hints: I'm wondering if we are not just missing some for NotificationObserver, so that the system knows that it must destroy it when destroying Notification object.
Doug, Vivien recommended needinfo? you on this topic. I fear we may lack some cycle collection magical macro for the NotificationObserver object, but I haven't been able to find any info on my own on this topic, so I'm reaching out to you for some help :)
Flags: needinfo?(dougt)
Flags: needinfo?(dougt)
Doug, can you recommend someone else for this maybe?
Flags: needinfo?(dougt)
Meanwhile, here is a patch that checks the window object in the observer and returns an error if it's not there. With this workaround, at least, I can re-enable the skipped test in Gaia integration tests.
Attachment #8434128 - Flags: review?(mhenretty)
blocking-b2g: 2.0? → 2.0+
Comment on attachment 8434128 [details] [diff] [review]
0001-Bug-1006537-Check-window-validity-in-NotificationObs.patch

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

Returning the error when we have no window means an exception will get throw when the alert service tries to notify the observer[1], correct? If so, LGTM.

1.) http://hg.mozilla.org/mozilla-central/annotate/ba7c2ae77d55/b2g/components/AlertsService.js#l139
Attachment #8434128 - Flags: review?(mhenretty) → review+
(In reply to Michael Henretty [:mhenretty] from comment #11)
> Comment on attachment 8434128 [details] [diff] [review]
> 0001-Bug-1006537-Check-window-validity-in-NotificationObs.patch
> 
> Review of attachment 8434128 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Returning the error when we have no window means an exception will get throw
> when the alert service tries to notify the observer[1], correct? If so, LGTM.
> 
> 1.)
> http://hg.mozilla.org/mozilla-central/annotate/ba7c2ae77d55/b2g/components/
> AlertsService.js#l139

Yes, it will throw when calling the listener and thus goes to the system message code path.
Flags: needinfo?(dougt)
Keywords: checkin-needed
Blocks: 1020982
Do you happen to have a Try link handy for this? :)
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #13)
> Do you happen to have a Try link handy for this? :)

Yep, I forgot to add it. It's there: https://tbpl.mozilla.org/?tree=Try&rev=c511584f5248
Flags: needinfo?(ryanvm)
Just set checkin-needed next time ;)
Flags: needinfo?(ryanvm)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/262aa76f4bd2
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.