Closed
Bug 1088224
Opened 10 years ago
Closed 10 years ago
[Window Management] When the user taps on the new voicemail notification nothing happens
Categories
(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)
Tracking
(blocking-b2g:2.1+, b2g-v2.0 unaffected, b2g-v2.1 verified, b2g-v2.2 verified)
Tracking | Status | |
---|---|---|
b2g-v2.0 | --- | unaffected |
b2g-v2.1 | --- | verified |
b2g-v2.2 | --- | verified |
People
(Reporter: KTucker, Assigned: gerard-majax)
References
()
Details
(Keywords: regression, Whiteboard: [2.1-exploratory-3][systemsfe])
Attachments
(9 files, 8 obsolete files)
73.66 KB,
text/plain
|
Details | |
46 bytes,
text/x-github-pull-request
|
Details | Review | |
3.35 KB,
text/x-log
|
Details | |
236.04 KB,
text/plain
|
Details | |
1.69 KB,
application/zip
|
Details | |
9.17 KB,
patch
|
mikehenrty
:
review+
fabrice
:
approval-mozilla-b2g34-
|
Details | Diff | Splinter Review |
564 bytes,
patch
|
Details | Diff | Splinter Review | |
10.06 KB,
patch
|
mikehenrty
:
review+
bajaj
:
approval-mozilla-b2g34+
|
Details | Diff | Splinter Review |
46 bytes,
text/x-github-pull-request
|
mikehenrty
:
review+
|
Details | Review |
Description:
When the user taps on a new voicemail notification, nothing will happen. The dialer is not launched and the user's voicemail is not called.
Repro Steps:
1) Updated Flame to Build ID: 20141023001201
2) Insert a SIM card that has a voicemail.
3) Power on the dut and wait for the new voicemail notification.
4) Pull down the status bar and tap on the new voicemail notification.
Actual:
Nothing happens when tapping a new voicemail notification.
Expected:
Dialer is opened and the user's voicemail is automatically called.
Environmental Variables
Device: Flame 2.1 (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141023001201
Gaia: 1e48e3e40e0780c0cd07a3457e5fe2efeeb542d1
Gecko: 09fb60a37850
Gonk: 05aa7b98d3f891b334031dc710d48d0d6b82ec1d
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
Notes:
Repro frequency: 100%
See attached: Logcat, Video
Youtube: http://youtu.be/GK_fdbjQobk
Reporter | ||
Comment 1•10 years ago
|
||
This issue also occurs on the Flame 2.2(319mb)(KK)(Full Flash)
Nothing happens when the user taps on the new voicemail notification.
Flame 2.2
Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141023040204
Gaia: 27a1d1baaa8e375b70e043efee67d5f2206c330b
Gecko: 88adcf8fef83
Gonk: 05aa7b98d3f891b334031dc710d48d0d6b82ec1d
Version: 36.0a1 (2.2 Master)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0
-------------------------------------------------------------------------------------
This issue does not occur on the Flame 2.0(319mb)(KK)(Full Flash)
The user's voicemail is called automatically when they tap on the new voicemail notification.
Flame 2.0
Device: Flame 2.0 (319mb)(Kitkat Base)(Full Flash)
Build ID: 20141023001201
Gaia: 1e48e3e40e0780c0cd07a3457e5fe2efeeb542d1
Gecko: 09fb60a37850
Version: 34.0 (2.1)
Firmware Version: v188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Reporter | ||
Comment 2•10 years ago
|
||
Comment 3•10 years ago
|
||
[Blocking Requested - why for this release]: Regression in a major feature, broken expected/basic functionality
blocking-b2g: --- → 2.1?
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
Keywords: regressionwindow-wanted
Updated•10 years ago
|
QA Contact: aalldredge
Comment 4•10 years ago
|
||
----------------------------------------------------
B2G-Inbound Regression Window (Shallow Flash)
----------------------------------------------------
Last Working:
Device: Flame 2.1
BuildID: 20140725123906
Gaia: dd42afb7d6d66a6c2bd999692fec6b6d553d23de
Gecko: 7c5efe8157e0
Version: 34.0a1 (2.1)
Firmware: V123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
First Broken:
Device: Flame 2.1
BuildID: 20140725135209
Gaia: 58242f4d34493a412907663513d935c1ddf19fd5
Gecko: f5fb3dffd8bb
Version: 34.0a1 (2.1)
Firmware: V123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
Last Working Gaia First Broken Gecko: Issue does NOT reproduce
Gaia: dd42afb7d6d66a6c2bd999692fec6b6d553d23de
Gecko: f5fb3dffd8bb
First Broken Gaia Last Working Gecko: Issue DOES reproduce
Gaia: 58242f4d34493a412907663513d935c1ddf19fd5
Gecko: 7c5efe8157e0
Pushlog:
https://github.com/mozilla-b2g/gaia/compare/dd42afb7d6d66a6c2bd999692fec6b6d553d23de...58242f4d34493a412907663513d935c1ddf19fd5
Caused by Bug 1041383
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Keywords: regressionwindow-wanted
Comment 5•10 years ago
|
||
Broken by Bug 1041383 ? Can you take a look Chris
Blocks: 1041383
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell) → needinfo?(chrislord.net)
QA Contact: aalldredge
Comment 6•10 years ago
|
||
Functional regression. We do have a long-standing issue with voicemail notifications where the number is not set on the SIM and tapping on the notification does nothing.
blocking-b2g: 2.1? → 2.1+
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Gregor Wagner [:gwagner] from comment #6)
> Functional regression. We do have a long-standing issue with voicemail
> notifications where the number is not set on the SIM and tapping on the
> notification does nothing.
We are suppposed to show the user a UI if he has no voicemail number available at all, in bug 983459.
Assignee | ||
Comment 8•10 years ago
|
||
I don't reproduce any issue on my Nexus S with Free Mobile SIM card: tapping on voicemail Notification is properly placing a call to my voicemail.
Assignee | ||
Comment 9•10 years ago
|
||
Please make sure your SIM card has a MBDN value and/or that you have set a Voicemail number in settings.
If there is no voicemail number set in settings and your SIM has no MBDN, we should display a dialog to get your to Voicemail Settings.
If there is a voicemail number, or your SIM card has MBDN, then we should call.
Besides, the provided logcat has nothing usable.
Flags: needinfo?(aalldredge)
Assignee | ||
Comment 10•10 years ago
|
||
Maybe a regression from bug 833229 ?
Updated•10 years ago
|
Target Milestone: --- → 2.1 S8 (7Nov)
Updated•10 years ago
|
Assignee: nobody → chrislord.net
Comment 11•10 years ago
|
||
I think the regression window may be false - bug 1041383 broke clicking on *all* notifications, and was subsequently fixed by bug 1044406. Taking this into account, does bug 1041383 still come up as the cause of this?
Flags: needinfo?(chrislord.net)
Comment 12•10 years ago
|
||
There is a voice mail number set automatically. But pressing the voice mail notification in the utility tray does not call it.
Flags: needinfo?(aalldredge)
Assignee | ||
Comment 13•10 years ago
|
||
(In reply to Adam Alldredge [:AdamA] from comment #12)
> There is a voice mail number set automatically. But pressing the voice mail
> notification in the utility tray does not call it.
Can you elaborate ? """automatically""" ? This would mean the either we know a voicemail for your provider, or we read it from SIM card.
Either way, this clearly does not match my experience on current master.
Comment 14•10 years ago
|
||
After seeing comment 11 I rechecked the issue around the time that bug 104406 was fixed. The voice mail notification was working after that issue was fixed so the window I initially posted was incorrect. I am obtaining the correct window right now.
---------------------------------------
In response to comment 13.
I flashed the build went into settings and there was a number set under the voice mail section in "Call settings".
Comment 15•10 years ago
|
||
So the regression-window stays the same even after testing a build that includes the patch from bug 1044406?
Comment 16•10 years ago
|
||
I'm not sure what to do about this bug right now - there's conflicting information as to whether or not this actually happens here, and no regression window - n?AdamA for the latter. (I assume this is happening, but as this is assigned to me I'd just like to keep track of it so it doesn't get forgotten (by me))
Flags: needinfo?(aalldredge)
Comment 17•10 years ago
|
||
Hi Chris -
sorry for any confusion going on - Just to let you know - Adam is still working on the new regression-window as per comment 14. I believe comment 15 simply overlooked comment 14 and added to the chaos.
Flags: needinfo?(aalldredge)
Comment 18•10 years ago
|
||
I was unable to obtain a regression window for this issue. The repro rate drops significantly as you go back to earlier builds. The repro rate is based on flashing the build. When you flash the build and this issue is occurring it will happen 100% of the time for that flash. But if the exact same build is flashed again the issue may not occur. I only observed the repro changing after flashing. The repro rate became less than 1 out of 10 in the earliest builds I was seeing it in.
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Comment 19•10 years ago
|
||
Chris -
it seems that we will not be able to get a regression window for this issue. Please see comment 18. While the repro rate is listed as 100% the further back you go the worse it gets until it gets to a point where getting a window is no longer viable. I had the tester spend the better part of 2 days attempting it anyway but everytime he double checks his swaps he finds anomalies indicating that the window is not correct.
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
Comment 20•10 years ago
|
||
I'll see about writing a patch that adds logging and perhaps we can test with that. Pretty certain that bug 1041383 has nothing to do with this though, it sounds like a pretty rare, perhaps hardware-specific bug (we can probably work around it, I imagine).
n?myself so I don't forget.
Flags: needinfo?(chrislord.net)
Comment 21•10 years ago
|
||
ok, so I've got a problem trying to reproduce this in that the SIM I have doesn't give me voicemail notifications the usual way, it sends an SMS message (which works fine).
Is there another way of reproducing this? Do we have a way of inserting synthetic voicemail notifications, and would that be adequate to test this?
Flags: needinfo?(chrislord.net) → needinfo?(ktucker)
Reporter | ||
Comment 22•10 years ago
|
||
My voicemail notifications always appear in my notifications tray. I have not seen a voicemail come through on SMS. I'm using an AT&T SIM. I do not know how to create this scenario synthetically.
Marcia, can you please look at this?
Flags: needinfo?(ktucker) → needinfo?(mozillamarcia.knous)
Comment 23•10 years ago
|
||
(In reply to KTucker [:KTucker] from comment #22)
> My voicemail notifications always appear in my notifications tray. I have
> not seen a voicemail come through on SMS. I'm using an AT&T SIM. I do not
> know how to create this scenario synthetically.
>
> Marcia, can you please look at this?
Adding stephend - we just discussed a service that they are using in their automation that may be of use here.
Flags: needinfo?(mozillamarcia.knous) → needinfo?(stephen.donner)
(In reply to Marcia Knous [:marcia - use needinfo] from comment #23)
> (In reply to KTucker [:KTucker] from comment #22)
> > My voicemail notifications always appear in my notifications tray. I have
> > not seen a voicemail come through on SMS. I'm using an AT&T SIM. I do not
> > know how to create this scenario synthetically.
> >
> > Marcia, can you please look at this?
>
> Adding stephend - we just discussed a service that they are using in their
> automation that may be of use here.
It's not under my team's purview any longer, but looks like the Python tests are still using https://www.plivo.com to make/receive calls: https://github.com/mozilla-b2g/gaia/search?utf8=%E2%9C%93&q=plivo
Flags: needinfo?(stephen.donner)
Updated•10 years ago
|
Target Milestone: 2.1 S8 (7Nov) → 2.1 S9 (21Nov)
Comment 25•10 years ago
|
||
Borrowed gmarty's SIM - I don't reproduce this on master, will try flashing 2.1 and doing the same now.
Comment 26•10 years ago
|
||
adding ni for qa to try testing this again once we have VM set up on our SIM cards.
Flags: needinfo?(mozillamarcia.knous)
Comment 27•10 years ago
|
||
I can't reproduce this on master or 2.1 - in both cases, when I have no voicemail number set and I tap on the voicemail notification, a dialog pops up (instantly, with no transition :() saying 'Voicemail number is unavailable - Unable to call the voicemail. Set up the voicemail number to place the call.'
So there are two other bugs (lack of transition, bad grammar), but I can't reproduce this one.
Flags: needinfo?(mhenretty)
Assignee | ||
Comment 28•10 years ago
|
||
I'll provide a logging patch, since nobody on our side is able to reproduce.
Assignee | ||
Comment 29•10 years ago
|
||
Please apply this PR locally and record logcat while reproducing, so that we can get logging of what happens.
Flags: needinfo?(ktucker)
Flags: needinfo?(aalldredge)
Comment 30•10 years ago
|
||
Here's the log from a notification that I had from earlier, which worked correctly. The gap is before I tapped the notification.
Things of note: The notification was pre-existing rather than new, I had only just installed 2.1 with reset-gaia and I removed the voicemail number in that same session (as resetting brings it back from the SIM).
Comment 31•10 years ago
|
||
Attached logcat of issue occurring on Flame 2.1 with patch applied. Issue occurs as described in Comment 0.
Device: Flame 2.1 (319mb)(Kitkat Base)(Shallow Flash)
BuildID: 20141111001201
Gaia: cf49dfa0cb78a4c7adcd351a8650d3686cf083c7
Gecko: 452db1a0c9a0
Version: 34.0 (2.1)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
Flags: needinfo?(ktucker)
Flags: needinfo?(aalldredge)
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Updated•10 years ago
|
Flags: needinfo?(ktucker)
Assignee | ||
Comment 32•10 years ago
|
||
There's no trace of the click event being triggered ...
Updated•10 years ago
|
Flags: needinfo?(mozillamarcia.knous)
Assignee | ||
Comment 33•10 years ago
|
||
Well, sorry for asking a dumb question, but can you try to play with the Notification API ?
Specifically, flashing an eng build and playing with the "UI Tests" application, triggering notifications and tapping on them.
Because according to the log, the code behaves as expected and there is no trace of getting into the notification click handler.
Flags: needinfo?(bzumwalt)
Flags: needinfo?(aalldredge)
Comment 34•10 years ago
|
||
Results of tests on Engineering build:
Further testing reveals that issue is not initially occurring 100% of the time. For the times where the issue did not reproduce, performing 1 to 2 factory reset causes the issue to reproduce. Once the issue reproduces it will do so 100% of the time.
Video comparison of tapping voicemail notification versus tapping generated notification from UI Test application: http://youtu.be/sqEP9YVGQRY
Attached Zip file containing logcats of both attempting to tap voicemail notification (logNoteVoicemail.txt) and tapping notification generated from UI Test application (logNoteGen.txt)
Flags: needinfo?(bzumwalt)
Updated•10 years ago
|
Flags: needinfo?(aalldredge)
Reporter | ||
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Comment 35•10 years ago
|
||
I can reproduce on 2.1. You need to restart the phone once or twice to see it happen because it is a race condition between a resent notification and the new notification created by the voicemail statuschange event. What's happening is this:
1.) Phone starts booting, and voicemail.js listens for the statuschange voicemail event.
2.) This statuschange event fires, and voicemail.js creates a new notification and attaches the proper click handler on it.
3.) notifications.js receives the resent notification from the last time the phone was booted and creates a new notification with the same tag, thereby overriding the click handler setup in voicemail.js.
4.) Now when anyone clicks on this resent notification, there are no handlers and so it does nothing.
I know we clean up any old notifications in voicemail.js before creating the new notification, but unfortunately the system is already resending this old notification, and it can arrive after the new proper notification was created.
I think Alex might be the best guy to handle this.
Assignee: chrislord.net → lissyx+mozillians
Flags: needinfo?(mhenretty)
Assignee | ||
Comment 36•10 years ago
|
||
That would make sense, but:
- in setupNotification(), we attach the statuschanged event AFTER we cleaned previous notifications
- according to attachment 8520830 [details], we did properly close the notification before adding the event
- in attachment 8520830 [details] we clearly see that a new notification is created, triggered by the statuschanged event
So it would mean that the race is indeed that we already added a nice new notification object and that this gets overwritten by the resent one?
Flags: needinfo?(mhenretty)
Assignee | ||
Comment 37•10 years ago
|
||
I've pushed to the PR to get more debugging, and assert what michael suggested.
Assignee | ||
Comment 38•10 years ago
|
||
This Gecko change should make sure we do not consider resent notification when we already have a notification with the same id, and that has been NOT resent.
Do you mind giving a try? This should be applied on master.
Assignee | ||
Comment 39•10 years ago
|
||
(In reply to Alexandre LISSY :gerard-majax from comment #38)
> Created attachment 8521383 [details] [diff] [review]
> notifications-resend.diff
>
> This Gecko change should make sure we do not consider resent notification
> when we already have a notification with the same id, and that has been NOT
> resent.
>
> Do you mind giving a try? This should be applied on master.
At least I did a small check with this change:
- hacking gaia system app to delay by 1min the mozChromeNotifications use
- boot device
- shake => one notification shown, I can tap and the click handler works
- wait for the mozChromeNotifications to do its job
- "old" notification is resent
Now, tapping behavior depends on whether we early return in the proposed patch:
- with the early return, we properly trigger the click handler: the resent notification has not been used
- without the early return, nothing happens when we tap the notification
Assignee | ||
Comment 40•10 years ago
|
||
Let's see what breaks: https://tbpl.mozilla.org/?tree=Try&rev=99dfdbd78509
Assignee | ||
Comment 41•10 years ago
|
||
(In reply to Alexandre LISSY :gerard-majax from comment #40)
> Let's see what breaks: https://tbpl.mozilla.org/?tree=Try&rev=99dfdbd78509
Mostly green. Michael, would you mind making sure this does not break any gaia test ? My try was supposed to trigger them, but it looks like not.
Assignee | ||
Comment 42•10 years ago
|
||
(In reply to Alexandre LISSY :gerard-majax from comment #41)
> (In reply to Alexandre LISSY :gerard-majax from comment #40)
> > Let's see what breaks: https://tbpl.mozilla.org/?tree=Try&rev=99dfdbd78509
>
> Mostly green. Michael, would you mind making sure this does not break any
> gaia test ? My try was supposed to trigger them, but it looks like not.
Retrigger were green, but I missed B2G Desktop Linux 64 bits, so no Gaia test were run.
This new try should be good: https://tbpl.mozilla.org/?tree=Try&rev=28ee6308ae6c
Comment 43•10 years ago
|
||
Ahhh, I couldn't get b2g to build today (I recently upgraded to Yosemite and of course everything broke). The idea seems reasonable though, and I will look at this first thing tomorrow. Leaving ni? for now.
Assignee | ||
Comment 44•10 years ago
|
||
There we go: https://tbpl.mozilla.org/?tree=Try&rev=699f9ff33a96
Gij 3 failure for testing notification resending.
Comment 45•10 years ago
|
||
Yup this definitely fixes it. I only see 1 voicemail notification coming in on reboot, and I see this in the log:
I/Gecko ( 924): =*= AlertsService.js : showAppNotification: uid=app://system.gaiamobile.org/manifest.webapp#tag:voicemailNotification:1
I/Gecko ( 924): =*= AlertsService.js : showAppNotification: uid=app://system.gaiamobile.org/manifest.webapp#tag:voicemailNotification:1
I/Gecko ( 924): =*= AlertsService.js : showAppNotification: uid=app://system.gaiamobile.org/manifest.webapp#tag:voicemailNotification:1: aDetails.resent=true
I/Gecko ( 924): =*= AlertsService.js : showAppNotification: uid=app://system.gaiamobile.org/manifest.webapp#tag:voicemailNotification:1: this._listeners[uid].resent=undefined
I/Gecko ( 924): =*= AlertsService.js : showAppNotification: uid=app://system.gaiamobile.org/manifest.webapp#tag:voicemailNotification:1 already registered, and overwriting a non resent
I'm looking into the integration test failures now.
Flags: needinfo?(mhenretty)
Comment 46•10 years ago
|
||
The notification_events_test.js test is indeed failing, and for good reason since we now block resending of existing 'live' notifications. But this problem uncovered a bug. When we call performResend in ChromeNotifications.js, we have to make sure to not increment our resend count when appNotifier.showAppNotification gets skipped.
1.) http://hg.mozilla.org/mozilla-central/annotate/9712d6370c3f/dom/notification/ChromeNotifications.js#l74
Comment 47•10 years ago
|
||
Ahhh in comment 46 I meant to say notification_resend_test.js NOT notification_events_test.js.
However, notification_events_test.js has a similar problem. We can no longer simply call mozResendAllNotifications and expect our live notifications to get resent. The reason is that the notification observer cannot already be in the Alert Service's observer list (but instead only in the DB), otherwise we will now block the resend. I'm afraid we are going to need to find a new way to test resending notifications in gaia.
Assignee | ||
Comment 48•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8521383 -
Attachment is obsolete: true
Assignee | ||
Comment 49•10 years ago
|
||
(In reply to Michael Henretty [:mhenretty] from comment #46)
> The notification_events_test.js test is indeed failing, and for good reason
> since we now block resending of existing 'live' notifications. But this
> problem uncovered a bug. When we call performResend in
> ChromeNotifications.js, we have to make sure to not increment our resend
> count when appNotifier.showAppNotification gets skipped.
>
> 1.)
> http://hg.mozilla.org/mozilla-central/annotate/9712d6370c3f/dom/notification/
> ChromeNotifications.js#l74
This should be done in attachment 8522892 [details] [diff] [review]: we have to change the definition of showAppNotification. Making it return a boolean to notify us if it failed.
Matching try at: https://tbpl.mozilla.org/?tree=Try&rev=604fd289ae21
Assignee | ||
Comment 50•10 years ago
|
||
Fixing some build errors: https://tbpl.mozilla.org/?tree=Try&rev=545682278d63
Assignee | ||
Comment 51•10 years ago
|
||
(In reply to Alexandre LISSY :gerard-majax from comment #50)
> Fixing some build errors: https://tbpl.mozilla.org/?tree=Try&rev=545682278d63
Much failures :). One proper way to do those tests would be, I think, being able to restart Gaia after we sent the Notification.
Can we do this in Gaia JS integration tests ?
Flags: needinfo?(zcampbell)
Comment 52•10 years ago
|
||
(In reply to Alexandre LISSY :gerard-majax from comment #51)
> (In reply to Alexandre LISSY :gerard-majax from comment #50)
> > Fixing some build errors: https://tbpl.mozilla.org/?tree=Try&rev=545682278d63
>
> Much failures :). One proper way to do those tests would be, I think, being
> able to restart Gaia after we sent the Notification.
>
> Can we do this in Gaia JS integration tests ?
I'm not experienced enough with the JS integration tests, but you could do it in the Python ones. It would not be pretty, but you could do it.
Flags: needinfo?(zcampbell)
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(ktucker)
Assignee | ||
Comment 53•10 years ago
|
||
Michael, I think we can do something indeed. Let's define a new boolean pref, "dom.notification.mozchromenotifications.allow_resend_overwrite". This would be checked in showAppNotification(), and defaulting to false to have the new behavior want. This way, we can toggle this pref to true in our tests and those would work.
Flags: needinfo?(mhenretty)
Comment 54•10 years ago
|
||
(In reply to Alexandre LISSY :gerard-majax from comment #53)
> Michael, I think we can do something indeed. Let's define a new boolean
> pref, "dom.notification.mozchromenotifications.allow_resend_overwrite". This
> would be checked in showAppNotification(), and defaulting to false to have
> the new behavior want. This way, we can toggle this pref to true in our
> tests and those would work.
That works for me for now. I'll think about it over the weekend and try to come up with a way to do it without a test only pref, but that solution seems workable. Leaving ni? as a reminder.
Assignee | ||
Comment 55•10 years ago
|
||
Assignee | ||
Comment 56•10 years ago
|
||
Try with new patch: https://tbpl.mozilla.org/?tree=Try&rev=100968784db5
This one should produce failure on Gij 3, I just simplified a bit the code
Assignee | ||
Comment 57•10 years ago
|
||
Try with new patch and gaia-side changes: https://tbpl.mozilla.org/?tree=Try&rev=10ea57a7f775
This one should be green but without any test testing the new behavior :)
Assignee | ||
Comment 58•10 years ago
|
||
Gij 3 behavior is the expected one for now, except I see failures: "TEST-UNEXPECTED-FAIL | apps/system/test/marionette/lockscreen_notification_test.js | LockScreen notification tests system replace notification".
I had a look at the code and we are indeed not replacing the notification (according to the screenshot being taken). I'm not sure, however, if we are exposing an issue here or it's just racy code: LockScreenNotificationActions' fireNotification() method returns directly, so we may return while the new notification has not been yet made visible to the user.
Plus, this is not failures that I used to see in past try attempts, I only spotted it on recent ones.
Flags: needinfo?(gweng)
Comment 59•10 years ago
|
||
> I had a look at the code and we are indeed not replacing the notification (according to the screenshot being taken).
So you mean the symptom you captured is according to the screenshot of failed test, but not the code (yet)?
> I'm not sure, however, if we are exposing an issue here or it's just racy code: LockScreenNotificationActions' fireNotification() method returns directly, so we may return while the new notification has not been yet made visible to the user.
If this is the root cause, I think 'fireNotification' could have some waiting loop via Marionette's 'waitFor' method to wait the notification is visible from LockScreen's DOM tree (#notifications-lockscreen-container), and only when this condition is satisfied the function returns. In this way at least we can reduce the possible root causes here.
Flags: needinfo?(gweng)
Assignee | ||
Comment 60•10 years ago
|
||
Ok, I sent a Try where I remove the |return false| statement. Yet, Gij 3 is still failing: https://tbpl.mozilla.org/?tree=Try&rev=f7028cc89ae3
Flags: needinfo?(gweng)
Comment 61•10 years ago
|
||
Which statement? Are we talking about the same code here? I thought your diagnose (Comment 58) is for
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/marionette/lib/lockscreen_notification_actions.js#L54
Flags: needinfo?(gweng)
Assignee | ||
Comment 62•10 years ago
|
||
(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #61)
> Which statement? Are we talking about the same code here? I thought your
> diagnose (Comment 58) is for
>
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/marionette/
> lib/lockscreen_notification_actions.js#L54
Nevermind, running locally I could see that the Services.prefs.getBoolPref() call throws.
Assignee | ||
Comment 63•10 years ago
|
||
Assignee | ||
Comment 64•10 years ago
|
||
Pushed Gaia with a new test:
- try with all fixes: https://tbpl.mozilla.org/?tree=Try&rev=42a9aff87963
- try without fix: https://tbpl.mozilla.org/?tree=Try&rev=fdf4a8842a11
Assignee | ||
Comment 65•10 years ago
|
||
Assignee | ||
Comment 66•10 years ago
|
||
(In reply to Alexandre LISSY :gerard-majax from comment #64)
> Pushed Gaia with a new test:
> - try with all fixes: https://tbpl.mozilla.org/?tree=Try&rev=42a9aff87963
> - try without fix: https://tbpl.mozilla.org/?tree=Try&rev=fdf4a8842a11
Forgot a not in the condition :). New try, which is green locally:
- with fix: https://tbpl.mozilla.org/?tree=Try&rev=4ce6acf2ba9e
- without fix: https://tbpl.mozilla.org/?tree=Try&rev=22a8517371a5
Assignee | ||
Comment 67•10 years ago
|
||
(In reply to Alexandre LISSY :gerard-majax from comment #66)
> (In reply to Alexandre LISSY :gerard-majax from comment #64)
> > Pushed Gaia with a new test:
> > - try with all fixes: https://tbpl.mozilla.org/?tree=Try&rev=42a9aff87963
> > - try without fix: https://tbpl.mozilla.org/?tree=Try&rev=fdf4a8842a11
>
> Forgot a not in the condition :). New try, which is green locally:
> - with fix: https://tbpl.mozilla.org/?tree=Try&rev=4ce6acf2ba9e
> - without fix: https://tbpl.mozilla.org/?tree=Try&rev=22a8517371a5
This one does the job and we have the expected green/red.
Assignee | ||
Updated•10 years ago
|
Attachment #8522892 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8523527 -
Attachment is obsolete: true
Assignee | ||
Comment 68•10 years ago
|
||
Comment on attachment 8523860 [details] [diff] [review]
Priority to the new notifications
Michael, the approach seems to be good, regarding the try above. If you have a better idea, I'm all ears :)
Attachment #8523860 -
Flags: feedback?(mhenretty)
Comment 69•10 years ago
|
||
Comment on attachment 8523860 [details] [diff] [review]
Priority to the new notifications
Review of attachment 8523860 [details] [diff] [review]:
-----------------------------------------------------------------
In regards to adding a pref to test this, it is a little unfortunate we cannot manually restart the b2g process in marionette js tests. I filed bug 1100684 to fix that. In the meantime, I can't think of a better way to test resending notifications with this fix. So, fb+.
That said, I think we should land this change in 2.1 only. To fix this in 2.2, a better approach would be to add a mozbehavior that gives us the ability to specify that certain notifications should not be resent. So, system app could do something like:
var voicemail = new Notification('(2) voicemails', {
mozbehavior: {
noresend: true
}
});
That way, we could avoid this workaround. What do you think?
::: b2g/components/AlertsService.js
@@ +98,5 @@
> let dataObj = this.deserializeStructuredClone(aDetails.data);
> + debug("showAppNotification: uid=" + uid);
> +
> + if (this._listeners[uid]) {
> + let currentIsResend = aAlertListener === null;
I liked it better when we manually added the .resent property to notifications being resent. The reason is for future proofing; ie. if we ever change the way showAppNotification is called from C++, or we add callers, we have to make sure they honor the contract of always passing in an observer when not being resent. I think this is not obvious. Being explicit by appending .resent seems better to me.
@@ +105,5 @@
> + debug("showAppNotification: uid=" + uid + ": oldIsResend=" + oldIsResend);
> + let allow_resend_overwrite = false;
> + try {
> + allow_resend_overwrite = Services.prefs.getBoolPref(
> + "dom.notifications.mozchromenotifications.allow_resend_overwrite");
Do we want to cache this property and listen for changes rather than reading it every time an app sends a notification? Maybe Services.prefs does this for us?
@@ +116,5 @@
> + /**
> + * If the current notification is a resent one and the previously recorded one
> + * was not a resent, let's check if we are allowed to overwrite
> + **/
> + if (currentIsResend && !oldIsResend && denyOverwrite) {
In regards to the `!oldIsResend`, why do we want to avoid resending multiple times?
@@ +118,5 @@
> + * was not a resent, let's check if we are allowed to overwrite
> + **/
> + if (currentIsResend && !oldIsResend && denyOverwrite) {
> + debug("showAppNotification: uid=" + uid + " already registered, and overwriting a non resent");
> + Cu.reportError("Notification " + uid + ": overwriting tentative by resend.");
Is this actually an error, or an expected path?
Attachment #8523860 -
Flags: feedback?(mhenretty) → feedback+
Comment 70•10 years ago
|
||
(In reply to Michael Henretty [:mhenretty] from comment #69)
> That said, I think we should land this change in 2.1 only. To fix this in
> 2.2, a better approach would be to add a mozbehavior that gives us the
> ability to specify that certain notifications should not be resent. So,
> system app could do something like:
>
> var voicemail = new Notification('(2) voicemails', {
> mozbehavior: {
> noresend: true
> }
> });
>
> That way, we could avoid this workaround. What do you think?
Flags: needinfo?(mhenretty) → needinfo?(lissyx+mozillians)
Assignee | ||
Comment 71•10 years ago
|
||
(In reply to Michael Henretty [:mhenretty] from comment #69)
> Comment on attachment 8523860 [details] [diff] [review]
> Priority to the new notifications
>
> Review of attachment 8523860 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> In regards to adding a pref to test this, it is a little unfortunate we
> cannot manually restart the b2g process in marionette js tests. I filed bug
> 1100684 to fix that. In the meantime, I can't think of a better way to test
> resending notifications with this fix. So, fb+.
>
> That said, I think we should land this change in 2.1 only. To fix this in
> 2.2, a better approach would be to add a mozbehavior that gives us the
> ability to specify that certain notifications should not be resent. So,
> system app could do something like:
>
> var voicemail = new Notification('(2) voicemails', {
> mozbehavior: {
> noresend: true
> }
> });
>
> That way, we could avoid this workaround. What do you think?
100% agree.
>
> ::: b2g/components/AlertsService.js
> @@ +98,5 @@
> > let dataObj = this.deserializeStructuredClone(aDetails.data);
> > + debug("showAppNotification: uid=" + uid);
> > +
> > + if (this._listeners[uid]) {
> > + let currentIsResend = aAlertListener === null;
>
> I liked it better when we manually added the .resent property to
> notifications being resent. The reason is for future proofing; ie. if we
> ever change the way showAppNotification is called from C++, or we add
> callers, we have to make sure they honor the contract of always passing in
> an observer when not being resent. I think this is not obvious. Being
> explicit by appending .resent seems better to me.
Makes sense, done.
>
> @@ +105,5 @@
> > + debug("showAppNotification: uid=" + uid + ": oldIsResend=" + oldIsResend);
> > + let allow_resend_overwrite = false;
> > + try {
> > + allow_resend_overwrite = Services.prefs.getBoolPref(
> > + "dom.notifications.mozchromenotifications.allow_resend_overwrite");
>
> Do we want to cache this property and listen for changes rather than reading
> it every time an app sends a notification? Maybe Services.prefs does this
> for us?
Looking at this.
>
> @@ +116,5 @@
> > + /**
> > + * If the current notification is a resent one and the previously recorded one
> > + * was not a resent, let's check if we are allowed to overwrite
> > + **/
> > + if (currentIsResend && !oldIsResend && denyOverwrite) {
>
> In regards to the `!oldIsResend`, why do we want to avoid resending multiple
> times?
I don't think it's a problem that we do resend multiple times.
>
> @@ +118,5 @@
> > + * was not a resent, let's check if we are allowed to overwrite
> > + **/
> > + if (currentIsResend && !oldIsResend && denyOverwrite) {
> > + debug("showAppNotification: uid=" + uid + " already registered, and overwriting a non resent");
> > + Cu.reportError("Notification " + uid + ": overwriting tentative by resend.");
>
> Is this actually an error, or an expected path?
I'd keep it an error, because this is not something we want, so having an error popping could help noticing bad cases.
Assignee | ||
Comment 72•10 years ago
|
||
Assignee | ||
Comment 73•10 years ago
|
||
Flags: needinfo?(lissyx+mozillians)
Assignee | ||
Updated•10 years ago
|
Attachment #8523860 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8524720 -
Attachment is obsolete: true
Assignee | ||
Comment 74•10 years ago
|
||
Assignee | ||
Comment 75•10 years ago
|
||
Assignee | ||
Comment 76•10 years ago
|
||
Try on master: https://tbpl.mozilla.org/?tree=Try&rev=b92228db7805
Assignee | ||
Comment 77•10 years ago
|
||
Comment on attachment 8524732 [details] [diff] [review]
Priority to the new notifications r=mhenretty
Cherry pick of the master patch, with just the conflicts solved. I cannot trigger a try run for this one, so if you can do it for me it would be wonderful. Do not forget to apply attachment 8524733 [details] [diff] [review] so that the proper Gaia branch is used.
Attachment #8524732 -
Flags: review?(mhenretty)
Comment 78•10 years ago
|
||
Comment on attachment 8524732 [details] [diff] [review]
Priority to the new notifications r=mhenretty
Review of attachment 8524732 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good! Tried to trigger a try with this:
https://tbpl.mozilla.org/?tree=Try&rev=b964f7d10e67
Attachment #8524732 -
Flags: review?(mhenretty) → review+
Assignee | ||
Comment 79•10 years ago
|
||
This bug will be only about fixing on v2.1, a different patch is needed for master and will be proposed in bug 1100876.
Assignee | ||
Comment 80•10 years ago
|
||
(In reply to Michael Henretty [:mhenretty] from comment #78)
[...]
> Looks good! Tried to trigger a try with this:
>
> https://tbpl.mozilla.org/?tree=Try&rev=b964f7d10e67
I also could send a try myself https://tbpl.mozilla.org/?tree=Try&rev=29ec525c0299
It's perma red but it smells not related to my patch ; also, looking on tbpl, I see other try against v2.1 with the same behavior :(
Dale, are you aware of any breakage on, v2.1?
Flags: needinfo?(dale)
Assignee | ||
Comment 81•10 years ago
|
||
Looking at other v2.1 Gij and Gip, it's preexisting condition. I just forgot to r? the gaia part ...
Flags: needinfo?(dale)
Flags: needinfo?(bzumwalt)
Flags: needinfo?(aalldredge)
Assignee | ||
Comment 82•10 years ago
|
||
Attachment #8525390 -
Flags: review?(mhenretty)
Comment 83•10 years ago
|
||
Comment on attachment 8525390 [details] [review]
Gaia PR
r=me for the test changes and new test. However, the new test fails locally and on tbpl due to a startup race condition. I have that fixed and will submit a new pr shortly.
Attachment #8525390 -
Flags: review?(mhenretty) → review+
Comment 84•10 years ago
|
||
r+ carries. got irc-a=fabrice for landing this before the gecko part. waiting on tbpl now to make sure this doesn't burn anything.
Attachment #8525390 -
Attachment is obsolete: true
Attachment #8525641 -
Flags: review+
Comment 85•10 years ago
|
||
Comment on attachment 8524732 [details] [diff] [review]
Priority to the new notifications r=mhenretty
Bug caused by (feature/regressing bug #):
Not a regression, see comment 11 and comment 18 for more details.
User impact if declined:
When user reboots the phone with a voicemail pending, they will be unable to click on the notification to check there voicemail. As a dogfooder, this is a common use case.
Testing completed:
Manual on device testing. New gaia integration test (which must be enabled after this patch).
Risk to taking this patch (and alternatives if risky):
Moderately risky, as this affects the notification code path in gecko. But we took pains to make sure the changes only affect resent notifications. Unfortunately, there are no alternative solutions we could think of for fixing the voicemail issue.
String or UUID changes made by this patch:
None.
Attachment #8524732 -
Flags: approval-mozilla-b2g34?
Comment 86•10 years ago
|
||
Comment on attachment 8524732 [details] [diff] [review]
Priority to the new notifications r=mhenretty
Review of attachment 8524732 [details] [diff] [review]:
-----------------------------------------------------------------
a- until the DesktopNotification.cpp change is clarified.
::: dom/notification/DesktopNotification.cpp
@@ +88,5 @@
>
> + bool rv;
> + appNotifier->ShowAppNotification(mIconURL, mTitle, mDescription,
> + mObserver, val, &rv);
> + return NS_OK;
Why are you returning NS_OK here? What if ShwoAppNotification() throws?
Attachment #8524732 -
Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34-
Assignee | ||
Comment 87•10 years ago
|
||
Assignee | ||
Comment 88•10 years ago
|
||
(In reply to Alexandre LISSY :gerard-majax from comment #87)
> Created attachment 8525910 [details] [diff] [review]
> Priority to the new notifications r=mhenretty
This should address fabrice's comment.
Try: https://tbpl.mozilla.org/?tree=Try&rev=b10d88265c6c
Assignee | ||
Comment 89•10 years ago
|
||
PR updated to integrate michael's fixes. I've also noticed one small thing that may make test fail.
Assignee | ||
Comment 90•10 years ago
|
||
New try: https://tbpl.mozilla.org/?tree=Try&rev=537332aff93c
This one is with fix from attachment 8525910 [details] [diff] [review], and updated PR.
Assignee | ||
Comment 91•10 years ago
|
||
Michael, I've updated my PR, integrating your fix. I also changed |container.removeChild(nodes[0])| to |nodes[0].remove()|. With this, I get green Gij when run locally against my b2g desktop build. If I disable the Gecko-side fix, then the new test is failing as expected.
Once try https://tbpl.mozilla.org/?tree=Try&rev=537332aff93c is green, we should merge the PR with the test disabled until Gecko-part lands.
Attachment #8525641 -
Attachment is obsolete: true
Attachment #8525955 -
Flags: review?(mhenretty)
Assignee | ||
Comment 92•10 years ago
|
||
Comment on attachment 8525910 [details] [diff] [review]
Priority to the new notifications r=mhenretty
Michael, just another round to make sure we are extra cautious here.
Attachment #8525910 -
Flags: review?(mhenretty)
Comment 93•10 years ago
|
||
Comment on attachment 8525910 [details] [diff] [review]
Priority to the new notifications r=mhenretty
Review of attachment 8525910 [details] [diff] [review]:
-----------------------------------------------------------------
LGTM.
Attachment #8525910 -
Flags: review?(mhenretty) → review+
Comment 94•10 years ago
|
||
Comment on attachment 8525910 [details] [diff] [review]
Priority to the new notifications r=mhenretty
Bug caused by (feature/regressing bug #):
Not a regression, see comment 11 and comment 18 for more details.
User impact if declined:
When user reboots the phone with a voicemail pending, they will be unable to click on the notification to check there voicemail. As a dogfooder, this is a common use case.
Testing completed:
Manual on device testing. New gaia integration test (which must be enabled after this patch).
Risk to taking this patch (and alternatives if risky):
Moderately risky, as this affects the notification code path in gecko. But we took pains to make sure the changes only affect resent notifications. Unfortunately, there are no alternative solutions we could think of for fixing the voicemail issue.
String or UUID changes made by this patch:
None.
Attachment #8525910 -
Flags: approval-mozilla-b2g34?
Comment 95•10 years ago
|
||
Comment on attachment 8525955 [details] [review]
Updated PR
I'll submit a new PR that cherry-picks your commit, but also disables the new test. I'll then revert that 2nd commit when the gecko patch makes it to b2g-inbound.
Attachment #8525955 -
Flags: review?(mhenretty) → review+
Comment 96•10 years ago
|
||
r+ carries.
Attachment #8525955 -
Attachment is obsolete: true
Attachment #8526367 -
Flags: review+
Comment 97•10 years ago
|
||
The gip failures on gaia-try are being investigated in : https://bugzilla.mozilla.org/show_bug.cgi?id=1102560, I'll go ahead with approval, Henretty it'll help if you can sure these failures are not mirrored to b2g34-v2.1, to be safe! Thanks!
NI :pragmatic for help with testing once this lands and Kwierso for help with checking this in ob b2g34-2.1 once a+ed.
Flags: needinfo?(parul)
Flags: needinfo?(kwierso)
Updated•10 years ago
|
Keywords: checkin-needed,
verifyme
Updated•10 years ago
|
Attachment #8525910 -
Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
v2.1: https://github.com/mozilla-b2g/gaia/commit/cac575e35fb0ae753635928df542a8da5de17f21
Unsure what all the flags and resolutions should be set to with Gecko's part still missing. Leaving them unchanged until someone more involved says otherwise.
Flags: needinfo?(kwierso)
Keywords: checkin-needed
Comment 100•10 years ago
|
||
Let's leave it open until we enable the test from attachment 8526367 [details] [review].
Keywords: leave-open
Comment 101•10 years ago
|
||
Issue is fixed on Flame 2.1
Tested issue on both engineering and production build across 12 different attempts.
Production: Bug does not repro across three restarts OR across three resets
Engineering: Bug does not repro across three restarts OR across three resets
Device: Flame 2.1 (319mb)(Kitkat Base)(Shallow Flash)
BuildID: 20141121001202
Gaia: 6c739275e963465658c18c7a9ebaa48cbe927d34
Gecko: 9bfc7a166a94
Version: 34.0 (2.1)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Comment 102•10 years ago
|
||
test enabled on v2.1:
v2.1: https://github.com/mozilla-b2g/gaia/commit/d0febb8f7c52b49f4568c9c8152c98b2bcb4bea2
Updated•10 years ago
|
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Reporter | ||
Updated•10 years ago
|
Reporter | ||
Comment 103•10 years ago
|
||
Will verify again once 2.2 has been uplifted.
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(parul)
Comment 104•10 years ago
|
||
Does something need to land on trunk/master here still?
Flags: needinfo?(lissyx+mozillians)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(lissyx+mozillians) → needinfo?(mhenretty)
Comment 105•10 years ago
|
||
Nothing needs to be landed on trunk here. Bug 1100876 fixed this problem in a better way for 2.2. Please verify this on 2.2.
Flags: needinfo?(mhenretty) → needinfo?(ktucker)
Comment 106•10 years ago
|
||
Issue verified fixed on Flame 2.2
On phone start, pulling down notification tray and tapping voicemail notification opens dialer and automatically calls voicemail inbox.
Device: Flame 2.2 Master (319mb)(Kitkat Base)(Shallow Flash)
BuildID: 20150109010206
Gaia: 5f0dd37917c4a6d8fa8724715d4d3797419f9013
Gecko: b3f84cf78dc2
Version: 37.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
Reporter | ||
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Comment 107•7 years ago
|
||
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in
before you can comment on or make changes to this bug.
Description
•