991666 - [OMA CP] The notification of an accepted OMA CP message does not dissapear once it has been processed

Status: RESOLVED FIXED

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

Description

[isabelrios]

Attachment: VID_0003.3gp

v1.3 04/03 buri build:
Gecko-1bbed3c
Gaia-c5cd3a1

STEPS
Receive an OMA CP message
Open it tapping on the notification layer before it dissapears
Accept the message and store the APN
Go back to homescreen
Pull down the utility tray as there is still a notification about a configuration message
Tap on that notification

EXPECTED
Once the configuration message has been open and processed, the notification of its arrival should dissapear

ACTUAL
The notification of the configuration message does not dissapear once the message has been processed. It is necesary to go to the utility tray and tap on it. 
A message saying that the message has expired is shown.

Please see video file attached.

Comment 1

[Beatriz Rodríguez [:brg]]

This is reported as blocker during IOT process.

We do think this feature was working fine in 1.3 when we tested it in Nov and Jan.

Comment 2

[Jason Smith [:jsmith]]

David - Can you get someone assigned to look into this issue?

Comment 3

[David Scravaglieri [:scravag]]

Gabriele, could you take a look please ?

Comment 4

[Gabriele Svelto [:gsvelto]]

Yes, taking this. Isable since I don't have access to NowSMS or a similar program the only way I have to test this is within the emulator. For that I need either the raw SMS PDU of the message or its dumped out contents.

If you can't retrieve the raw SMS PDU one way to give me the contents of the message is the following:

- Make sure you're using an engineering build on your buri
- Send the message to your device
- Look into the logcat, you'll find something like this:

I/GeckoDump(  241): ###################### Wap Push Received ######################
I/GeckoDump(  241): Sender: [phone]
I/GeckoDump(  241): Content Type: text/vnd.wap.si
I/GeckoDump(  241): Content: <si><indication href="http://www.oreilly.com/">Check this website</indication></si>
I/GeckoDump(  241): ###############################################################

- Paste this message on the bug, this will allow me to reproduce the issue

Comment 5

[José Antonio Olivera Ortega [:jaoo]]

Attachment: 991666-OMACP-message.txt

Gabriele, OMA CP message example attached. Please feel free to request us what you need. FYI AFAIK this bug only reproduces on v1.3 branch. Thanks.

Comment 6

[Gabriele Svelto [:gsvelto]]

Thanks José, this should do the trick.

Comment 7

[José Antonio Olivera Ortega [:jaoo]]

Gabriele, just tested bug 991628 (OMA CP) in v1.3 and found this issue:

E/GeckoConsole(  841): [JavaScript Error: "TypeError: timestamp is undefined" {file: "app://wappush.gaiamobile.org/js/messagedb.js" line: 237}]

This seems to cause all messages have expired once the user click on a notification.

Comment 8

