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

RESOLVED FIXED in B2G C2 (20nov-10dec)

Status

Firefox OS
Gaia::Settings
P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: chucklee, Assigned: baku)

Tracking

({regression})

unspecified
B2G C2 (20nov-10dec)
All
Gonk (Firefox OS)
regression

Firefox Tracking Flags

(blocking-basecamp:+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
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.)
blocking-basecamp: --- → ?

Updated

6 years ago
Assignee: nobody → ehung

Updated

6 years ago
blocking-basecamp: ? → +
Priority: -- → P3

Comment 2

6 years ago
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

6 years ago
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.
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.
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.

Updated

6 years ago
Component: Gaia → Gaia::Settings
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

6 years ago
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. :(
Assignee: ehung → nobody
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.)
(Assignee)

Comment 9

6 years ago
[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.
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.
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?
Assignee: nobody → mrbkap

Updated

6 years ago
Keywords: regression
Priority: P3 → P2
Target Milestone: --- → B2G C2 (20nov-10dec)
(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]
Component: Gaia::Settings → DOM: Device Interfaces
Product: Boot2Gecko → Core
Andrea, do you think you can land this by Monday?
Assignee: mrbkap → amarchesini
(Assignee)

Comment 14

5 years ago
Andrew, I can try :)
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).
(Assignee)

Comment 16

5 years ago
Created attachment 689771 [details] [diff] [review]
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?
Attachment #689771 - Flags: review?(mrbkap)
(Assignee)

Comment 17

5 years ago
Created attachment 689772 [details] [diff] [review]
forget the network

In the meantime this new release of the patch removes a warning.
Attachment #689771 - Attachment is obsolete: true
Attachment #689771 - Flags: review?(mrbkap)
Attachment #689772 - Flags: review?(mrbkap)
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?
Flags: needinfo?(amarchesini)
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.
Attachment #689772 - Flags: review?(mrbkap) → review?(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.
Attachment #689772 - Flags: review?(kaze) → review+
(Assignee)

Comment 21

5 years ago
> 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
Flags: needinfo?(amarchesini)
Keywords: checkin-needed
Component: DOM: Device Interfaces → Gaia::Settings
Product: Core → Boot2Gecko
Keywords: checkin-needed
Andrea, I can’t merge your PR because it seems to address two different bugs. Please split it.
(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?
(Assignee)

Comment 26

5 years ago
> 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?
You need to log in before you can comment on or make changes to this bug.