930794 - Notification.get() returns notifications for cleared notifications

Status: RESOLVED FIXED

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

Description

[James Burke [:jrburke]]

Attachment: Example app, unzip and add to gaia/apps and push to device

This is related to the work done in bug 899574:

If a notification is generated, and either the user clicks on the notification or clears the notification from the notification tray, I expected that this would remove the notification from the list of notifications tracked for Notification.get() calls.

However, the notification seems to stick around. I will attach a test app that shows the behavior.

When talking to :mikehenrty in IRC, he asked to be assigned to this bug, and pointed to this section as possibly being the issue, where the notification.close may not be called as part of it:

https://github.com/mozilla-b2g/gaia/blob/535469386800afec205ea2047f4fa0863ce65eab/apps/system/js/notifications.js#L393

Comment 1

[James Burke [:jrburke]]

For the example app, click the button to trigger a notification. Then either click the notification or clear it from the notification area.

Then click on the button again in the app to generate the new notification. It grabs any Notification.get() notifications and uses a count stored in it to update total count.

In the expected case, the count should always be 1 if the notification was previously cleared/no longer visible to the user.

Comment 2

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

Notification.close() should be called whenever a user either clears or clicks on a notification. So we should close the notification here as well:

https://github.com/mozilla-b2g/gaia/blob/535469386800afec205ea2047f4fa0863ce65eab/apps/system/js/notifications.js#L216

Comment 3

[James Burke [:jrburke]]

Would it be possible to get this scheduled for 1.4? 

Since bug 890440 landed, it changed the auto closing behavior of notifications, so we would like to land bug 922722 and bug 915643 on the email side to adjust to the new behavior, but this bug blocks those other bugs.

Comment 4

[Gregor Wagner [:gwagner]]

Peter, thats a bug for our backlog.

Comment 5

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

This is something I really need to fix, but after talking to Gregor it won't block the release. We will put it on our backlog (non-blocking 1.4 bugs).

Comment 6

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

Thinking about how to fix this, the problem is that the system app doesn't have the actual notification object when it displays the notification, it only has a copy of the data (passed to it from shell.js). In order to properly call close(), it will either need the original notification object, or it will need to be passed some function to trigger a close on the original notification object. Simply firing the 'desktop-notification-close' mozContentEvent will not remove the object from notification storage.

Comment 7

[Jason Smith [:jsmith]]

I'm unsure if what's proposed here in the expected behavior is the right path we want to go down. For example, there might be reasons why we want to get a notification after it's closed, as that was a motivating factor for why bug 899574 was filed in the first place. What I think we should be doing here instead is providing a state-driven attribute to determine if the notification is currently displayed or closed.

Comment 8

[Jason Smith [:jsmith]]

Anne - Can you weigh in here from the spec perspective on what we should do here?

Comment 9

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

