804459 - [Settings] Can't re-enter Wifi password if password is incorrect after attempting to connect

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect, P2)

Description

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

STR:
1. Open Settings -> Wi-Fi.
2. Connect to password-protected AP with incorrect password.
3. Reconnect to same AP.

Expected:
1. AP shows "connection failed." after step 2.
2. Dialog for entering password is shown after step 3.

Actual:
1. AP(mostly WPA, sometime WEP) shows its security type after step 2.
2. No dialog is shown and connect to AP again directly after step 3.

Comment 1

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

Seems to be the side effect of bug 796461.

connectingfail/disconnect triggers a scan, and the scan result overwrites AP status(as mentioned in Actual 1.)

Without connection fail status, user can't re-enter password by reconnect(as mentioned in Actual 2.)

Comment 2

[Larissa Co [:lco]]

A side effect of this bug is that in the Settings list, the subtitle under Wi-Fi says "Connecting to null".

Also, the incorrect password message should be "Unable to join the network. Check the entered password." It shouldn't just say "connection failed"

Comment 3

[Larissa Co [:lco]]

Sorry for the mistake in the previous comment. Please see pg. 19 of http://people.mozilla.com/~lco/Settings_B2G/Release_1_Specs/R1_Connectivity_v11.pdf
for the appropriate error messages.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

For what it's worth, it's now possible to remove the network with an incorrect password by using the "Manage Networks" button at the bottom of the wifi settings pane.

Comment 5

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

Sure user can remove network settings in "Manage Networks", but I think it's not a instinct operation for them.

More important is that user gets no information about the connection failure, because the status is flushed out by the re-scan.

They might just keep tapping on the AP, wondering why it is not connected.

Comment 6

[Dietrich Ayala (:dietrich)]

Hi Evelyn, you assigned this about a month ago, but no fix yet. Do you have an ETA for a fix? Or if you do not expect to get to this soon, un-assign so someone else can take it?

Comment 7

[Evelyn Hung]

