Fix a loss of data.

Fix a bug in DataConnection state machine where the notification
of disconnection completion was sent before we actually transitioned
to the in active state. Also, change conn.reset to send a response
so the user can know when the transition to the in active state
completes for that also.

bug: 2471897
Change-Id: I5776324ac89a607925d07f4a600bc5b34c3f3ed6

4 files changed, 177 insertions, 31 deletions

diff --git a/telephony/java/com/android/internal/telephony/DataConnection.java b/telephony/java/com/android/internal/telephony/DataConnection.java
index 562a9f8..521072e 100644
--- a/telephony/java/com/android/internal/telephony/DataConnection.java
+++ b/telephony/java/com/android/internal/telephony/DataConnection.java
@@ -51,7 +51,7 @@ import android.util.EventLog;
  *
  * DataConnection {
  *   + mDefaultState {
- *        EVENT_RESET { clearSettings, >mInactiveState }.
+ *        EVENT_RESET { clearSettings, notifiyDisconnectCompleted, >mInactiveState }.
  *        EVENT_CONNECT {  notifyConnectCompleted(FailCause.UNKNOWN) }.
  *        EVENT_DISCONNECT { notifyDisconnectCompleted }.
  *
@@ -60,8 +60,10 @@ import android.util.EventLog;
  *        EVENT_GET_LAST_FAIL_DONE,
  *        EVENT_DEACTIVATE_DONE.
  *     }
- *   ++ # mInactiveState {
- *            EVENT_RESET.
+ *   ++ # mInactiveState 
+ *        e(doNotifications)
+ *        x(clearNotifications) {
+ *            EVENT_RESET { notifiyDisconnectCompleted }.
  *            EVENT_CONNECT {startConnecting, >mActivatingState }.
  *        }
  *   ++   mActivatingState {
@@ -338,6 +340,8 @@ public abstract class DataConnection extends HierarchicalStateMachine {
         if (DBG) log("NotifyDisconnectCompleted");
 
         Message msg = dp.onCompletedMsg;
+        log(String.format("msg.what=%d msg.obj=%s",
+                msg.what, ((msg.obj instanceof String) ? (String) msg.obj : "<no-reason>")));
         AsyncResult.forMessage(msg);
         msg.sendToTarget();
 
@@ -437,6 +441,9 @@ public abstract class DataConnection extends HierarchicalStateMachine {
                 case EVENT_RESET:
                     if (DBG) log("DcDefaultState: msg.what=EVENT_RESET");
                     clearSettings();
+                    if (msg.obj != null) {
+                        notifyDisconnectCompleted((DisconnectParams) msg.obj);
+                    }
                     transitionTo(mInactiveState);
                     break;
 
@@ -467,9 +474,48 @@ public abstract class DataConnection extends HierarchicalStateMachine {
      * The state machine is inactive and expects a EVENT_CONNECT.
      */
     private class DcInactiveState extends HierarchicalState {
+        private ConnectionParams mConnectionParams = null;
+        private FailCause mFailCause = null;
+        private DisconnectParams mDisconnectParams = null;
+
+        public void setEnterNotificationParams(ConnectionParams cp, FailCause cause) {
+            log("DcInactiveState: setEnterNoticationParams cp,cause");
+            mConnectionParams = cp;
+            mFailCause = cause;
+        }
+
+        public void setEnterNotificationParams(DisconnectParams dp) {
+          log("DcInactiveState: setEnterNoticationParams dp");
+            mDisconnectParams = dp;
+        }
+
         @Override protected void enter() {
             mTag += 1;
+
+            /**
+             * Now that we've transitioned to Inactive state we
+             * can send notifications. Previously we sent the
+             * notifications in the processMessage handler but
+             * that caused a race condition because the synchronous
+             * call to isInactive.
+             */
+            if ((mConnectionParams != null) && (mFailCause != null)) {
+                log("DcInactiveState: enter notifyConnectCompleted");
+                notifyConnectCompleted(mConnectionParams, mFailCause);
+            }
+            if (mDisconnectParams != null) {
+              log("DcInactiveState: enter notifyDisconnectCompleted");
+                notifyDisconnectCompleted(mDisconnectParams);
+            }
+        }
+
+        @Override protected void exit() {
+            // clear notifications
+            mConnectionParams = null;
+            mFailCause = null;
+            mDisconnectParams = null;
         }
+
         @Override protected boolean processMessage(Message msg) {
             boolean retVal;
 
@@ -478,6 +524,9 @@ public abstract class DataConnection extends HierarchicalStateMachine {
                     if (DBG) {
                         log("DcInactiveState: msg.what=EVENT_RESET, ignore we're already reset");
                     }
+                    if (msg.obj != null) {
+                        notifyDisconnectCompleted((DisconnectParams) msg.obj);
+                    }
                     retVal = true;
                     break;
 
@@ -526,12 +575,14 @@ public abstract class DataConnection extends HierarchicalStateMachine {
                     switch (result) {
                         case SUCCESS:
                             // All is well
-                            notifyConnectCompleted(cp, FailCause.NONE);
+                            mActiveState.setEnterNotificationParams(cp, FailCause.NONE);
                             transitionTo(mActiveState);
                             break;
                         case ERR_BadCommand:
                             // Vendor ril rejected the command and didn't connect.
-                            notifyConnectCompleted(cp, result.mFailCause);
+                            // Transition to inactive but send notifications after
+                            // we've entered the mInactive state.
+                            mInactiveState.setEnterNotificationParams(cp, result.mFailCause);
                             transitionTo(mInactiveState);
                             break;
                         case ERR_BadDns:
@@ -565,7 +616,9 @@ public abstract class DataConnection extends HierarchicalStateMachine {
                             int rilFailCause = ((int[]) (ar.result))[0];
                             cause = getFailCauseFromRequest(rilFailCause);
                         }
-                         notifyConnectCompleted(cp, cause);
+                        // Transition to inactive but send notifications after
+                        // we've entered the mInactive state.
+                         mInactiveState.setEnterNotificationParams(cp, cause);
                          transitionTo(mInactiveState);
                     } else {
                         if (DBG) {
@@ -591,6 +644,35 @@ public abstract class DataConnection extends HierarchicalStateMachine {
      * The state machine is connected, expecting an EVENT_DISCONNECT.
      */
     private class DcActiveState extends HierarchicalState {
+        private ConnectionParams mConnectionParams = null;
+        private FailCause mFailCause = null;
+
+        public void setEnterNotificationParams(ConnectionParams cp, FailCause cause) {
+            log("DcInactiveState: setEnterNoticationParams cp,cause");
+            mConnectionParams = cp;
+            mFailCause = cause;
+        }
+
+        @Override public void enter() {
+            /**
+             * Now that we've transitioned to Active state we
+             * can send notifications. Previously we sent the
+             * notifications in the processMessage handler but
+             * that caused a race condition because the synchronous
+             * call to isActive.
+             */
+            if ((mConnectionParams != null) && (mFailCause != null)) {
+                log("DcActiveState: enter notifyConnectCompleted");
+                notifyConnectCompleted(mConnectionParams, mFailCause);
+            }
+        }
+
+        @Override protected void exit() {
+            // clear notifications
+            mConnectionParams = null;
+            mFailCause = null;
+        }
+
         @Override protected boolean processMessage(Message msg) {
             boolean retVal;
 
@@ -627,7 +709,9 @@ public abstract class DataConnection extends HierarchicalStateMachine {
                     AsyncResult ar = (AsyncResult) msg.obj;
                     DisconnectParams dp = (DisconnectParams) ar.userObj;
                     if (dp.tag == mTag) {
-                        notifyDisconnectCompleted((DisconnectParams) ar.userObj);
+                        // Transition to inactive but send notifications after
+                        // we've entered the mInactive state.
+                        mInactiveState.setEnterNotificationParams((DisconnectParams) ar.userObj);
                         transitionTo(mInactiveState);
                     } else {
                         if (DBG) log("DcDisconnectState EVENT_DEACTIVATE_DONE stale dp.tag="
@@ -660,7 +744,9 @@ public abstract class DataConnection extends HierarchicalStateMachine {
                     ConnectionParams cp = (ConnectionParams) ar.userObj;
                     if (cp.tag == mTag) {
                         if (DBG) log("DcDisconnectingBadDnsState msg.what=EVENT_DEACTIVATE_DONE");
-                        notifyConnectCompleted(cp, FailCause.UNKNOWN);
+                        // Transition to inactive but send notifications after
+                        // we've entered the mInactive state.
+                        mInactiveState.setEnterNotificationParams(cp, FailCause.UNKNOWN);
                         transitionTo(mInactiveState);
                     } else {
                         if (DBG) log("DcDisconnectingBadDnsState EVENT_DEACTIVE_DONE stale dp.tag="
@@ -683,9 +769,12 @@ public abstract class DataConnection extends HierarchicalStateMachine {
 
     /**
      * Disconnect from the network.
+     *
+     * @param onCompletedMsg is sent with its msg.obj as an AsyncResult object.
+     *        With AsyncResult.userObj set to the original msg.obj.
      */
-    public void reset() {
-        sendMessage(obtainMessage(EVENT_RESET));
+    public void reset(Message onCompletedMsg) {
+        sendMessage(obtainMessage(EVENT_RESET, new DisconnectParams(onCompletedMsg)));
     }
 
     /**
@@ -726,6 +815,9 @@ public abstract class DataConnection extends HierarchicalStateMachine {
     // ****** The following are used for debugging.
 
     /**
+     * TODO: This should be an asynchronous call and we wouldn't
+     * have to use handle the notification in the DcInactiveState.enter.
+     *
      * @return true if the state machine is in the inactive state.
      */
     public boolean isInactive() {
@@ -734,7 +826,10 @@ public abstract class DataConnection extends HierarchicalStateMachine {
     }
 
     /**
-     * @return true if the state machine is in the inactive state.
+     * TODO: This should be an asynchronous call and we wouldn't
+     * have to use handle the notification in the DcActiveState.enter.
+     *
+     * @return true if the state machine is in the active state.
      */
     public boolean isActive() {
         boolean retVal = getCurrentState() == mActiveState;
diff --git a/telephony/java/com/android/internal/telephony/DataConnectionTracker.java b/telephony/java/com/android/internal/telephony/DataConnectionTracker.java
index cab7b81..e8e18a1 100644
--- a/telephony/java/com/android/internal/telephony/DataConnectionTracker.java
+++ b/telephony/java/com/android/internal/telephony/DataConnectionTracker.java
@@ -101,6 +101,7 @@ public abstract class DataConnectionTracker extends Handler {
     protected static final int EVENT_CDMA_OTA_PROVISION = 35;
     protected static final int EVENT_RESTART_RADIO = 36;
     protected static final int EVENT_SET_MASTER_DATA_ENABLE = 37;
+    protected static final int EVENT_RESET_DONE = 38;
 
     /***** Constants *****/
 
@@ -265,6 +266,7 @@ public abstract class DataConnectionTracker extends Handler {
     protected abstract void onRadioOffOrNotAvailable();
     protected abstract void onDataSetupComplete(AsyncResult ar);
     protected abstract void onDisconnectDone(AsyncResult ar);
+    protected abstract void onResetDone(AsyncResult ar);
     protected abstract void onVoiceCallStarted();
     protected abstract void onVoiceCallEnded();
     protected abstract void onCleanUpConnection(boolean tearDown, String reason);
@@ -331,6 +333,10 @@ public abstract class DataConnectionTracker extends Handler {
                 onSetDataEnabled(enabled);
                 break;
 
+            case EVENT_RESET_DONE:
+                onResetDone((AsyncResult) msg.obj);
+                break;
+
             default:
                 Log.e("DATA", "Unidentified event = " + msg.what);
                 break;
diff --git a/telephony/java/com/android/internal/telephony/cdma/CdmaDataConnectionTracker.java b/telephony/java/com/android/internal/telephony/cdma/CdmaDataConnectionTracker.java
index b5461bf..9218715 100644
--- a/telephony/java/com/android/internal/telephony/cdma/CdmaDataConnectionTracker.java
+++ b/telephony/java/com/android/internal/telephony/cdma/CdmaDataConnectionTracker.java
@@ -368,7 +368,7 @@ public final class CdmaDataConnectionTracker extends DataConnectionTracker {
      * @param reason reason for the clean up.
      */
     private void cleanUpConnection(boolean tearDown, String reason) {
-        if (DBG) log("Clean up connection due to " + reason);
+        if (DBG) log("cleanUpConnection: reason: " + reason);
 
         // Clear the reconnect alarm, if set.
         if (mReconnectIntent != null) {
@@ -380,25 +380,25 @@ public final class CdmaDataConnectionTracker extends DataConnectionTracker {
 
         setState(State.DISCONNECTING);
 
-        for (DataConnection connBase : dataConnectionList) {
-            CdmaDataConnection conn = (CdmaDataConnection) connBase;
-
+        boolean notificationDeferred = false;
+        for (DataConnection conn : dataConnectionList) {
             if(conn != null) {
                 if (tearDown) {
-                    Message msg = obtainMessage(EVENT_DISCONNECT_DONE, reason);
-                    conn.disconnect(msg);
+                    if (DBG) log("cleanUpConnection: teardown, call conn.disconnect");
+                    conn.disconnect(obtainMessage(EVENT_DISCONNECT_DONE, reason));
                 } else {
-                    conn.reset();
+                    if (DBG) log("cleanUpConnection: !tearDown, call conn.reset");
+                    conn.reset(obtainMessage(EVENT_RESET_DONE, reason));
                 }
+                notificationDeferred = true;
             }
         }
 
         stopNetStatPoll();
 
-        if (!tearDown) {
-            setState(State.IDLE);
-            phone.notifyDataConnection(reason);
-            mIsApnActive = false;
+        if (!notificationDeferred) {
+            if (DBG) log("cleanupConnection: !tearDown && !resettingConn");
+            gotoIdleAndNotifyDataConnection(reason);
         }
     }
 
@@ -622,6 +622,13 @@ public final class CdmaDataConnectionTracker extends DataConnectionTracker {
         setState(State.FAILED);
     }
 
+    private void gotoIdleAndNotifyDataConnection(String reason) {
+        if (DBG) log("gotoIdleAndNotifyDataConnection: reason=" + reason);
+        setState(State.IDLE);
+        phone.notifyDataConnection(reason);
+        mIsApnActive = false;
+    }
+
     protected void onRecordsLoaded() {
         if (state == State.FAILED) {
             cleanUpConnection(false, null);
@@ -731,7 +738,7 @@ public final class CdmaDataConnectionTracker extends DataConnectionTracker {
     }
 
     /**
-     * @override com.android.internal.telephony.DataConnectionTracker
+     * Called when EVENT_DISCONNECT_DONE is received.
      */
     protected void onDisconnectDone(AsyncResult ar) {
         if(DBG) log("EVENT_DISCONNECT_DONE");
@@ -762,6 +769,20 @@ public final class CdmaDataConnectionTracker extends DataConnectionTracker {
     }
 
     /**
+     * Called when EVENT_RESET_DONE is received so goto
+     * IDLE state and send notifications to those interested.
+     */
+    @Override
+    protected void onResetDone(AsyncResult ar) {
+      if (DBG) log("EVENT_RESET_DONE");
+      String reason = null;
+      if (ar.userObj instanceof String) {
+          reason = (String) ar.userObj;
+      }
+      gotoIdleAndNotifyDataConnection(reason);
+    }
+
+    /**
      * @override com.android.internal.telephony.DataConnectionTracker
      */
     protected void onVoiceCallStarted() {
diff --git a/telephony/java/com/android/internal/telephony/gsm/GsmDataConnectionTracker.java b/telephony/java/com/android/internal/telephony/gsm/GsmDataConnectionTracker.java
index 30beaaa..f26e54e 100644
--- a/telephony/java/com/android/internal/telephony/gsm/GsmDataConnectionTracker.java
+++ b/telephony/java/com/android/internal/telephony/gsm/GsmDataConnectionTracker.java
@@ -502,20 +502,22 @@ public final class GsmDataConnectionTracker extends DataConnectionTracker {
 
         setState(State.DISCONNECTING);
 
+        boolean notificationDeferred = false;
         for (DataConnection conn : pdpList) {
             if (tearDown) {
-                Message msg = obtainMessage(EVENT_DISCONNECT_DONE, reason);
-                conn.disconnect(msg);
+                if (DBG) log("cleanUpConnection: teardown, call conn.disconnect");
+                conn.disconnect(obtainMessage(EVENT_DISCONNECT_DONE, reason));
             } else {
-                conn.reset();
+                if (DBG) log("cleanUpConnection: !tearDown, call conn.reset");
+                conn.reset(obtainMessage(EVENT_RESET_DONE, reason));
             }
+            notificationDeferred = true;
         }
         stopNetStatPoll();
 
-        if (!tearDown) {
-            setState(State.IDLE);
-            phone.notifyDataConnection(reason);
-            mActiveApn = null;
+        if (!notificationDeferred) {
+            if (DBG) log("cleanupConnection: !tearDown && !resettingConn");
+            gotoIdleAndNotifyDataConnection(reason);
         }
     }
 
@@ -749,6 +751,13 @@ public final class GsmDataConnectionTracker extends DataConnectionTracker {
         mReregisterOnReconnectFailure = false;
     }
 
+    private void gotoIdleAndNotifyDataConnection(String reason) {
+        if (DBG) log("gotoIdleAndNotifyDataConnection: reason=" + reason);
+        setState(State.IDLE);
+        phone.notifyDataConnection(reason);
+        mActiveApn = null;
+    }
+
     /**
      * This is a kludge to deal with the fact that
      * the PDP state change notification doesn't always work
@@ -1172,6 +1181,9 @@ public final class GsmDataConnectionTracker extends DataConnectionTracker {
         }
     }
 
+    /**
+     * Called when EVENT_DISCONNECT_DONE is received.
+     */
     protected void onDisconnectDone(AsyncResult ar) {
         String reason = null;
         if(DBG) log("EVENT_DISCONNECT_DONE");
@@ -1186,6 +1198,19 @@ public final class GsmDataConnectionTracker extends DataConnectionTracker {
         }
     }
 
+    /**
+     * Called when EVENT_RESET_DONE is received.
+     */
+    @Override
+    protected void onResetDone(AsyncResult ar) {
+        if (DBG) log("EVENT_RESET_DONE");
+        String reason = null;
+        if (ar.userObj instanceof String) {
+            reason = (String) ar.userObj;
+        }
+        gotoIdleAndNotifyDataConnection(reason);
+    }
+
     protected void onPollPdp() {
         if (state == State.CONNECTED) {
             // only poll when connected
@@ -1487,5 +1512,4 @@ public final class GsmDataConnectionTracker extends DataConnectionTracker {
     protected void log(String s) {
         Log.d(LOG_TAG, "[GsmDataConnectionTracker] " + s);
     }
-
 }