[M110] Fix dangling pointer crash around connection_warning_icon_

NetworkListViewControllerImpl's `connection_warning_icon_` is owned by
`connection_warning_`. It was possible to end up in a state where
`connection_warning_` is deleted and `connection_warning_icon_` points
to invalid memory.

Fix this crash.

Also add a regression test which crashes without the fix. The test
requires changing FakeCrosNetworkConfig to support non-active
networks to allow simulating disconnecting and reconnecting a network.

(cherry picked from commit 2251d02)

Bug: b:263803248
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4128614
Reviewed-by: Gordon Seto <gordonseto@google.com>
Commit-Queue: Gordon Seto <gordonseto@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#1087414}
Change-Id: I448ad6ca7ae6b99bba5d14f05834790b542a5df2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4142560
Commit-Queue: Pavol Marko <pmarko@chromium.org>
Cr-Commit-Position: refs/branch-heads/5481@{#186}
Cr-Branched-From: 130f3e4-refs/heads/main@{#1084008}

ash/system/network/network_list_view_controller_impl.cc

@@ -350,7 +350,7 @@ size_t NetworkListViewControllerImpl::ShowConnectionWarningIfNetworkMonitored(
     network_detailed_network_view()->network_list()->ReorderChildView(
         connection_warning_, index++);
   } else if (connected_vpn_guid_.empty() && !using_proxy) {
-    RemoveAndResetViewIfExists(&connection_warning_);
+    HideConnectionWarning();
   }
 
   return index;
@@ -398,6 +398,13 @@ void NetworkListViewControllerImpl::MaybeShowConnectionWarningManagedIcon(
 void NetworkListViewControllerImpl::OnGetManagedPropertiesResult(
     const std::string& guid,
     ManagedPropertiesPtr properties) {
+  // Bail out early if no connection warning is being shown.
+  // This could happen if the connection warning is hidden while the async
+  // GetManagedProperties step is in progress.
+  if (!connection_warning_) {
+    return;
+  }
+
   // Check if the proxy is managed.
   const NetworkStateProperties* default_network = GetDefaultNetwork();
   if (default_network && default_network->guid == guid) {
@@ -855,6 +862,13 @@ void NetworkListViewControllerImpl::ShowConnectionWarning(
           std::move(connection_warning));
 }
 
+void NetworkListViewControllerImpl::HideConnectionWarning() {
+  // If `connection_warning_icon_` existed, it must be cleared first because
+  // `connection_warning_` owns it.
+  RemoveAndResetViewIfExists(&connection_warning_icon_);
+  RemoveAndResetViewIfExists(&connection_warning_);
+}
+
 void NetworkListViewControllerImpl::UpdateScanningBarAndTimer() {
   if (is_wifi_enabled_ && !network_scan_repeating_timer_.IsRunning())
     ScanAndStartTimer();

ash/system/network/network_list_view_controller_impl.h

@@ -159,6 +159,9 @@ class ASH_EXPORT NetworkListViewControllerImpl
   // secure DNS template URIs contain identifiers.
   void ShowConnectionWarning(bool show_managed_icon);
 
+  // Hides a connection warning, if visible.
+  void HideConnectionWarning();
+
   // Determines whether a scan for WiFi and Tether networks should be requested
   // and updates the scanning bar accordingly.
   void UpdateScanningBarAndTimer();

ash/system/network/network_list_view_controller_unittest.cc

@@ -1319,6 +1319,53 @@ TEST_P(NetworkListViewControllerTest, ConnectionWarningManagedIconProxy) {
   EXPECT_TRUE(IsManagedIcon(icon));
 }
 
+// Disconnect and re-connect a network that shows a warning.
+// Regression test for b/263803248.
+TEST_P(NetworkListViewControllerTest, ConnectionWarningDisconnectReconnect) {
+  EXPECT_THAT(GetConnectionWarning(), IsNull());
+
+  SetDefaultNetworkForTesting(GetDefaultNetworkWithProxy(kWifiName));
+  SetManagedNetworkPropertiesForTesting(GetManagedNetworkPropertiesWithProxy(
+      /*is_managed=*/true));
+  AddWifiDevice();
+
+  ASSERT_THAT(GetConnectionWarning(), NotNull());
+  ASSERT_THAT(GetConnectionLabelView(), NotNull());
+  EXPECT_EQ(
+      l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_NETWORK_MANAGED_WARNING),
+      GetConnectionLabelView()->GetText());
+
+  {
+    views::ImageView* icon = GetConnectionWarningIcon();
+    ASSERT_THAT(icon, NotNull());
+    EXPECT_TRUE(IsManagedIcon(icon));
+  }
+
+  // Disconnect the network and check that no warning is shown.
+  SetDefaultNetworkForTesting(nullptr);
+  SetManagedNetworkPropertiesForTesting(nullptr);
+  network_state_helper()->ClearDevices();
+  EXPECT_THAT(GetConnectionWarning(), IsNull());
+
+  // Reconnect the network. This should not crash (regression test for
+  // b/263803248). Afterwards, the warning should be shown again.
+  SetDefaultNetworkForTesting(GetDefaultNetworkWithProxy(kWifiName));
+  SetManagedNetworkPropertiesForTesting(GetManagedNetworkPropertiesWithProxy(
+      /*is_managed=*/true));
+  AddWifiDevice();
+
+  ASSERT_THAT(GetConnectionWarning(), NotNull());
+  ASSERT_THAT(GetConnectionLabelView(), NotNull());
+  EXPECT_EQ(
+      l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_NETWORK_MANAGED_WARNING),
+      GetConnectionLabelView()->GetText());
+  {
+    views::ImageView* icon = GetConnectionWarningIcon();
+    ASSERT_THAT(icon, NotNull());
+    EXPECT_TRUE(IsManagedIcon(icon));
+  }
+}
+
 TEST_P(NetworkListViewControllerTest,
        ConnectionWarningDnsTemplateUriWithIdentifier) {
   EXPECT_THAT(GetConnectionWarning(), IsNull());