[Gabriele Svelto [:gsvelto]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #7)
> Gabriele, just tested bug 991628 (OMA CP) in v1.3 and found this issue:
> 
> E/GeckoConsole(  841): [JavaScript Error: "TypeError: timestamp is
> undefined" {file: "app://wappush.gaiamobile.org/js/messagedb.js" line: 237}]
> 
> This seems to cause all messages have expired once the user click on a
> notification.

Yeah, it looks like an issue in the notification event handler.

Comment 9

[Gabriele Svelto [:gsvelto]]

I managed to confirm the STR in comment 0 but this is not a WAP Push-specific issue: I get the same problem using the SMS app. Will investigate further to discover what's broken.

Comment 10

[Gabriele Svelto [:gsvelto]]

I've verified that this is not a WAP Push issue, the problem appears with all applications using notifications. When tapping on the notification on the overlay the relevant code in the system app does not remove the notification as it would do when tapped on the utility tray. I've traced down the issue to this statement which is being evaluated as false:

https://github.com/mozilla-b2g/gaia/blob/v1.3/apps/system/js/notifications.js#L245

I still have to figure out why this is happening but hopefully it should be a quick thing. Moving to the Gaia::System component as that's where this belongs.

Comment 11

[Jason Smith [:jsmith]]

(In reply to Gabriele Svelto [:gsvelto] from comment #10)
> I've verified that this is not a WAP Push issue, the problem appears with
> all applications using notifications. When tapping on the notification on
> the overlay the relevant code in the system app does not remove the
> notification as it would do when tapped on the utility tray. I've traced
> down the issue to this statement which is being evaluated as false:
> 
> https://github.com/mozilla-b2g/gaia/blob/v1.3/apps/system/js/notifications.
> js#L245
> 
> I still have to figure out why this is happening but hopefully it should be
> a quick thing. Moving to the Gaia::System component as that's where this
> belongs.

That means it's a regression from bug 890440.

Comment 12

[Gabriele Svelto [:gsvelto]]

After studying the code I can confirm this is indeed a regression introduced by bug 890440. I have more STRs than the original one as more scenarios can trigger this issue:

- SMS app

- open the SMS app and stay in the thread-list on the DUT
- send a message to the DUT
- tap on the notification when it appears in the overlay on top of the screen (not the utility tray)
- the SMS app shows the message but the notification is not removed

Comment 13

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH WIP] Fix removal of old API notifications when selected from the toaster

This patch fixes the issue by modifying Notification.tap() to not mix up the toaster and notification nodes when deciding if a notification should be removed. This lacks unit-tests so consider it a WIP, I'll add them ASAP.

Comment 14

[Gabriele Svelto [:gsvelto]]

Attachment: [PULL REQUEST] Fix removal of old API notifications when selected from the toaster

Here's the complete pull request.

Comment 15

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Fix removal of old API notifications when selected from the toaster

Comment 16

[Gabriele Svelto [:gsvelto]]

Note for the reviewer, my patch fixes the underlying issue to the problem described in comment 0, i.e. old style notifications are not properly removed when tapping on the toaster. My patch fixes this and was written against gaia/master but it also applies cleanly to gaia/1.3. The STR in comment 0 does not apply to gaia/master because the WAP Push app has been using the new API for notifications since 1.4 but other applications still using old-style notifications might be affected so I thought it would be better to fix it once and for all.

Comment 17

[Gabriele Svelto [:gsvelto]]

FYI this is the Travis run: https://travis-ci.org/mozilla-b2g/gaia/builds/22692981

Comment 18

[Gabriele Svelto [:gsvelto]]

Thanks for the review Alive, pushed to gaia/master cca15facac7ba6a8e8654685149a84a307523691

Comment 19

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8404681 [details] [diff] [review]
[PATCH] Fix removal of old API notifications when selected from the toaster

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 890440
[User impact] if declined: when tapping on the notification toaster the associated notification is not removed from the utility tray in applications using the old notification API, this affects practically all notifications in v1.3/v1.3t
[Testing completed]: tested on a device and in the emulator both with the SMS and WAP Push applications. The issue was also covered in unit-tests.
[Risk to taking this patch] (and alternatives if risky): none that I can think of, the change only affects old-style notifications and doesn't change existing code dealing with them except for the toaster-specific case in the system app
[String changes made]: none

Comment 20

[[:fabrice] Fabrice Desré]

Comment on attachment 8404681 [details] [diff] [review]
[PATCH] Fix removal of old API notifications when selected from the toaster

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

I'm not doing approvals for 1.4

Comment 21

[bhavana bajaj [:bajaj]]

NI on :echen to help verify this on master/m-c before the branch uplifts.

Comment 22

[Jason Smith [:jsmith]]

I think you meant to flag Eric.

Comment 23

[Eric Chang [:ericcc] [:echang]]

http://youtu.be/CLehe7gvFbE

MASTER
Gaia      553da99ab09b6b894d9f95bb06b16b6e1ddbf0a1                             
Gecko     https://hg.mozilla.org/mozilla-central/rev/5b6e82e7bbbf              
BuildID   20140414160205                                                       
Version   31.0a1         

The notification is clear, but the accept button is not working, let me search if there is existing bug for that.

Comment 24

[bhavana bajaj [:bajaj]]

(In reply to Eric Chang [:ericcc] [:echang] from comment #23)
> http://youtu.be/CLehe7gvFbE
> 
> MASTER
> Gaia      553da99ab09b6b894d9f95bb06b16b6e1ddbf0a1                          
> 
> Gecko     https://hg.mozilla.org/mozilla-central/rev/5b6e82e7bbbf           
> 
> BuildID   20140414160205                                                    
> 
> Version   31.0a1         
> 
> The notification is clear, but the accept button is not working, let me
> search if there is existing bug for that.

Assuming this as verified then, please file a new bug if need be as a follow-up here.

Comment 25

[Eric Chang [:ericcc] [:echang]]

Attachment: OMACP-3-Confirmation.png

With the OMA CP message sent from carrier, it works, make it all the way to the data settings, no need to file another bug, thanks.
                                                                    
Gaia      0996eecb1b34e66dba6ab082a9b4f03e6861c0de    
Gecko     b7d7d9a2d4c5c9c2d620d237685700dd560cafce    
BuildID   20140417041552                              
Version   30.0a2

Comment 26

[Ryan VanderMeulen [:RyanVM]]

v1.4: https://github.com/mozilla-b2g/gaia/commit/3b4f48230eefb87e0e7add68d36904626bcd38ef
v1.3: https://github.com/mozilla-b2g/gaia/commit/4aae5c8e9ac9a3d7ea7e83172dda6e11169cd677

Comment 27

[bmorgan]

2.0 Flame: When overlay notification is tapped, SMS app is opened and notification in utility tray is removed.

1.4 Buri: When overlay notification is tapped, SMS app is opened and notification in utility tray is removed.

1.3 Buri: When overlay notification is tapped, SMS app is opened and notification in utility tray is removed.


1.3 Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20140528024003
Gaia: 0ce948e378cab7ed3db20231281dd7ca2eb99779
Gecko: ce0dd450bffb
Version: 28.0
Firmware Version: v1.2-device.cfg

1.4 Environmental Variables:
Device: Buri 1.4 MOZ
BuildID: 20140529000204
Gaia: 7bc1c15c67661a0b8e35f18f15a9d03d1d2cfcd5
Gecko: 2181cac4d0fc
Version: 30.0
Firmware Version: v1.2-device.cfg

2.0 Environmental Variables:
Device: Flame 2.0 MOZ
BuildID: 20140529040201
Gaia: 4142df90c71abdc1e3a87cd37dff1a22d5e38b34
Gecko: 1e712b724d17
Version: 32.0a1
Firmware Version: 10g-2