(In reply to Jason Smith [:jsmith] from comment #7)
> I'm unsure if what's proposed here in the expected behavior is the right
> path we want to go down. For example, there might be reasons why we want to
> get a notification after it's closed, as that was a motivating factor for
> why bug 899574 was filed in the first place. What I think we should be doing
> here instead is providing a state-driven attribute to determine if the
> notification is currently displayed or closed.

Based on the spec, we should not be returning closed notifications in Notification.get()
http://notifications.spec.whatwg.org/#close-steps
http://notifications.spec.whatwg.org/#dom-notification-get (step 4)

In the current implementation, if you call .close() on a notification it will correctly no longer be returned in Notification.get(). The problem here is that the system app has no handle to this notification (since it is created by one of the apps), and so it has no ability to call .close(). In the past we worked around this by manually firing the event (desktop-notification-close) in the system app. However, this is no longer sufficient since this event will not remove the notification from storage. We either need to provide a way for the system app to call close on the notifications it receives, or remove notifications from storage based on this event rather than based on calling .close().

Comment 10

[Jason Smith [:jsmith]]

Okay - I'll have to update the test cases for this then.

Comment 11

[Anne (:annevk)]

Wait, you're saying the main reason to have Notification.get() is to get closed notifications? If that is indeed true we should revisit the specification... I thought the reason mostly to know what notifications are in the "notification center" for a given global.

Comment 12

[Jason Smith [:jsmith]]

(In reply to Anne (:annevk) from comment #11)
> Wait, you're saying the main reason to have Notification.get() is to get
> closed notifications? If that is indeed true we should revisit the
> specification... I thought the reason mostly to know what notifications are
> in the "notification center" for a given global.

Hmm...I was going off these statements here -  https://bugzilla.mozilla.org/show_bug.cgi?id=899574#c0:

"However, in real cases, we may display the notification in one app instance, and close the notification in another app instance. I mean that a notification might be long-lived, survive app restart, and even survive reboot (see bug 881159 and bug 874364).

On the Desktop, it might survive a Firefox Desktop restart, or navigating away the website and coming back to it."

Although I later read mention about the easiest fix was to get an active/pending notification list for a website. I think I might have misread the comment though - I see mention that the main use case for this is for dealing with cases when the notification is long-lived (e.g. survive app restart, survive reboot).

Comment 13

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

Note: the implementation for this will be much easier once we fix bug 874364. When the system app can fetch all notifications across apps, it should be able to call .close() on them when they are cleared.

Note2: we may need to add extra information (like "id") to the Notification.get() results so that the system app knows which notification to close in the case where a user only clears a single notification.

Comment 15

[:gerard-majax]

As we discussed on IRC, I started working on this using my dogfooding profile. This made me aware of one issue: we lack an upgrade process in the notificationdb/notificationstorage mechanism. Rationale is that, for example, we may already have notifications stored that lacks some fields.

Comment 16

[:gerard-majax]

I have a patch that is starting to work to address this issue. For now, biggest change is that we need to get the ID used for database. This means we have to add an element to the AppNotificationServiceOptions dictionnary and add it in Notification.cpp when we call ShowAppNotification.

The same kind of change is needed in my API for resending notification at reboot. I'll post patch when ready to get feedback.

Comment 17

[:gerard-majax]

Attachment: 0003-Bug-930794-Delete-notification-cleared-from-tray.patch

Michael, please find attached this patch that addresses the issue. This relies on the notifications after reboot patches, but it's easy to apply without. For now, only a feedback on how to do it, no test.

Comment 18

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

Comment on attachment 8392925 [details] [diff] [review]
0003-Bug-930794-Delete-notification-cleared-from-tray.patch

The naming convention is really too bad, ie that we call alertName "id" when it reaches the alert service, but "id" means something very different inside Notification.cpp and NotificationStorage. In any case, not really much we can/should do about that at this point.

Comment 19

[:gerard-majax]

(In reply to Michael Henretty [:mhenretty] from comment #18)
> Comment on attachment 8392925 [details] [diff] [review]
> 0003-Bug-930794-Delete-notification-cleared-from-tray.patch
> 
> The naming convention is really too bad, ie that we call alertName "id" when
> it reaches the alert service, but "id" means something very different inside
> Notification.cpp and NotificationStorage. In any case, not really much we
> can/should do about that at this point.

Thanks. I'll ask review on this once I have tests.

Comment 20

[:gerard-majax]

Attachment: 0001-Bug-930794-Delete-notification-cleared-from-tray.patch

Updated the patch: do not depend on notification-after-reboot branch. Also, as discussed on IRC, I will implement testing for this as a system app integration test on Gaia side.

Comment 21

[:gerard-majax]

Attachment: Link to Github https://github.com/mozilla-b2g/gaia/pull/17458

Pleas find attached a link to the github pull request implementing an integration test targetting this.

In your Gaia checkout, you will have to run:
$ make test-integration TEST_FILES=apps/system/test/marionette/notification_get_test.js

If it is done against a gecko which does not have the patch, the new test will fail. Symlinking gaia/b2g/ to your obj-x86_64-unknown-linux-gnu/dist/b2g/, and running the test again, it should now pass. At least this is the behavior I get locally.

Comment 22

[:gerard-majax]

Travis is failing on the new test, which is expected until the gecko patch lands.

Comment 23

[:gerard-majax]

Attachment: 0001-Bug-930794-Delete-notification-cleared-from-tray.patch

Rebased, this is still working as expected.

Comment 24

[:gerard-majax]

Attachment: 0001-Bug-930794-Delete-notification-cleared-from-tray.patch

Updated, landing this first before the next changes to notifications.

Comment 25

[:gerard-majax]

Attachment: 0001-Bug-930794-Delete-notification-cleared-from-tray.patch

Rebased a blocking bug 967475

Comment 26

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

Comment on attachment 8394887 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/17458

Made a build, and ran this test, looks good to me!

Comment 27

[:gerard-majax]

Attachment: 0001-Bug-930794-Delete-notification-cleared-from-tray.patch

Carrying previous r+, added a check on listener.dbId value.

Comment 28

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

Comment on attachment 8400726 [details] [diff] [review]
0001-Bug-930794-Delete-notification-cleared-from-tray.patch

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

Looks good with the single nit. Test this, and it works well on b2g. Can we also get a try run to make sure we don't break anything on desktop?

::: b2g/components/AlertsService.js
@@ +133,4 @@
>  
>      // we're done with this notification
>      if (topic === "alertfinished") {
> +      notificationStorage.delete(listener.manifestURL, listener.dbId);

Let's make sure we have a dbId before calling notificationStorage.delete. In the case this notification is a desktop notification (old api), the storage service will still make an IPC call with the undefined id for desktop notifications. So let's prevent that.

Comment 29

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

Comment on attachment 8400726 [details] [diff] [review]
0001-Bug-930794-Delete-notification-cleared-from-tray.patch

Midair bugzilla collision.

Comment 30

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

Comment on attachment 8403235 [details] [diff] [review]
0001-Bug-930794-Delete-notification-cleared-from-tray.patch

Carrying r+ from previous review, since comments were addressed.

Comment 31

[:gerard-majax]

Thanks. Putting checkin-needed for the Gecko part only. Once the gecko part has made it through building, travis should be green on the Gaia part and we can land and close this.

Comment 32

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/b2g-inbound/rev/b7c010d00d7e

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b7c010d00d7e

Comment 35

[:gerard-majax]

And now Travis gets green: https://travis-ci.org/mozilla-b2g/gaia/builds/22753402 «    ◦ desktop-notification-close removes notification:   ✓ desktop-notification-close removes notification (5481ms) »

Comment 36

[:gerard-majax]

https://github.com/mozilla-b2g/gaia/commit/30d9f891be1d8ea2321e687a05ccf0c00bcbaadf

Comment 38

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

This issue is occurring on the latest 2.0 Flame build.

Device: Flame 2.0
Build ID: 20140620000202
Gaia: c5dd47e3f9e18872961946735fdbc071a1656ac9
Gecko: 71b1b4b0850c
Version: 32.0a2 (2.0)
Firmware Version: v121-2

Results: Notifications from previous sessions are shown when using the get.

Comment 39

[Jason Smith [:jsmith]]

This is a closed bug - please file a new bug for the issue found.

Comment 40

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Please see bug 1030222.