From 78b0d4b2a64b78033e8c3d5c76cc0a1b2ebc41ab Mon Sep 17 00:00:00 2001
From: Chiachang Wang <chiachangwang@google.com>
Date: Mon, 3 Dec 2018 17:06:54 +0800
Subject: [PATCH] Re-factor state machine in NetworkMonitor

In ProbingState, most events are deferred and never exited if
the probe complete message indicated that the probe timed out or
failed. It means it will need to remember to explicitly return
HANDLED or NOT_HANDLED in the ProbingState for every new
message. Thus, re-factor the design for better architecture.

Test: - atest FrameworksNetTests
      - manually test for state transition
Bug: 120014928
Change-Id: I18500b958b35383335fcdef6af4e08dbbdfdffb0
---
 .../server/connectivity/NetworkMonitor.java   | 49 +++++++++++++------
 1 file changed, 33 insertions(+), 16 deletions(-)

diff --git a/services/core/java/com/android/server/connectivity/NetworkMonitor.java b/services/core/java/com/android/server/connectivity/NetworkMonitor.java
index c2f4406c615d9..bf95210195b79 100644
--- a/services/core/java/com/android/server/connectivity/NetworkMonitor.java
+++ b/services/core/java/com/android/server/connectivity/NetworkMonitor.java
@@ -313,6 +313,7 @@ public class NetworkMonitor extends StateMachine {
     private final State mCaptivePortalState = new CaptivePortalState();
     private final State mEvaluatingPrivateDnsState = new EvaluatingPrivateDnsState();
     private final State mProbingState = new ProbingState();
+    private final State mWaitingForNextProbeState = new WaitingForNextProbeState();
 
     private CustomIntentReceiver mLaunchCaptivePortalAppBroadcastReceiver = null;
 
@@ -368,6 +369,7 @@ public class NetworkMonitor extends StateMachine {
         addState(mMaybeNotifyState, mDefaultState);
             addState(mEvaluatingState, mMaybeNotifyState);
                 addState(mProbingState, mEvaluatingState);
+                addState(mWaitingForNextProbeState, mEvaluatingState);
             addState(mCaptivePortalState, mMaybeNotifyState);
         addState(mEvaluatingPrivateDnsState, mDefaultState);
         addState(mValidatedState, mDefaultState);
@@ -877,6 +879,11 @@ public class NetworkMonitor extends StateMachine {
 
         @Override
         public void enter() {
+            if (mEvaluateAttempts >= BLAME_FOR_EVALUATION_ATTEMPTS) {
+                //Don't continue to blame UID forever.
+                TrafficStats.clearThreadStatsUid();
+            }
+
             final int token = ++mProbeToken;
             mThread = new Thread(() -> sendMessage(obtainMessage(CMD_PROBE_COMPLETE, token, 0,
                     isCaptivePortal())));
@@ -904,29 +911,16 @@ public class NetworkMonitor extends StateMachine {
                         mLastPortalProbeResult = probeResult;
                         transitionTo(mCaptivePortalState);
                     } else {
-                        final Message msg = obtainMessage(CMD_REEVALUATE, ++mReevaluateToken, 0);
-                        sendMessageDelayed(msg, mReevaluateDelayMs);
                         logNetworkEvent(NetworkEvent.NETWORK_VALIDATION_FAILED);
                         notifyNetworkTestResultInvalid(probeResult.redirectUrl);
-                        if (mEvaluateAttempts >= BLAME_FOR_EVALUATION_ATTEMPTS) {
-                            // Don't continue to blame UID forever.
-                            TrafficStats.clearThreadStatsUid();
-                        }
-                        mReevaluateDelayMs *= 2;
-                        if (mReevaluateDelayMs > MAX_REEVALUATE_DELAY_MS) {
-                            mReevaluateDelayMs = MAX_REEVALUATE_DELAY_MS;
-                        }
+                        transitionTo(mWaitingForNextProbeState);
                     }
                     return HANDLED;
-                case CMD_REEVALUATE:
-                    // Leave the event to EvaluatingState. Defer this message will result in reset
-                    // of mReevaluateDelayMs and mEvaluateAttempts.
-                case CMD_NETWORK_DISCONNECTED:
                 case EVENT_DNS_NOTIFICATION:
+                    // Leave the event to DefaultState to record correct dns timestamp.
                     return NOT_HANDLED;
                 default:
-                    // TODO: Some events may able to handle in this state, instead of deferring to
-                    // next state.
+                    // Wait for probe result and defer events to next state by default.
                     deferMessage(message);
                     return HANDLED;
             }
@@ -941,6 +935,29 @@ public class NetworkMonitor extends StateMachine {
         }
     }
 
+    // Being in the WaitingForNextProbeState indicates that evaluating probes failed and state is
+    // transited from ProbingState. This ensures that the state machine is only in ProbingState
+    // while a probe is in progress, not while waiting to perform the next probe. That allows
+    // ProbingState to defer most messages until the probe is complete, which keeps the code simple
+    // and matches the pre-Q behaviour where probes were a blocking operation performed on the state
+    // machine thread.
+    private class WaitingForNextProbeState extends State {
+        @Override
+        public void enter() {
+            final Message msg = obtainMessage(CMD_REEVALUATE, ++mReevaluateToken, 0);
+            sendMessageDelayed(msg, mReevaluateDelayMs);
+            mReevaluateDelayMs *= 2;
+            if (mReevaluateDelayMs > MAX_REEVALUATE_DELAY_MS) {
+                mReevaluateDelayMs = MAX_REEVALUATE_DELAY_MS;
+            }
+        }
+
+        @Override
+        public boolean processMessage(Message message) {
+            return NOT_HANDLED;
+        }
+    }
+
     // Limits the list of IP addresses returned by getAllByName or tried by openConnection to at
     // most one per address family. This ensures we only wait up to 20 seconds for TCP connections
     // to complete, regardless of how many IP addresses a host has.