Oh sorry, I was always thinking "will solve this this week" but unexpectedly been interrupted by others... Now I think I should un-assign myself first. :(

Comment 8

[Kevin Grandon :kgrandon]

I was looking at this for a bit, but didn't make too much progress. I seem to never have the onstatuschange event fired with a "connectingfailed" status. When trying with an invalid pasword, I only receive connecting and disconnected events. (Two disconnected events get fired.)

Comment 9

[Andrea Marchesini [:baku]]

[JavaScript Error: "NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: '[JavaScript Error: "Permission denied to access property 'password'" {file: "app://settings.gaiamobile.org/js/wifi.js" line: 676}]' when calling method: [nsIDOMEventListener::handleEvent]" {file: "jar:file:///system/b2g/omni.ja!/components/DOMWifiManager.js" line: 270}]

The problem seems this. It throws when we reset the password.

Comment 10

[Vincent Chang[:vchang][changyihsin]]

Can you try it on AP which set its security to wep or wpa psk/wpa2 psk first ? "Mozilla Mobile" maybe a good choice. 

We have to fire the other bug to handle the AP which set its security type to WPA-EAP.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I can still reproduce with a wpa2 network.

When it fails to connect, we seem to get a permanent "wifi-connecting" icon in the status bar too.

mrbkap, do you have cycles to look at the platform bug here?

Comment 12

[Vincent Chang[:vchang][changyihsin]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #11)
> I can still reproduce with a wpa2 network.
> 
> When it fails to connect, we seem to get a permanent "wifi-connecting" icon
> in the status bar too.

When we try to connect to wpa-psk network with incorrect password, WifiWorker will receive below message from wpa_supplicant to indicate incorrect password event. After WifiWorker receives this event for three times(may takes several seconds), we will disable this network and send status change event with _connectionStatus = connectingfailed to application. 

"WifiWorker component: Event coming in: WPA: 4-Way Handshake failed - pre-shared key may be incorrect"

I just try it with the AP at home using unagi. To make thing clear, I removed all the known network from phone using Manage networks first, and then I try to connect to wpa-psk network with incorrect password. After threes times retry, I saw the wpa_supplicant stops trying to connect to the network. Checking with wpa_cli command and you will see the network is disabled. 

> list_network 
network id / ssid / bssid / flags
0	BUFFALO-6D93F8	any	[DISABLED]

Comment 13

[Andrew Overholt [:overholt]]

Andrea, do you think you can land this by Monday?

Comment 14

[Andrea Marchesini [:baku]]

Andrew, I can try :)

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Andrea and I talked about this and looked a bit on IRC today. This appears to be a Gaia bug. Basically, when it gets the connectingfailed notification, it attempts to delete the "password" property off of mozWifiManager.connection.network. However, that object is unrelated to any other object and changing it doesn't do anything (this is why there's the security error). It would be better (probably) for Gaia to forget the network when we fail to connect to it (or we could add an API to disable it, but that will be hard to implement sanely in the short term).

Comment 16

[Andrea Marchesini [:baku]]

Attachment: forget the network

I don't like this approach but it seems the only (very very easy) approach that we can do without adding a new API or improving the events between backend and app.

The problem is that:
1. when we select a wifi network, we store the network object in a "global" object called gCurrentNetwork: wifi.js:506
2. in the meantime we continuously scan the network: wifi.js:65
3. this scan() creates new network objects
4. if the connection fails we try to delete the password: wifi.js:677 using the global object gCurrentNetwork. Unfortunately this is not referred to the 'network' object used in the list.
5. So when the same network is selected again, network === gCurrentNetwork is false, and the password has not been deleted from the network object.

Ideally I would like to implement a new event when the connection fails because of the invalid password. Then we call forget(network) only in this case.

Blake, do you agree with this analysis?

Comment 17

[Andrea Marchesini [:baku]]

Attachment: forget the network

In the meantime this new release of the patch removes a warning.

Comment 18

[Andrew Overholt [:overholt]]

We discussed this during triage today.  Andrea told me on IRC that he thinks there's a way we can do this properly (handle the signal that authentication caused the connection failure) but it will take some time and touching of a number of places.

It was our understanding during triage that the user will have to forget a network if they typed in the wrong password.  While not ideal, we think this is probably okay for now.

If that's okay with you, can you land the workaround patch, Andrea, and file a followup to do the fix you proposed to me on IRC?

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 689772 [details] [diff] [review]
forget the network

I'm not actually a peer in Gaia, so passing this off to kaze. I did mention that the delete should probably be nuked, since it doesn't actually do anything and would only exist to confuse.

Comment 20

[Fabien Cazenave [:kaze]]

Comment on attachment 689772 [details] [diff] [review]
forget the network

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

Looks good, please address the nitpick and let’s merge it ASAP.

::: apps/settings/js/wifi.js
@@ +676,2 @@
>        // connection has failed, probably an authentication issue...
> +      delete(gCurrentNetwork.password);

As Blake mentioned, this delete() seems useless. Please drop it.

Comment 21

[Andrea Marchesini [:baku]]

> I'm not actually a peer in Gaia, so passing this off to kaze. I did mention
> that the delete should probably be nuked, since it doesn't actually do
> anything and would only exist to confuse.

I prefer to keep the 'delete' because we cannot be sure that the list has been refresh in the meantime we receive the 'disconnect'. If we don't the password is still in the 'network' object.

Blake, question for you: Why do we receive multiple 'disconnect' events? Can it be a bug? should we emit 'disconnect' events only after a 'connect' request?

Pull Request: https://github.com/mozilla-b2g/gaia/pull/6885

Comment 22

[Fabien Cazenave [:kaze]]

Andrea, I can’t merge your PR because it seems to address two different bugs. Please split it.

Comment 23

[Andrea Marchesini [:baku]]

https://github.com/mozilla-b2g/gaia/pull/6920

Comment 24

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://github.com/mozilla-b2g/gaia/commit/13a8d8a2ebda3c3ac60b2c643855784ede2b2943

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Andrea Marchesini (:baku) from comment #21)
> Blake, question for you: Why do we receive multiple 'disconnect' events? Can
> it be a bug? should we emit 'disconnect' events only after a 'connect'
> request?

We don't make any attempt in the backend to maintain that, and in general we could get a disconnected status after any attempt to connect (or scan, I think). So I don't think we need to worry about it. Is dealing with multiple disconnect events causing problems?

Comment 26

[Andrea Marchesini [:baku]]

> think). So I don't think we need to worry about it. Is dealing with multiple
> disconnect events causing problems?

Well.. any time the wifi.js receives a "disconnect" event, the scan() is called. This refreshes the list of networks and makes complex to invalidate the password of the current network if the authentication fails.

Maybe would be nice to improve the wifi so that we emit disconnect events only when needed.
Do we want to file a follow up bug?