shill: wifi: let interface proxy outlive "started"
Disabling the WiFi interface leads to Stop(), which tells wpa_supplicant
to disconnect and tear down the interface; however we don't wait to hear
anything back from wpa_supplicant (e.g., PropertiesChanged -> update
disconnect reason metrics), because we immediately destruct the
SupplicantInterfaceProxy.
Instead, retain the interface proxy until the next Start(). This should
give us time to process any property events, and is otherwise safe
(we're mostly just logging metrics). Add a few enabled() guards on
SupplicantEventDelegateInterface callbacks, so we don't try to
* stash scan results until the next Start()
* perform another full Disconnect (via CurrentBSSChanged()) after we've
already cleanly torn things down (Stop())
NB: without this change,
network_WiFi_DisconnectReason.disable_client_wifi would fail 100%,
except that b/172219209 causes a lot of false-PASS results.
BUG=b:172217291
TEST=`truncate -s 0 /var/log/net.log` (to work around b/172219209)
+ network_WiFi_DisconnectReason.disable_client_wifi
Disallow-Recycled-Builds: test-failures
Change-Id: Ic6febe34506a7afb568303c9072bf2159992f788
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2594267
Tested-by: Brian Norris <briannorris@chromium.org>
Commit-Queue: Brian Norris <briannorris@chromium.org>
Reviewed-by: Matthew Wang <matthewmwang@chromium.org>
diff --git a/shill/wifi/wifi.cc b/shill/wifi/wifi.cc
index 8cb4a4b..c0888dd 100644
--- a/shill/wifi/wifi.cc
+++ b/shill/wifi/wifi.cc
@@ -260,8 +260,6 @@
if (supplicant_present_ && supplicant_interface_proxy_) {
supplicant_process_proxy()->RemoveInterface(supplicant_interface_path_);
}
- supplicant_interface_path_ = RpcIdentifier("");
- SetSupplicantInterfaceProxy(nullptr);
pending_scan_results_.reset();
current_service_ = nullptr; // breaks a reference cycle
pending_service_ = nullptr; // breaks a reference cycle
@@ -303,6 +301,12 @@
void WiFi::AddPendingScanResult(const RpcIdentifier& path,
const KeyValueStore& properties,
bool is_removal) {
+ // BSS events might come immediately after Stop(). Don't bother stashing them
+ // at all.
+ if (!enabled()) {
+ return;
+ }
+
if (!pending_scan_results_) {
pending_scan_results_.reset(new PendingScanResults(
Bind(&WiFi::PendingScanResultsHandler,
@@ -344,10 +348,9 @@
SLOG(this, 2) << __func__;
// Called from D-Bus signal handler, but may need to send a D-Bus
// message. So defer work to event loop.
- dispatcher()->PostTask(
- FROM_HERE,
- Bind(&WiFi::PropertiesChangedTask,
- weak_ptr_factory_while_started_.GetWeakPtr(), properties));
+ dispatcher()->PostTask(FROM_HERE,
+ Bind(&WiFi::PropertiesChangedTask,
+ weak_ptr_factory_.GetWeakPtr(), properties));
}
void WiFi::ScanDone(const bool& success) {
@@ -355,6 +358,11 @@
// processor.
LOG(INFO) << __func__;
+ if (!enabled()) {
+ SLOG(this, 2) << "Ignoring scan completion while disabled";
+ return;
+ }
+
// Defer handling of scan result processing, because that processing
// may require the the registration of new D-Bus objects. And such
// registration can't be done in the context of a D-Bus signal
@@ -1571,6 +1579,11 @@
}
void WiFi::CertificationTask(const KeyValueStore& properties) {
+ // Events may come immediately after Stop().
+ if (!enabled()) {
+ return;
+ }
+
if (!current_service_) {
LOG(ERROR) << "WiFi " << link_name() << " " << __func__
<< " with no current service.";
@@ -1585,6 +1598,11 @@
}
void WiFi::EAPEventTask(const string& status, const string& parameter) {
+ // Events may come immediately after Stop().
+ if (!enabled()) {
+ return;
+ }
+
if (!current_service_) {
LOG(ERROR) << "WiFi " << link_name() << " " << __func__
<< " with no current service.";
@@ -1620,8 +1638,10 @@
// Note that order matters here. In particular, we want to process
// changes in the current BSS before changes in state. This is so
// that we update the state of the correct Endpoint/Service.
- if (properties.Contains<RpcIdentifier>(
- WPASupplicant::kInterfacePropertyCurrentBSS)) {
+ // Also note that events may occur (briefly) after Stop(), so we need to make
+ // explicit decisions here on what to do when !enabled().
+ if (enabled() && properties.Contains<RpcIdentifier>(
+ WPASupplicant::kInterfacePropertyCurrentBSS)) {
CurrentBSSChanged(properties.Get<RpcIdentifier>(
WPASupplicant::kInterfacePropertyCurrentBSS));
}
@@ -2612,14 +2632,14 @@
<< " supplicant: " << (supplicant_present_ ? "present" : "absent")
<< " proxy: "
<< (supplicant_interface_proxy_.get() ? "non-null" : "null");
- // The check for |supplicant_interface_proxy_| is mainly for testing,
- // to avoid recreation of supplicant interface proxy.
- if (!enabled() || !supplicant_present_ || supplicant_interface_proxy_) {
+ if (!enabled() || !supplicant_present_) {
return;
}
OnWiFiDebugScopeChanged(
ScopeLogger::GetInstance()->IsScopeEnabled(ScopeLogger::kWiFi));
+ RpcIdentifier previous_supplicant_interface_path(supplicant_interface_path_);
+
KeyValueStore create_interface_args;
create_interface_args.Set<string>(WPASupplicant::kInterfacePropertyName,
link_name());
@@ -2660,9 +2680,23 @@
<< supplicant_connect_attempts_;
metrics()->NotifyWiFiSupplicantSuccess(supplicant_connect_attempts_);
- SetSupplicantInterfaceProxy(
- control_interface()->CreateSupplicantInterfaceProxy(
- this, supplicant_interface_path_));
+ // Only (re)create the interface proxy if its D-Bus path changed, or if we
+ // haven't created one yet. This lets us watch interface properties
+ // immediately after Stop() (e.g., for metrics collection) and also allows
+ // tests to skip recreation (by retaining the same interface path).
+ if (!supplicant_interface_proxy_ ||
+ previous_supplicant_interface_path != supplicant_interface_path_) {
+ SLOG(this, 2) << StringPrintf(
+ "Updating interface path from \"%s\" to \"%s\"",
+ previous_supplicant_interface_path.value().c_str(),
+ supplicant_interface_path_.value().c_str());
+ SetSupplicantInterfaceProxy(
+ control_interface()->CreateSupplicantInterfaceProxy(
+ this, supplicant_interface_path_));
+ } else {
+ SLOG(this, 2) << "Reusing existing interface at "
+ << supplicant_interface_path_.value();
+ }
RTNLHandler::GetInstance()->SetInterfaceFlags(interface_index(), IFF_UP,
IFF_UP);
diff --git a/shill/wifi/wifi_test.cc b/shill/wifi/wifi_test.cc
index 9acf570..028eb4a 100644
--- a/shill/wifi/wifi_test.cc
+++ b/shill/wifi/wifi_test.cc
@@ -662,6 +662,10 @@
EXPECT_EQ(method, wifi_->scan_method_);
}
+ void PropertiesChanged(const KeyValueStore& props) {
+ wifi_->PropertiesChanged(props);
+ }
+
protected:
using MockWiFiServiceRefPtr = scoped_refptr<MockWiFiService>;
@@ -1187,6 +1191,7 @@
std::unique_ptr<SupplicantInterfaceProxyInterface>
CreateSupplicantInterfaceProxy(SupplicantEventDelegateInterface* delegate,
const RpcIdentifier& object_path) {
+ CHECK(supplicant_interface_proxy_);
return std::move(supplicant_interface_proxy_);
}
@@ -2154,6 +2159,23 @@
EXPECT_EQ(nullptr, GetCurrentService());
}
+TEST_F(WiFiMainTest, StopDisconnectReason) {
+ StartWiFi();
+
+ KeyValueStore props;
+ props.Set<int32_t>(WPASupplicant::kInterfacePropertyDisconnectReason,
+ -IEEE_80211::kReasonCodeSenderHasLeft);
+
+ PropertiesChanged(props);
+ StopWiFi();
+ EXPECT_CALL(*metrics(),
+ Notify80211Disconnect(Metrics::kDisconnectedNotByAp,
+ IEEE_80211::kReasonCodeSenderHasLeft));
+
+ event_dispatcher_->DispatchPendingEvents();
+ Mock::VerifyAndClearExpectations(GetSupplicantInterfaceProxy());
+}
+
TEST_F(WiFiMainTest, ReconnectTimer) {
StartWiFi();
MockWiFiServiceRefPtr service(
@@ -3377,6 +3399,8 @@
}
TEST_F(WiFiMainTest, EAPCertification) {
+ StartWiFi();
+
MockWiFiServiceRefPtr service = MakeMockService(kSecurity8021x);
EXPECT_CALL(*service, AddEAPCertification(_, _)).Times(0);
@@ -3408,6 +3432,8 @@
}
TEST_F(WiFiTimerTest, ScanDoneDispatchesTasks) {
+ SetWiFiEnabled(true);
+
// Dispatch WiFi::ScanFailedTask if scan failed.
EXPECT_TRUE(ScanFailedCallbackIsCancelled());
EXPECT_CALL(*mock_dispatcher_,
