980132 - [B2G][Notifications API][Browser] "onclose" is not fire in logcat when notification is replaced

Status: RESOLVED WONTFIX

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

Description

[sarsenyev]

Attachment: log3514-2.txt

Description:
When replacing WebAPI Notification, "onclose isn't fire up in logcat

Repro Steps:
1) Updated Buri to Build ID: 20140304040200.
2) Go to http://mozqa.com/qa-testcase-data/webapi/notifications/index.html
3) Tap on "Request permission"
4) Upon permission granted, have title alone as "A" with tag x
5) Tap "Show Notification"
6) Replace title with letter "B"
7) Tap "Show Notification"

Actual:
The notification is replaced but doesn't show "onclose" in logcat

Expected:
The notification is replaced and "onclose" fires up in logcat

1.4 Environmental Variables:
Device: Buri 1.4 MOZ
BuildID: 20140304040200
Gaia: 6df4533f14ec2645bb13d8a803a5151583ca13a8
Gecko: 529b86b92b1d
Version: 30.0a1
Firmware Version: v1.2-device.cfg

Repro frequency: 100%, 
https://moztrap.mozilla.org/manage/cases/?filter-id=9954
See attached: logcat

Comment 1

[sarsenyev]

The issue doesn't reproduce on 1.3

1.3 Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20140304004001
Gaia: 242e477761643a440e6b244d291fb9d0ce204993
Gecko: 8b90bef8738c
Version: 28.0
Firmware Version: v1.2-device.cfg

Comment 2

[Andrea Marchesini [:baku]]

I suspect it's a regression in the console API. I take this.

Comment 3

[Jason Smith [:jsmith]]

Unblocking since this is web console regression, which is minor overall.

Comment 4

[Andrea Marchesini [:baku]]

On desktop I cannot reproduce it. I'm flashing the device to see there.

Comment 5

[Andrea Marchesini [:baku]]

It doesn't seem a regression in the Console API but some notification/gaia issue.
ConsoleAPI is not called at all. It means that 'close' event is not dispatched (?!?)

Comment 6

[Jason Smith [:jsmith]]

Ok - reverting blocking flag then since this is a notifications API regression.

Comment 7

[Andrea Marchesini [:baku]]

I don't know how the Notifications are displayed/dismissed on B2G. Alive, do you know if what and when should an alert div be closed?

On Desktop an alert is a window and the 'close' event is dispatched when that window receives the unload event. What about B2G?

Comment 8

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

(In reply to Andrea Marchesini (:baku) from comment #7)
> On Desktop an alert is a window and the 'close' event is dispatched when
> that window receives the unload event. What about B2G?

In B2G, we have a special case for when we detect a notification replacement by tag [1]. We need to manually fire a close event here (like we do here [2]) in order to follow the spec [3]. Should be a simple fix. Baku, you are welcome to still take this, but if you want someone from B2G to help out I or gerard-max could take it.


1.) https://github.com/mozilla-b2g/gaia/blob/98cf46/apps/system/js/notifications.js#L338
2.) https://github.com/mozilla-b2g/gaia/blob/98cf46/apps/system/js/notifications.js#L459
3.) http://notifications.spec.whatwg.org/#replacing-a-notification

Comment 9

[Andrea Marchesini [:baku]]

> In B2G, we have a special case for when we detect a notification replacement
> by tag [1]. We need to manually fire a close event here (like we do here
> [2]) in order to follow the spec [3]. Should be a simple fix. Baku, you are
> welcome to still take this, but if you want someone from B2G to help out I
> or gerard-max could take it.

Ok. Thanks.

Comment 10

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

Looks like don't need me here, clear.

Comment 11

[Matthew Vaughan]

This issue started reproducing on the 12/18/13 1.4 build. One thing I notice in the builds that this issue reproduces in is that notifications cannot be dismissed from the notifications dropdown. Even after being tapped in some way, the notification will persist and therefore not trigger the "onclose".

- Last Working -
Device: Buri v1.4 MOZ RIL
BuildID: 20131217040201
Gaia: 545aacf3feff6430140cc9ade757002df4895b77
Gecko: b1e5ade62913
Version: 29.0a1
Firmware Version: V1.2-device.cfg

- First Broken -
Device: Buri v1.4 MOZ RIL
BuildID: 20131218040201
Gaia: e2f0e09e980b1cb3275a0bb033931cb48f9d521c
Gecko: 862cb6a1cc88
Version: 29.0a1
Firmware Version: V1.2-device.cfg

**After swapping the builds' geckos/gaias, this looks to be a gaia issue**

last working gaia/first broken gecko = NO REPRO
Gaia  545aacf3feff6430140cc9ade757002df4895b77
Gecko 862cb6a1cc88

first broken gaia/last working gecko = REPRO
Gaia  e2f0e09e980b1cb3275a0bb033931cb48f9d521c
Gecko b1e5ade62913

Push log: https://github.com/mozilla-b2g/gaia/compare/545aacf3feff6430140cc9ade757002df4895b77...e2f0e09e980b1cb3275a0bb033931cb48f9d521c

Comment 12

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

