Closed Bug 991666 Opened 7 years ago Closed 7 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:1.3+, b2g-v1.3 verified, b2g-v1.3T fixed, b2g-v1.4 verified, b2g-v2.0 verified)

RESOLVED FIXED
1.4 S6 (25apr)
blocking-b2g 1.3+
Tracking Status
b2g-v1.3 --- verified
b2g-v1.3T --- fixed
b2g-v1.4 --- verified
b2g-v2.0 --- verified

People

(Reporter: isabelrios, Assigned: gsvelto)

References

Details

(Keywords: regression, verifyme, Whiteboard: [cert][systemsfe])

Attachments

(5 files, 1 obsolete file)

Attached video 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.
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.
blocking-b2g: --- → 1.3?
blocking-b2g: 1.3? → 1.3+
Component: Gaia::Settings → Gaia::Wappush
David - Can you get someone assigned to look into this issue?
Flags: needinfo?(dscravaglieri)
Whiteboard: cert
Whiteboard: cert → [cert]
Gabriele, could you take a look please ?
Flags: needinfo?(dscravaglieri) → needinfo?(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: +31641600986
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
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Flags: needinfo?(gsvelto) → needinfo?(isabelrios)
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.
Flags: needinfo?(isabelrios)
Thanks José, this should do the trick.
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.
(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.
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.
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.
Component: Gaia::Wappush → Gaia::System
(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.
Blocks: 890440
Keywords: regression
Whiteboard: [cert] → [cert][systemsfe]
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
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.
Attachment #8404630 - Flags: review?(alive)
Attachment #8404630 - Attachment is obsolete: true
Attachment #8404630 - Flags: review?(alive)
Attachment #8404681 - Flags: review?(alive)
Attachment #8404630 - Attachment description: [PATCH] Fix removal of old API notifications when selected from the toaster → [PATCH WIP] Fix removal of old API notifications when selected from the toaster
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.
Attachment #8404681 - Flags: review?(alive) → review+
Thanks for the review Alive, pushed to gaia/master cca15facac7ba6a8e8654685149a84a307523691
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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
Attachment #8404681 - Flags: approval-gaia-v1.4?(fabrice)
Attachment #8404681 - Flags: approval-gaia-v1.3?(fabrice)
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
Attachment #8404681 - Flags: approval-gaia-v1.4?(release-mgmt)
Attachment #8404681 - Flags: approval-gaia-v1.4?(fabrice)
Attachment #8404681 - Flags: approval-gaia-v1.3?(release-mgmt)
Attachment #8404681 - Flags: approval-gaia-v1.3?(fabrice)
NI on :echen to help verify this on master/m-c before the branch uplifts.
Flags: needinfo?(echen)
Keywords: verifyme
I think you meant to flag Eric.
Flags: needinfo?(echen) → needinfo?(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.
Flags: needinfo?(echang)
(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.
Attachment #8404681 - Flags: approval-gaia-v1.4?(release-mgmt)
Attachment #8404681 - Flags: approval-gaia-v1.4+
Attachment #8404681 - Flags: approval-gaia-v1.3?(release-mgmt)
Attachment #8404681 - Flags: approval-gaia-v1.3+
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
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
You need to log in before you can comment on or make changes to this bug.