Closed Bug 736089 Opened 8 years ago Closed 8 years ago

Wifi: Handle broken wifi configurations more gracefully

Categories

(Core :: DOM: Device Interfaces, defect, P1)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
blocking-kilimanjaro +
blocking-basecamp +

People

(Reporter: mrbkap, Assigned: vchang)

References

Details

(Whiteboard: [LOE:M])

Attachments

(1 file, 2 obsolete files)

Currently, if you mis-enter a password or try to connect to a mis-configured network, wpa_supplicant loops until user interaction. We should:

a) Detect when we're looping through the authentication phases and automatically disable the network after a few tries.
b) Catch "invalid password" type messages from the supplicant and fire events
Assignee: nobody → vchang
blocking-basecamp: --- → ?
blocking-kilimanjaro: --- → ?
Sounds like it results in a state in which no connection occurs and the user is not notified, which is not OK. Blocking+.
blocking-basecamp: ? → +
blocking-kilimanjaro: ? → +
Should it be handled by network manager and then deployed the error to apps?
khu: I don't understand what you mean, exactly. The network manager is only notified when interfaces go up and down and for connection/disconnection events. It doesn't handle things at a low enough level.

That being said, there are a few things that need to happen here:
  - wpa_supplicant will sometimes send error messages (right now we have an internal message for "pre-shared key might be incorrect"). In that case, we should notify the DOM side via a notification and cancel the connection attempt (disable the network).
  - Sometimes (especially with a weak signal, or with some WPA/WEP networks) there won't be any notification, but we'll associate and then fail to get DHCP, or we'll go from one of the connecting states earlier than "COMPLETED" repeatedly. We need to detect when we're looping between these states and abandon the connection attempt.

One tricky bit will be to figure out when to re-enable disabled networks in a way that doesn't knock off a lower-priority but established connection.
How are we doing here?
Priority: -- → P1
(In reply to Blake Kaplan (:mrbkap) from comment #4)
> khu: I don't understand what you mean, exactly. The network manager is only
> notified when interfaces go up and down and for connection/disconnection
> events. It doesn't handle things at a low enough level.
> 
> That being said, there are a few things that need to happen here:
>   - wpa_supplicant will sometimes send error messages (right now we have an
> internal message for "pre-shared key might be incorrect"). In that case, we
> should notify the DOM side via a notification and cancel the connection
> attempt (disable the network).

  We can have a counter and increase the count when receive this error message. If the counter is bigger than certain value in DISCONNECTED state, disable the current network and notify DOM side. 

>   - Sometimes (especially with a weak signal, or with some WPA/WEP networks)
> there won't be any notification, but we'll associate and then fail to get
> DHCP, or we'll go from one of the connecting states earlier than "COMPLETED"
> repeatedly. We need to detect when we're looping between these states and
> abandon the connection attempt.

  wpa_supplicant may send these messages "AUTHENTICATING, ASSOCIATING, ASSOCIATED, FOUR_WAY_HANDSHAKE, GROUP_HANDSHAKE" to our event worker. When we receive one of these message, we are in handshake state. If we receive one of these message continually(more than four times) in handshake state, then we are in a loop. We have to disable the current network and notify DOM side.   
> 
> One tricky bit will be to figure out when to re-enable disabled networks in
> a way that doesn't knock off a lower-priority but established connection.
Do we need to disconnect a lower-priority Ap and re-connect to higher priority here ?
Whiteboard: [LOE:M]
Attached patch WIP (obsolete) — Splinter Review
Fire the connection fail event to DOM site in case of 
1. pre-shared key may be incorrect
2. detect wpa_supplicant loop in handshake state.
Attachment #652695 - Flags: feedback?(mrbkap)
Comment on attachment 652695 [details] [diff] [review]
WIP

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

This looks basically right (so f+) but there are a couple of non-nit comments.

::: dom/wifi/WifiWorker.js
@@ +1224,5 @@
> +    var isStateInHandShake = manager.isHandShakeState(state);
> +
> +    if (isPrevStateInHandShake) {
> +      // Increase the count if we are already in HandShake state.
> +      if (isStateInHandShake) {

I think this will increase loopDetectionCount too quickly. IMO we're only looping if we go e.g. from a later state to an earlier state. It's a little annoying that internally our states are simply strings, since it makes that comparison harder to do, but I think we have to do something like that.

@@ +1586,5 @@
>                                    netId: WifiManager.connectionInfo.id };
>            WifiManager.getNetworkConfiguration(self.currentNetwork, function(){});
>          }
> +        // The full authentication process is completed, reset the count.
> +        WifiManager.authenticationFailuresCount = 0;

Do you have to set loopDetectionCount to 0 here as well?
Attachment #652695 - Flags: feedback?(mrbkap) → feedback+
> I think this will increase loopDetectionCount too quickly. IMO we're only
> looping if we go e.g. from a later state to an earlier state. It's a little
> annoying that internally our states are simply strings, since it makes that
> comparison harder to do, but I think we have to do something like that.

loopDetectionCount will be increased by 1 when previous state is in handshake state and current state is also in handshake state. And reset to zero when first time move from other states to handshake state.  
"previous state" "current state" "loopdetectcount" 
0                0               0
0                1               0
1                1               1
1                1               2
1                0               2 
0                0               2
0                1               0 
          
> > +        // The full authentication process is completed, reset the count.
> > +        WifiManager.authenticationFailuresCount = 0;
> 
> Do you have to set loopDetectionCount to 0 here as well?

Because the logical above can help to reset loopDetectionCount, we may not have to do it here. But can be safer if set it to zero as well.
(In reply to Vincent Chang from comment #9)
> loopDetectionCount will be increased by 1 when previous state is in
> handshake state and current state is also in handshake state.

This feels counterintuitive to me. We're not looping until we go from a later state to an earlier one. The way this is described, the number of loops that are possible will depend on the type of security used (more handshake states means fewer loops). I'd really prefer to see "loop" actually meaning loop.

> Because the logical above can help to reset loopDetectionCount, we may not
> have to do it here. But can be safer if set it to zero as well.

Ah, right. I'd still prefer to explicitly reset it for clarity and explicitness (also since it parallels authenticationFailuresCount.
Attached patch Updated the patch V2 (obsolete) — Splinter Review
Updated the patch based on comment 8 and 10.
Attachment #652695 - Attachment is obsolete: true
Attachment #656135 - Flags: review?(mrbkap)
1. set the current network to null 
2. restore one blank line
Attachment #656135 - Attachment is obsolete: true
Attachment #656135 - Flags: review?(mrbkap)
Attachment #656147 - Flags: review?(mrbkap)
Comment on attachment 656147 [details] [diff] [review]
Updated the patch V2.1

r=me. Thanks a lot!
Attachment #656147 - Flags: review?(mrbkap) → review+
https://hg.mozilla.org/mozilla-central/rev/a0240c1043ee
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.