From fc52868213d44318f169a64f91ce8d8ae134ba82 Mon Sep 17 00:00:00 2001
From: Hungming Chen <nuccachen@google.com>
Date: Mon, 16 Jan 2023 20:44:34 +0800
Subject: [PATCH] Tethering: ignore duplicate upstream changed event

Different network objects with the same network ID should be
treated as the same network.

chooseUpstreamType compares the current upstream and new upstream
by object (comparison operator) instead of network id
(Network#equals). This implies that different objects with the
same network id still trigger upstream changed report.

Since this commit, reportUpstreamChanged is only called when upsteam
has really changed (connected, switched or disconnected) in
chooseUpstreamType. In previous code, reportUpstreamChanged is also
called if EVENT_ON_CAPABILITIES is called when the upstream is the
same but its capabilities changed.

The NotificationUpdater#onUpstreamCapabilitiesChanged method only
needs to be called by chooseUpstreamType when it chooses a new
upstream. If the upstream remains the same but its capabilities
change, the EVENT_ON_CAPABILITIES will call
onUpstreamCapabilitiesChanged.

Bug: 243516306
Test: atest TetheringTest
Change-Id: I009383a61a5fabd249ba78fcffd524a5bbe4602e
---
 .../android/networkstack/tethering/Tethering.java   |  8 +++++---
 .../networkstack/tethering/TetheringTest.java       | 13 +++++++------
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/Tethering/src/com/android/networkstack/tethering/Tethering.java b/Tethering/src/com/android/networkstack/tethering/Tethering.java
index e5f644ed4a..b371178244 100644
--- a/Tethering/src/com/android/networkstack/tethering/Tethering.java
+++ b/Tethering/src/com/android/networkstack/tethering/Tethering.java
@@ -157,6 +157,7 @@ import java.util.Collection;
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.Executor;
@@ -1845,11 +1846,12 @@ public class Tethering {
 
             setUpstreamNetwork(ns);
             final Network newUpstream = (ns != null) ? ns.network : null;
-            if (mTetherUpstream != newUpstream) {
+            if (!Objects.equals(mTetherUpstream, newUpstream)) {
                 mTetherUpstream = newUpstream;
                 reportUpstreamChanged(mTetherUpstream);
-                // Need to notify capabilities change after upstream network changed because new
-                // network's capabilities should be checked every time.
+                // Need to notify capabilities change after upstream network changed because
+                // upstream may switch to existing network which don't have
+                // UpstreamNetworkMonitor.EVENT_ON_CAPABILITIES callback.
                 mNotificationUpdater.onUpstreamCapabilitiesChanged(
                         (ns != null) ? ns.networkCapabilities : null);
             }
diff --git a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java
index 687171ba5f..2280e38f8e 100644
--- a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java
+++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java
@@ -2685,10 +2685,9 @@ public class TetheringTest {
         final UpstreamNetworkState upstreamState2 = buildMobileIPv4UpstreamState();
         initTetheringUpstream(upstreamState2);
         stateMachine.chooseUpstreamType(true);
-        // Bug: duplicated upstream change event.
-        mTetheringEventCallback.expectUpstreamChanged(upstreamState2.network);
-        inOrder.verify(mNotificationUpdater)
-                .onUpstreamCapabilitiesChanged(upstreamState2.networkCapabilities);
+        // Expect that no upstream change event and capabilities changed event.
+        mTetheringEventCallback.assertNoUpstreamChangeCallback();
+        inOrder.verify(mNotificationUpdater, never()).onUpstreamCapabilitiesChanged(any());
 
         // Set the upstream with the same network ID but different object and different capability.
         final UpstreamNetworkState upstreamState3 = buildMobileIPv4UpstreamState();
@@ -2696,11 +2695,13 @@ public class TetheringTest {
         upstreamState3.networkCapabilities.addCapability(NET_CAPABILITY_VALIDATED);
         initTetheringUpstream(upstreamState3);
         stateMachine.chooseUpstreamType(true);
-        // Bug: duplicated upstream change event.
-        mTetheringEventCallback.expectUpstreamChanged(upstreamState3.network);
+        // Expect that no upstream change event and capabilities changed event.
+        mTetheringEventCallback.assertNoUpstreamChangeCallback();
+        stateMachine.handleUpstreamNetworkMonitorCallback(EVENT_ON_CAPABILITIES, upstreamState3);
         inOrder.verify(mNotificationUpdater)
                 .onUpstreamCapabilitiesChanged(upstreamState3.networkCapabilities);
 
+
         // Lose upstream.
         initTetheringUpstream(null);
         stateMachine.chooseUpstreamType(true);