1005775 - [Wifi] [Follow up of Bug 999388]Refactor wifi when send terminate command to wpa_supplicant

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Wifi, defect)

Description

[Vincent Chang[:vchang][changyihsin]]

This is found in Tarako platfrom that CTRL-EVENT-TERMINATING event may not send from wpa_supplicant if stop supplicant command is sent right after sending terminate wpa_supplicant command. 

The terminate command response from the supplicant doesn't mean that supplicant is terminated successfully. Instead of that, we should wait CTRL-EVENT-TERMINATING from event thread, and stop and close supplicant there.

Comment 1

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.0

Comment 2

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

Comment on attachment 8417816 [details] [diff] [review]
Patch v1.0

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

The concept looks good to me, I think there might be a timing issue to resolve.

::: dom/wifi/WifiWorker.js
@@ +849,5 @@
> +      waitForTerminateEventTimer.cancel();
> +      waitForTerminateEventTimer = null;
> +    }
> +  };
> +  function createWaitForTerminateEventTimer(onTimeout) {

If we only accept one timer at a time, we need to check if |waitForTerminateEventTimer| is already created before.
If |waitForTerminateEventTimer| is not null, we need to cancel previous timer or ignore this call.

@@ +959,5 @@
>        // until it does.
>        manager.state = "DISABLING";
>        wifiCommand.terminateSupplicant(function (ok) {
>          manager.connectionDropped(function () {
> +          manager.stopSupplicantCallback = (function () {

|manager.connectionDropped()| have function calls to netutil which might takes long time, and if "CTRL-EVEN-TERMINATING" is received before |manager.stopSupplicantCallback()| and |createWaitForTerminateEventTimer()| are set, we have to wait 2 seconds for timer to trigger it.

This won't happen now, but might happen if we land bug 994564.

I think it's better set |manager.stopSupplicantCallback()| before calling |terminateSupplicant()|.

@@ +972,5 @@
> +          }).bind(this);
> +          let terminateEventCallback = (function() {
> +            handleEvent("CTRL-EVENT-TERMINATING");
> +          }).bind(this);
> +          createWaitForTerminateEventTimer(terminateEventCallback);

As mentioned above, timer might be created in the middle of |manager.stopSupplicantCallback()|, and we will get two "CTRL-EVENT-TERMINATING" events.
Although it seems second event won't do any harm to us, it would be better if we can prevent create an unnecessary timer here.

Maybe only create timer if |manager.stopSupplicantCallback| is not null, and we can set |manager.stopSupplicantCallback| to null earlier in |handleEvent{}| by

if (manager.stopSupplicantCallback) {
  cancelWaitForTerminateEventTimer();
  let stopSupplicantCallback = manager.stopSupplicantCallback.bind(manager);
  manager.stopSupplicantCallback = null;
  stopSupplicantCallback();
  stopSupplicantCallback = null;
}

Not sure if worth it though.

Comment 3

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.1 for mc

Address the review comments.

Comment 4

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

Comment on attachment 8420021 [details] [diff] [review]
Patch v1.1 for mc

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

Looks good, but I think it's better to wait for more feedback from bug 1007766 before landing this.

Comment 5

[Preeti Raghunath(:Preeti)]

Greg,

Appreciate your effort testing this patch.

Please check if bug 1007766 and bug 1001897 are fixed with the patch.

Comment 6

[Greg Grisco]

I'm still waiting for test results, will report back here once I have the info.

Comment 9

[Vincent Chang[:vchang][changyihsin]]

Nominate for 1.4+, see comment 32 in bug 1007766.

Comment 10

[Greg Grisco]

We haven't seen the crash with this signature reproduce since applying the patch:

[@ wpa_ctrl_recv | wifi_ctrl_recv | wifi_wait_on_socket | ICSWpaSupplicantImpl::do_wifi_wait_for_event(char const*, char*, unsigned int) ]

Comment 11

[Preeti Raghunath(:Preeti)]

1.4+

Comment 12

[Vincent Chang[:vchang][changyihsin]]

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

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f638c7fb76d1

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/498d2bc8e5b5

Comment 15

[cafbot (PoC: ggrisco)]

Crash observed on: 

Device: msm8226
Gonk Version: AU_LINUX_GECKO_B2G_KK_3.5.01.04.00.113.091
Moz BuildID: 20140505000201
B2G Version: 1.4
Gecko Version: 30.0
Gaia:  http://git.mozilla.org/?p=releases/gaia.git;a=commit;h=fccb15d6940db51615545574877a62d69406b1c2
Gecko: http://git.mozilla.org/?p=releases/gecko.git;a=commit;h=3ed3bbf1941e608bc9630942c7063a8b818f36bc

Comment 16

[cafbot (PoC: ggrisco)]

Crash observed on: 

Device: msm8226
Gonk Version: AU_LINUX_GECKO_B2G_KK_3.5.01.04.00.113.091
Moz BuildID: 20140505000201
B2G Version: 1.4
Gecko Version: 30.0
Gaia:  http://git.mozilla.org/?p=releases/gaia.git;a=commit;h=fccb15d6940db51615545574877a62d69406b1c2
Gecko: http://git.mozilla.org/?p=releases/gecko.git;a=commit;h=3ed3bbf1941e608bc9630942c7063a8b818f36bc

Comment 17

[Greg Grisco]

please ignore comments 15 and 16.

Comment 18

[cafbot (PoC: ggrisco)]

Crash observed on: 

Device: msm8226
Gonk Version: AU_LINUX_GECKO_B2G_KK_3.5.01.04.00.113.091
Moz BuildID: 20140505000201
B2G Version: 1.4
Gecko Version: 30.0
Gaia:  http://git.mozilla.org/?p=releases/gaia.git;a=commit;h=fccb15d6940db51615545574877a62d69406b1c2
Gecko: http://git.mozilla.org/?p=releases/gecko.git;a=commit;h=3ed3bbf1941e608bc9630942c7063a8b818f36bc

Comment 19

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to Greg Grisco from comment #17)
> please ignore comments 15 and 16.

Comment 18 too, then, I assume?