1030063 - Captive portal notification spam

Status: RESOLVED FIXED

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

Description

[:gerard-majax]

The captive portal detection code spams a lot when detecting the same captive portal.

STR:
 0. Be under WiFi coverage with captive portal (SFR WiFi Gare, eduspot are reproducing easily)
 1. Enable network

Expected:
 When a captive portal (identified by SSID) is detected several times, I should only have one notification.

Actual:
 After some time you can get several notifications in the notification tray for the same SSID.

This is reproducing on 2.0 and is a very bad UX from a user point of view.

Looking at the code in the captive portal manager, it seems it does not make use of the Notification API at all!
https://github.com/mozilla-b2g/gaia/blob/05174650872cc733462cd4fcfb04f70c0215bba7/apps/system/js/captive_portal.js#L36

Looking at the history shows this has always been the case.

Alive, is there any reason for keeping code like this?

I don't see anything that cannot be achieved with the Notification API. Moreover, using this API would allow to use SSID as a tag and hence avoid spamming user.

Comment 1

[Jason Smith [:jsmith]]

William - Are you able to reproduce this?

Comment 2

[:gerard-majax]

(In reply to Jason Smith [:jsmith] from comment #1)
> William - Are you able to reproduce this?

I hope so, Flame 2.0, with the latest update installed yesterday.

Please note you need to leave the phone with WiFi enabled. I suspect that the device disables/enables WiFi when sleeping, and that each time its enabled, it connects and trigger Captive Portal.

Comment 3

[Preeti Raghunath(:Preeti)]

Seems bad ux to the user.

Comment 4

[Henry Chang [:hchang]]

(In reply to Alexandre LISSY :gerard-majax from comment #2)
> (In reply to Jason Smith [:jsmith] from comment #1)
> > William - Are you able to reproduce this?
> 
> I hope so, Flame 2.0, with the latest update installed yesterday.
> 
> Please note you need to leave the phone with WiFi enabled. I suspect that
> the device disables/enables WiFi when sleeping, and that each time its
> enabled, it connects and trigger Captive Portal.

If I am not reading the code wrong, when captive portal is being detected 
and then we disable wifi, gecko is supposed to send a 'captive-portal-login-abort' chrome event.
and gaia is supposed to pop the notification because of [1].

Also, this should also happen before 2.0 since the code is existing for a while.

[1] https://github.com/mozilla-b2g/gaia/blob/df90d63f43bf8956ec5595916542476482a375a3/apps/system/js/captive_portal.js#L64

Comment 5

[William Hsu [:whsu]]

This is current design from v1.0 ~ present.
QA have followed the rule to test Captive Portal for a long time.
- https://moztrap.mozilla.org/manage/cases/?pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-tag=1529&filter-productversion=177

If you feel it is bad design, we can NI? UX team to see if we need to change the current design.

By the way, as I known, the notification is not always displayed.
Once the system detects WIFI signal(Captive Portal) that meets 3/5+ signal strength, then system will pop up the notification.

Thanks! :)

Comment 6

[Jason Smith [:jsmith]]

If this is a long-standing issue then this isn't a blocker. We can look into improving the UX in the future though.

Comment 7

[:gerard-majax]

(In reply to Jason Smith [:jsmith] from comment #6)
> If this is a long-standing issue then this isn't a blocker. We can look into
> improving the UX in the future though.

I disagree. In my case, the device at some point had more than 20 notifications ...

Comment 8

[:gerard-majax]

(In reply to William Hsu [:whsu] from comment #5)
> This is current design from v1.0 ~ present.
> QA have followed the rule to test Captive Portal for a long time.
> -
> https://moztrap.mozilla.org/manage/cases/
> ?pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-
> tag=1529&filter-productversion=177
> 
> If you feel it is bad design, we can NI? UX team to see if we need to change
> the current design.
> 
> By the way, as I known, the notification is not always displayed.
> Once the system detects WIFI signal(Captive Portal) that meets 3/5+ signal
> strength, then system will pop up the notification.
> 
> Thanks! :)

That's cool, but in the present case I'm talking about real life usage, not QA testing. And in this dogfooding context, the improper behavior makes the captive portal feature very very very noisy and disturbing.

Comment 9

[:gerard-majax]

I don't know if the events chain from Gecko is handled properly or not, but I know that instead of manipulating directly the NotificationScreen, if there was use of the Notification API with a tag linked to the SSID discovered, then user impact of this bug could be mitigated and avoid the captive portal to be too noisy.

Comment 10

[William Hsu [:whsu]]

(In reply to Alexandre LISSY :gerard-majax from comment #8)

> That's cool, but in the present case I'm talking about real life usage, not
> QA testing. And in this dogfooding context, the improper behavior makes the
> captive portal feature very very very noisy and disturbing.

Yes! You got that right. We need to stand in user's shoes.
I mentioned the QA testing here that just because I want to tell you the current design (V1.0~Present).
Also, we can request more information to see if V2.0 recently had any change and impact the behavior.
If we don't have any change regarding the design, we can request UX to review the behavior.
What do you think? :)

Also, NI? Vincent and Chuck here to see if they have any suggestion.

---------------------------------------------------------------------------
Hi, Vincent and Chuck,

Could I have your help?
Alexandre had some concerned regarding the notification of captive portal.
We cannot sure if the events chain had any change recently. Any suggestion?
If it doesn't have any change and the frequency of notification is expected, we would like to request UX team to give some suggestions.
Thanks!

Comment 11

[William Hsu [:whsu]]

(In reply to Alexandre LISSY :gerard-majax from comment #7)
> (In reply to Jason Smith [:jsmith] from comment #6)
> > If this is a long-standing issue then this isn't a blocker. We can look into
> > improving the UX in the future though.
> 
> I disagree. In my case, the device at some point had more than 20
> notifications ...

One more request, could you please give the detailed information/steps regarding "some point" you mentioned?
If you can provide a video, We would appreciate your help. :)

Comment 12

[:gerard-majax]

(In reply to William Hsu [:whsu] from comment #11)
> (In reply to Alexandre LISSY :gerard-majax from comment #7)
> > (In reply to Jason Smith [:jsmith] from comment #6)
> > > If this is a long-standing issue then this isn't a blocker. We can look into
> > > improving the UX in the future though.
> > 
> > I disagree. In my case, the device at some point had more than 20
> > notifications ...
> 
> One more request, could you please give the detailed information/steps
> regarding "some point" you mentioned?
> If you can provide a video, We would appreciate your help. :)

No, I cannot provide a video, but the STRs are already documented:
 0. Make sure you are under captive portal coverage (reproduced with "SFR WiFi Gares" and "eduspot" networks for sure)
 1. Enable wifi, leave your device in sleep mode.

And then at some point you get several notifications.

I should be able to provide a screenshot of the resulting notifications, however.

Comment 13

[:gerard-majax]

And by « at some point », I meant after some time.

Comment 14

[:gerard-majax]

Attachment: 2014-06-26-18-08-11.png

Here is the screenshot. Device is running Aurora, buildid 20140624000201. I can't remember the base image I flashed, but it should be at least v120F.

Comment 15

[Jason Smith [:jsmith]]

Ok, I'll reset the blocking flag then after setting the screenshot.

Comment 16

[William Hsu [:whsu]]

Good job, Alexandre! Thanks so much!! :)

Oh~ the user scenario I concerned finally happened.
Because we used signal strength to determine whether we need to pop up notification or not.
So, if you stay at a hotspot which the WIFI signal strength is unstable.
This phenomenon will happen.

ex:
00:00 AM => WIFI signal strength is 3/5, then the notification is displayed.
00:10 AM => WIFI signal strength changes to 0/5, then the WIFI is disconnected.
00:00 AM => WIFI signal strength is 4/5, then the notification is displayed again.
etc...

I think this phenomenon also happened on V1.0 build.

--- -- - --- -- - --- -- - --- -- - --- -- - --- -- - --- -- - 
Hi, Vincent and Chuck,

Could I have your comment on this?
Thanks!

Comment 17

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

I think there's nothing wrong for gecko to send out captive-portal event on every new wifi connection(or reconnection) - gecko won't know how last event is handled.

Since the captive portal notification is maintained by system app, I think maybe it can do something to prevent notification redundancy.

Comment 18

[William Hsu [:whsu]]

> ex:
> 00:00 AM => WIFI signal strength is 3/5, then the notification is displayed.
> 00:10 AM => WIFI signal strength changes to 0/5, then the WIFI is
> disconnected.
> 00:00 AM => WIFI signal strength is 4/5, then the notification is displayed
> again.

Sorry for the incorrect information. It should be...
00:20 AM => WIFI signal strength is 4/5, then the notification is displayed again.

Comment 19

[:gerard-majax]

(In reply to Chuck Lee [:chucklee] from comment #17)
> I think there's nothing wrong for gecko to send out captive-portal event on
> every new wifi connection(or reconnection) - gecko won't know how last event
> is handled.
> 
> Since the captive portal notification is maintained by system app, I think
> maybe it can do something to prevent notification redundancy.

I agree, let's just make notifications used properly: either fix the code path that uses NotificationScreen, or switch to Notification API (and clear remaining notifications on startup).

Comment 20

[Vincent Chang[:vchang][changyihsin]]

I think comment 17 has answered the NI.

Comment 21

[Henry Chang [:hchang]]

(In reply to Alexandre LISSY :gerard-majax from comment #19)
> (In reply to Chuck Lee [:chucklee] from comment #17)
> > I think there's nothing wrong for gecko to send out captive-portal event on
> > every new wifi connection(or reconnection) - gecko won't know how last event
> > is handled.
> > 
> > Since the captive portal notification is maintained by system app, I think
> > maybe it can do something to prevent notification redundancy.
> 
> I agree, let's just make notifications used properly: either fix the code
> path that uses NotificationScreen, or switch to Notification API (and clear
> remaining notifications on startup).

I am still trying to understand the difference between NotificationScreen and
Notification API but as what I mentioned on comment 4, one of the following
might have happened to result in notification spam:

1) Gecko didn't send correct captive-portal-login-abort event.
2) Gaia didn't receive corrent captive-portal-login-abort event.
3) NotificationScreen.removeNotification didn't work at all.

Comment 22

[:gerard-majax]

(In reply to Henry Chang [:henry] from comment #21)

[...]

> 
> I am still trying to understand the difference between NotificationScreen and
> Notification API but as what I mentioned on comment 4, one of the following

NotificationScreen is the part of the system app that handles displaying the notification to the user. Notification API is a WebAPI to manage notifications.

Comment 23

[Henry Chang [:hchang]]

I tried on flame with some hacks which make captive portal would be detected every time connecting to wifi. The notification was dismissed as expected when I disabled wifi, which implies none of the three cases mentioned on comment 21 happened. Then I left the phone over night and the notification did not accumulate in the morning. It means the current design basically works to avoid spam. Maybe there are exceptional cases that is not covered.

Comment 24

[:gerard-majax]

(In reply to Henry Chang [:henry] from comment #23)
> I tried on flame with some hacks which make captive portal would be detected
> every time connecting to wifi. The notification was dismissed as expected
> when I disabled wifi, which implies none of the three cases mentioned on
> comment 21 happened. Then I left the phone over night and the notification
> did not accumulate in the morning. It means the current design basically
> works to avoid spam. Maybe there are exceptional cases that is not covered.

What Flame and Gecko build did you tried? Are you sure you are really reproducing the very same environment?

We are talking about a public hotspot with captive portal and people connecting on it. Since you are mentionning signal impact, what makes you sure that when you left your phone over the night, you were able to reproduce the very same signal variations?

Comment 25

[:gerard-majax]

(In reply to Henry Chang [:henry] from comment #21)

[...]

> 
> I am still trying to understand the difference between NotificationScreen and
> Notification API but as what I mentioned on comment 4, one of the following
> might have happened to result in notification spam:
> 
> 1) Gecko didn't send correct captive-portal-login-abort event.
> 2) Gaia didn't receive corrent captive-portal-login-abort event.
> 3) NotificationScreen.removeNotification didn't work at all.

Let aside case 3, shouldn't the Captive Portal code make sure there is no way for spamming the user?
Your use of |NotificationScreen.addNotification()| makes use of an 'id' at https://github.com/mozilla-b2g/gaia/blob/34a52e7f024cc3d0e3aade94970773d2555f5ccb/apps/system/js/captive_portal.js#L20 that is being passed directly. The addNotification method makes use of this id to replace previous notifications, as per https://github.com/mozilla-b2g/gaia/blob/34a52e7f024cc3d0e3aade94970773d2555f5ccb/apps/system/js/notifications.js#L343. But what is the semantic and value of this id in the case of the captive portal event?

Consider a user moving around somewhere with a lot of different captive portal, do we want the user to get a notification for each?

Comment 26

[:gerard-majax]

The events details are produced from http://hg.mozilla.org/mozilla-central/file/606848e8adfc/toolkit/components/captivedetect/captivedetect.js#l334, and more specifically |let id = this._allocateRequestId();|.

Looking at this method at http://hg.mozilla.org/mozilla-central/file/606848e8adfc/toolkit/components/captivedetect/captivedetect.js#l389 shows that we are jsut incrementing |_nextRequestId|.

So each new captive portal notification for the very same SSID will have a different id, hence the NotificationScreen code logic cannot detect an old notification to replace.

I cannot, for now, document or explain whether we get the login abort event or not, but just the notification add steps seems to be spam-friendly.

Comment 27

[Henry Chang [:hchang]]

(In reply to Alexandre LISSY :gerard-majax from comment #24)
> (In reply to Henry Chang [:henry] from comment #23)
> > I tried on flame with some hacks which make captive portal would be detected
> > every time connecting to wifi. The notification was dismissed as expected
> > when I disabled wifi, which implies none of the three cases mentioned on
> > comment 21 happened. Then I left the phone over night and the notification
> > did not accumulate in the morning. It means the current design basically
> > works to avoid spam. Maybe there are exceptional cases that is not covered.
> 
> What Flame and Gecko build did you tried? Are you sure you are really
> reproducing the very same environment?
> 
> We are talking about a public hotspot with captive portal and people
> connecting on it. Since you are mentionning signal impact, what makes you
> sure that when you left your phone over the night, you were able to
> reproduce the very same signal variations?

I tried gecko that I built a couple of days ago. Since I am not the one who
made the current design, I just read the code and want to verify if everything 
is running as I suppose.

Also, I didn't know the signal impact on captive portal before. I also heard it 
from William on the his comments so that I didn't consider the signal variant 
in my simple test.

Regarding your question:

"Consider a user moving around somewhere with a lot of different captive portal, 
do we want the user to get a notification for each?"

My answer is, if the user connects to yet another captive portal, the previous
notification (say, it has id 99) should be removed since gecko will send 
a 'captive-portal-login-abort' with id 99 and gaia will then remove the notification id 99.

By the way, I am not defensing the current code :p I am just figuring out why the spam
occurred since gecko+gaia already dealt with it.

Thanks!

Comment 28

[:gerard-majax]

There is something wrong, at some point. I just reproduced this a couple of minutes ago on my Nexus S. Just walking to get to my train and waiting for its departure, over a timeframe of 10 minutes, I received two notifications for the very same captive portal.

Comment 29

[:gerard-majax]

Taking for now.

Comment 30

[:gerard-majax]

Attachment: Link to Github https://github.com/mozilla-b2g/gaia/pull/21518

First fix for this: moving the CaptivePortal system app handler to use Notification API. This is expected to be exactly iso-functionnal to the previous code, i.e., no behavior change.

It still lacks old notifications cleanup on boot, but I'll update the PR asap with this. Captive portal unit tests are locally green with this.

Comment 31

[:gerard-majax]

And looking at the linters status now that my PR removed the disabling of jshint, here we go:

> apps/system/js/captive_portal.js: line 88, col 45, Expected a 'break' statement before 'case'. (ERROR)

That may explain the bug, since the missing break is after the 'captive-portal-login-abort' case handling. Hence, 'captive-portal-login-abort' will also trigger this.handleLoginSuccess() ...

Comment 32

[:gerard-majax]

So cool. Bug 1027471 just completely invalidated my work.

Comment 33

[:gerard-majax]

Attachment: Link to Github https://github.com/mozilla-b2g/gaia/pull/21527

Restarting everything on current master.

Comment 34

[:gerard-majax]

Attachment: Link to Github https://github.com/mozilla-b2g/gaia/pull/21568

Restarting the previous PR since bug 1027471 has been backed out.

Comment 35

[:gerard-majax]

Gaia-Try is (mostly) green: https://tbpl.mozilla.org/?rev=1085a7a2e39574b9f25978dfd15bc618d066c817&tree=Gaia-Try

Comment 36

[:gerard-majax]

Comment on attachment 8453597 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/21568

Alive, please find attached a PR that should fix the issue. It's green on all tests, and by using Notification API with tag we should not run into the spamming again.

I have no way to test this on a real captive portal in the given time frame, maybe William can help us making sure it's all okay?

Comment 37

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

Comment on attachment 8453597 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/21568

I need QA's confirm nothing is broken.
The only concern is the notification iterator before attaching mozChromeEvent handler when CaptivePortal is inited.

The reason we didn't use notification API was there's no new Notification API one year ago.

Any way thanks for helping, I felt the code is really old fashion to me.....

Comment 38

[:gerard-majax]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #37)
> Comment on attachment 8453597 [details] [review]
> Link to Github https://github.com/mozilla-b2g/gaia/pull/21568
> 
> I need QA's confirm nothing is broken.
> The only concern is the notification iterator before attaching
> mozChromeEvent handler when CaptivePortal is inited.

Do you fear race ?

> 
> The reason we didn't use notification API was there's no new Notification
> API one year ago.

Of course, I can't blame for not using something that was non existent :)

Comment 39

[William Hsu [:whsu]]

Hi, Gerry,

Could I have your help because I am busy on Dolphin testing?
Please help Alexandre to verify following patch.
Comment on attachment 8453597 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/21568

Thanks!

Comment 40

[Gerry Chang [:gchang]]

Hi  Alexandre,
I can't recreate your issues. So, this might need you to verify the issue. you posted. I've run our captive portal test cases without problems.
I only have one question about tapping notification bar.
What is the behaviour if user click the notification bar in notification page. What I observe is that the bar can be clicked but nothing happened.

Comment 41

[:gerard-majax]

(In reply to Gerry Chang [:cfchang] from comment #40)
> Hi  Alexandre,
> I can't recreate your issues. So, this might need you to verify the issue.
> you posted. I've run our captive portal test cases without problems.
> I only have one question about tapping notification bar.
> What is the behaviour if user click the notification bar in notification
> page. What I observe is that the bar can be clicked but nothing happened.

That will be very hard. I don't have any captive portal in the Berlin work week.

Comment 42

[:gerard-majax]

Gerry, the bogus tapping may be my fault: we do not generate any 'tap' event listener in Gecko. So I switched to 'click', that should fix it :)

Comment 43

[Gerry Chang [:gchang]]

After applying your fix, the 'click' action can launch browser.

Comment 44

[:gerard-majax]

(In reply to Gerry Chang [:cfchang] from comment #43)
> After applying your fix, the 'click' action can launch browser.

Thanks, we're good to land, Try is green: https://tbpl.mozilla.org/?rev=8a23d8e161ddf0d6c0fcb619815ff506c72d5368&tree=Gaia-Try !

Comment 45

[:gerard-majax]

https://github.com/mozilla-b2g/gaia/commit/6a1f5efaa8d278e0848a1e14b09cd1133211683e

Comment 46

[William Hsu [:whsu]]

Good Job, Alexandre! Thanks!

--- -- - --- -- - --- -- - --- -- - --- -- - --- -- -
Hi, Gerry,

Could I have your help again?
Please verify it on latest v2.0 PVT build to make sure that it has been successfully integrated.
If you are happy with the result, please mark it as "VERIFIED"

I will be grateful for any help you can provide. :)

Comment 47

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for v2.0 uplift.

Comment 48

[:gerard-majax]

Attachment: Gaia PR against v2.0 branch

Ryan, this PR should be good to land against v2.0 branch :)

Comment 49

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/72fef00c8c7bc96927f70fbff2c955be7410aacf

Comment 50

[Ryan VanderMeulen [:RyanVM]]

Reverted for the Captive Portal test failures shown in the log below.
v2.0: https://github.com/mozilla-b2g/gaia/commit/db0358b53488a941ced352d890802342d7ced00e

https://tbpl.mozilla.org/php/getParsedLog.php?id=43843567&tree=Mozilla-Aurora

Comment 51

[:gerard-majax]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #50)
> Reverted for the Captive Portal test failures shown in the log below.
> v2.0:
> https://github.com/mozilla-b2g/gaia/commit/
> db0358b53488a941ced352d890802342d7ced00e
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=43843567&tree=Mozilla-Aurora

Sorry, I pushed and flagged without telling you that I was waiting for Try result :)

Comment 52

[:gerard-majax]

Attachment: Gaia PR against v2.0 branch, fixed

Gerry, can you just make sure this PR is okay ?

Comment 53

[Gerry Chang [:gchang]]

Hi Alexandre,
I've tested the same scenarios on your patch. No regression issues are found and "click" can also launch browser to login page.

Comment 54

[Gerry Chang [:gchang]]

Comment on attachment 8456473 [details] [review]
Gaia PR against v2.0 branch, fixed

As mentioned in comment #53.

Comment 55

[:gerard-majax]

So the PR is ready, rebased on top of 2.0, but I cannot get any green try because it's constantly failing about merge conflicts: https://tbpl.mozilla.org/?tree=Gaia-Try&rev=4e84eef11012

Comment 56

[:gerard-majax]

Attachment: Gaia PR against v2.0 branch, fixed, rebased

Let's hope try will work this time.

Comment 57

[:gerard-majax]

Setting checking-needed for branch 2.0. It is my understanding that Gaya-Try only does its job against master branch, which will obviously always fail in our case. Tests are green locally, though.

Comment 58

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/5fa5611fc747d397dbaa2c5ccdc776f1dfd2b6e8

Comment 59

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Alexandre LISSY :gerard-majax from comment #57)
> Setting checking-needed for branch 2.0. It is my understanding that Gaya-Try
> only does its job against master branch, which will obviously always fail in
> our case. Tests are green locally, though.

As in the master branch of Gecko?  that's no longer correct.  We use the correct builds of Gecko for each Gaia version.  To find out which Gecko bundle your commit ran against, look at the Gaia-Try repository commit at the linux64.json file.