938044 - B2G: data connection not automatically reconnected after wifi is disconnected

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Wifi, defect)

Description

[Jessica Jong [:jessica] (inactive)]

I found this issue using latest master branch.

STR:
1. Turn on data connection
2. Turn on wifi and connect to an AP
  (After wifi is successfully connected, data connection will be disconnected)
3. Turn off wifi

Expected results:
data connection should be automatically reconnected

Actual results:
data connection not reconnected

Comment 1

[Jessica Jong [:jessica] (inactive)]

Attachment: bug-938044.log

Looks like NetworkManager is not receiving the NETWORK_STATE_DISCONNECTED event from wifi after turning it off.

Comment 2

[Jessica Jong [:jessica] (inactive)]

Hi, could this be related to the fix in Bug 921620?

Comment 3

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

Seems related. Thanks.

Comment 4

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

Attachment: WIP - When disabling wifi, change to DISCONNECT state first.

Comment 5

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 831447 [details] [diff] [review]
WIP - When disabling wifi, change to DISCONNECT state first.

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

Let's try to come out a better solution, r- for now.

::: dom/wifi/WifiWorker.js
@@ +904,5 @@
>      } else {
>        // Note these following calls ignore errors. If we fail to kill the
>        // supplicant gracefully, then we need to continue telling it to die
>        // until it does.
> +      notifyStateChange({ state: "DISCONNECTED", BSSID: null, id: -1 });

It looks like a hack solution here. Can we have other way to fix this ? The bug here is because we block the state change event when we are in "UNINITIALIZED" and "DISABLING" state. Any reason why should we block the state change event ?

Comment 7

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

(In reply to Vincent Chang[:vchang] from comment #5)
> Comment on attachment 831447 [details] [diff] [review]
> WIP - When disabling wifi, change to DISCONNECT state first.
> 
> Review of attachment 831447 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Let's try to come out a better solution, r- for now.
> 
> ::: dom/wifi/WifiWorker.js
> @@ +904,5 @@
> >      } else {
> >        // Note these following calls ignore errors. If we fail to kill the
> >        // supplicant gracefully, then we need to continue telling it to die
> >        // until it does.
> > +      notifyStateChange({ state: "DISCONNECTED", BSSID: null, id: -1 });
> 
> It looks like a hack solution here. Can we have other way to fix this ? The
> bug here is because we block the state change event when we are in
> "UNINITIALIZED" and "DISABLING" state. Any reason why should we block the
> state change event ?

I only want to block state change from those two states.
Blocking the event is a side effect.

Comment 8

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

Attachment: WIP - When disabling wifi, change to DISCONNECT state first. V2

Comment 9

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

Attachment: When disabling wifi, change to DISCONNECT state first. V3

Change order of sending disconnect event to prevent race condition.

Comment 10

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

Attachment: When disabling wifi, change to DISCONNECT state first. V3.1

Change commit summary.

Comment 11

[Ken Chang[:kenkai]]

It is an critical regression.

Comment 12

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8336660 [details] [diff] [review]
When disabling wifi, change to DISCONNECT state first. V3.1

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

Thanks for your patch. It looks good.

Comment 13

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

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/7c8fd7cafb8b

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7c8fd7cafb8b