(In reply to Matthew Vaughan from comment #11)
> [snip]... One thing I notice
> in the builds that this issue reproduces in is that notifications cannot be
> dismissed from the notifications dropdown. Even after being tapped in some
> way, the notification will persist and therefore not trigger the "onclose".

This was an intentional change. See https://bugzilla.mozilla.org/show_bug.cgi?id=980593#c4

Comment 13

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

Hrm, looking at the code at the code it's hard for me to believe that B2G ever fired the onclose handler when replacing notifications. I tried with the last known working version of Gaia (545aacf3feff6430140cc9ade757002df4895b77), and I didn't see the onclose logcat entry. I also tried with the 1.3 build specified in comment 1, but still no "onclose". The only time I see the "onclose" entry is if I background and then foreground the browser, which is a bug we fixed (bug 890440).

Matthew, would you mind attaching the logcat dump from your last working build so I can poke around?

Comment 14

[Jason Smith [:jsmith]]

(In reply to Michael Henretty [:mhenretty] from comment #13)
> Hrm, looking at the code at the code it's hard for me to believe that B2G
> ever fired the onclose handler when replacing notifications. I tried with
> the last known working version of Gaia
> (545aacf3feff6430140cc9ade757002df4895b77), and I didn't see the onclose
> logcat entry. I also tried with the 1.3 build specified in comment 1, but
> still no "onclose". The only time I see the "onclose" entry is if I
> background and then foreground the browser, which is a bug we fixed (bug
> 890440).
> 
> Matthew, would you mind attaching the logcat dump from your last working
> build so I can poke around?

Note as a point of comparison - onclose fires correctly in the Desktop Firefox case just fine when replacing a notification.

Comment 15

[Jason Smith [:jsmith]]

If the regression range is correct here, then bug 890440 is definitely the cause.

Comment 16

[Jason Smith [:jsmith]]

Leaving qawanted for getting the logcat for the last working build.

Comment 17

[sarsenyev]

Attachment: log31114-2.txt

Logcat from the last working build

Comment 18

[Matthew Vaughan]

removing NI per comment 17.

Comment 19

[sarsenyev]

Note: "onclose" fires up only when closing browser app by tapping the home screen
And notification disappears from the notification screen when tapping the home button
On broken build "onclose" doesn't fire up at all

Comment 20

[Jason Smith [:jsmith]]

(In reply to sarsenyev from comment #19)
> Note: "onclose" fires up only when closing browser app by tapping the home
> screen
> And notification disappears from the notification screen when tapping the
> home button
> On broken build "onclose" doesn't fire up at all

That's the not the bug here. The bug here doesn't include STR that includes hitting the home button.

QA Wanted to reclarify STR.

Comment 21

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

Attachment: Github PR

While we are figuring out the regression window, let's have Alexandre take a look at the fix.

Comment 22

[Jason Smith [:jsmith]]

(In reply to Michael Henretty [:mhenretty] from comment #21)
> Created attachment 8389536 [details]
> Github PR
> 
> While we are figuring out the regression window, let's have Alexandre take a
> look at the fix.

If you've already got a patch at this point, then I don't think we need to continue QA analysis.

Comment 23

[:gerard-majax]

(In reply to Michael Henretty [:mhenretty] from comment #21)
> Created attachment 8389536 [details]
> Github PR
> 
> While we are figuring out the regression window, let's have Alexandre take a
> look at the fix.

I'm having a look. One of the travis jobs timed out, I've restarted it.

Comment 24

[Jason Smith [:jsmith]]

Unblocking this as checking this myself, I'm not seeing onclose fire in the logcat when replacing a notification. So this is not a regression.

Comment 25

[:gerard-majax]

Comment on attachment 8389536 [details]
Github PR

This is working fine after live testing. There is a few factoring that we may need to do before landing this, but other than that, I'm fine with this change.
I took time, also, to check whether this will work when we will land the notification after reboot support, and good news, it should be okay by then.

Comment 26

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

Comment on attachment 8389536 [details]
Github PR

Ugh, unfortunately this is not the right approach. We need to fire the close event on the old notification, not the new one. But sadly the system app has no way to distinguish between the two right now. This is because the unique identifier for each notification, which comes from gecko's alert service, is a combo of origin(app)/tag, which is shared by both the old and new notification. To fix this properly it will require changes to the alert service.

Comment 27

[:gerard-majax]

(In reply to Michael Henretty [:mhenretty] (PTO until June 30th) from comment #26)
> Comment on attachment 8389536 [details]
> Github PR
> 
> Ugh, unfortunately this is not the right approach. We need to fire the close
> event on the old notification, not the new one. But sadly the system app has
> no way to distinguish between the two right now. This is because the unique
> identifier for each notification, which comes from gecko's alert service, is
> a combo of origin(app)/tag, which is shared by both the old and new
> notification. To fix this properly it will require changes to the alert
> service.

We have timestamp available now.

Comment 28

[:gerard-majax]

This bug may lead to inconsistent user UX when dealing with resent notifications.

Comment 29

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

(In reply to Alexandre LISSY :gerard-majax from comment #28)
> This bug may lead to inconsistent user UX when dealing with resent
> notifications.

Can you be more specific? What use case are you concerned about with resent notifications?

Comment 30

[:gerard-majax]

(In reply to Michael Henretty [:mhenretty] from comment #29)
> (In reply to Alexandre LISSY :gerard-majax from comment #28)
> > This bug may lead to inconsistent user UX when dealing with resent
> > notifications.
> 
> Can you be more specific? What use case are you concerned about with resent
> notifications?

That's a good question, now that you are back from PTO :). I think my point was to remind that we had not fixed this now, and that the missing bit you were asking for is now available. So:
 - we now have timestamp
 - not having timestamp was a problem
 - not closing notification will lead to strange UX for user

Do you think we can fix this now?

Comment 31

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

I think instead of timestamp, you mean dbID? So yes I think we can fix this now!

(In reply to Alexandre LISSY :gerard-majax from comment #28)
> This bug may lead to inconsistent user UX when dealing with resent
> notifications.

Still not sure what this means. Do you mean by fixing this bug, we get inconsistent behavior, or not fixing this bug?

Comment 32

[BMO Automation]

Firefox OS is not being worked on