KTucker [:KTucker][Inactive 3/4/2016] Reporter
	Description • 10 years ago

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

	KTucker [:KTucker][Inactive 3/4/2016] 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

	Joshua Mitchell (Inactive)
	Comment 3 • 10 years ago

[Blocking Requested - why for this release]: Regression in a major feature, broken expected/basic functionality

	Adam Alldredge [:AdamA][Inactive]
	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

	Joshua Mitchell (Inactive)
	Comment 5 • 10 years ago

Broken by Bug 1041383 ? Can you take a look Chris

	Gregor Wagner [:gwagner]
	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.

	:gerard-majax 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.

	:gerard-majax 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.

	:gerard-majax 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.

	:gerard-majax Assignee
	Comment 10 • 10 years ago

Maybe a regression from bug 833229 ?

	Chris Lord [:cwiiis]
	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?

	Adam Alldredge [:AdamA][Inactive]
	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.

	:gerard-majax 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.

	Adam Alldredge [:AdamA][Inactive]
	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".

	Gregor Wagner [:gwagner]
	Comment 15 • 10 years ago

So the regression-window stays the same even after testing a build that includes the patch from bug 1044406?

	Chris Lord [:cwiiis]
	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))

	Joshua Mitchell (Inactive)
	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.

	Adam Alldredge [:AdamA][Inactive]
	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.

	Joshua Mitchell (Inactive)
	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.

	Chris Lord [:cwiiis]
	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.

	Chris Lord [:cwiiis]
	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?

	KTucker [:KTucker][Inactive 3/4/2016] 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?

	Marcia Knous [:marcia]
	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.

	Stephen Donner [:stephend] Not actively reading bugmail
	Comment 24 • 10 years ago

(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

	Chris Lord [:cwiiis]
	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.

	Marcia Knous [:marcia]
	Comment 26 • 10 years ago

adding ni for qa to try testing this again once we have VM set up on our SIM cards.

	Chris Lord [:cwiiis]
	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.

	:gerard-majax Assignee
	Comment 28 • 10 years ago

I'll provide a logging patch, since nobody on our side is able to reproduce.

	:gerard-majax 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.

	Chris Lord [:cwiiis]
	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).

	Brogan Zumwalt [Inactive]
	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

	:gerard-majax Assignee
	Comment 32 • 10 years ago

There's no trace of the click event being triggered ...

	:gerard-majax 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.

	Brogan Zumwalt [Inactive]
	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)

	Michael Henretty [:mikehenrty][:mhenretty]
	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.

	:gerard-majax 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?

	:gerard-majax Assignee
	Comment 37 • 10 years ago

I've pushed to the PR to get more debugging, and assert what michael suggested.

	:gerard-majax 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.

	:gerard-majax 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

	:gerard-majax Assignee
	Comment 40 • 10 years ago

Let's see what breaks: https://tbpl.mozilla.org/?tree=Try&rev=99dfdbd78509

	:gerard-majax 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.

	:gerard-majax 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

	Michael Henretty [:mikehenrty][:mhenretty]
	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.

	:gerard-majax Assignee
	Comment 44 • 10 years ago

There we go: https://tbpl.mozilla.org/?tree=Try&rev=699f9ff33a96
Gij 3 failure for testing notification resending.

	Michael Henretty [:mikehenrty][:mhenretty]
	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.

	Michael Henretty [:mikehenrty][: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

	Michael Henretty [:mikehenrty][:mhenretty]
	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.

	:gerard-majax 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

	:gerard-majax Assignee
	Comment 50 • 10 years ago

Fixing some build errors: https://tbpl.mozilla.org/?tree=Try&rev=545682278d63

	:gerard-majax 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 ?

	Zac C (:zac)
	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.

	:gerard-majax 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.

	Michael Henretty [:mikehenrty][: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.

	:gerard-majax 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

	:gerard-majax 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 :)

	:gerard-majax 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.

	Greg Weng [:snowmantw][: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.

	:gerard-majax 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

	Greg Weng [:snowmantw][: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

	:gerard-majax 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.

	:gerard-majax Assignee
	Comment 63 • 10 years ago

Green: https://tbpl.mozilla.org/?tree=Try&rev=b83102486cd4

	:gerard-majax 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

	:gerard-majax 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

	:gerard-majax 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.

	:gerard-majax 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 :)

	Michael Henretty [:mikehenrty][: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?

	Michael Henretty [:mikehenrty][:mhenretty]
	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?

	:gerard-majax 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.

	:gerard-majax Assignee
	Comment 73 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=b92228db7805

	:gerard-majax Assignee
	Comment 76 • 10 years ago

Try on master: https://tbpl.mozilla.org/?tree=Try&rev=b92228db7805

	:gerard-majax 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.

	Michael Henretty [:mikehenrty][: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

	:gerard-majax 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.

	:gerard-majax 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?

	:gerard-majax 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 ...

	Michael Henretty [:mikehenrty][: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.

	Michael Henretty [:mikehenrty][:mhenretty]
	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.

	Michael Henretty [:mikehenrty][:mhenretty]
	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.

	[:fabrice] Fabrice Desré
	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?

	:gerard-majax 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

	:gerard-majax 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.

	:gerard-majax 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.

	:gerard-majax 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.

	:gerard-majax 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.

	Michael Henretty [:mikehenrty][: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.

	Michael Henretty [:mikehenrty][:mhenretty]
	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.

	Michael Henretty [:mikehenrty][:mhenretty]
	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.

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 96 • 10 years ago

r+ carries.

	bhavana bajaj [:bajaj]
	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.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 98 • 10 years ago

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.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 99 • 10 years ago

b2g34: https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/ef18d4563bc2

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 100 • 10 years ago

Let's leave it open until we enable the test from attachment 8526367 [details] [review].

	Brogan Zumwalt [Inactive]
	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

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 102 • 10 years ago

test enabled on v2.1:

v2.1: https://github.com/mozilla-b2g/gaia/commit/d0febb8f7c52b49f4568c9c8152c98b2bcb4bea2

	KTucker [:KTucker][Inactive 3/4/2016] Reporter
	Comment 103 • 10 years ago

Will verify again once 2.2 has been uplifted.

	Ryan VanderMeulen [:RyanVM]
	Comment 104 • 9 years ago

Does something need to land on trunk/master here still?

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 105 • 9 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.

	Brogan Zumwalt [Inactive]
	Comment 106 • 9 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

	Firefox Bug Husbandry Bot
	Comment 107 • 6 years ago

Removing leave-open keyword from resolved bugs, per :sylvestre.