Merge "WifiVendorHal: improve handling of debug events" into oc-dev

2 files changed, 57 insertions, 17 deletions

diff --git a/service/java/com/android/server/wifi/WifiVendorHal.java b/service/java/com/android/server/wifi/WifiVendorHal.java
index 9c1ae94b5..88f189814 100644
--- a/service/java/com/android/server/wifi/WifiVendorHal.java
+++ b/service/java/com/android/server/wifi/WifiVendorHal.java
@@ -64,6 +64,7 @@ import android.net.wifi.WifiManager;
 import android.net.wifi.WifiScanner;
 import android.net.wifi.WifiSsid;
 import android.net.wifi.WifiWakeReasonAndCounts;
+import android.os.Handler;
 import android.os.Looper;
 import android.os.RemoteException;
 import android.util.MutableBoolean;
@@ -204,16 +205,23 @@ public class WifiVendorHal {
     private IWifiRttController mIWifiRttController;
     private final HalDeviceManager mHalDeviceManager;
     private final HalDeviceManagerStatusListener mHalDeviceManagerStatusCallbacks;
-    private final Looper mLooper;
     private final IWifiStaIfaceEventCallback mIWifiStaIfaceEventCallback;
     private final IWifiChipEventCallback mIWifiChipEventCallback;
     private final RttEventCallback mRttEventCallback;
 
+    // Plumbing for event handling.
+    //
+    // Being final fields, they can be accessed without synchronization under
+    // some reasonable assumptions. See
+    // https://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html#jls-17.5
+    private final Looper mLooper;
+    private final Handler mHalEventHandler;
 
     public WifiVendorHal(HalDeviceManager halDeviceManager,
                          Looper looper) {
         mHalDeviceManager = halDeviceManager;
         mLooper = looper;
+        mHalEventHandler = new Handler(looper);
         mHalDeviceManagerStatusCallbacks = new HalDeviceManagerStatusListener();
         mIWifiStaIfaceEventCallback = new StaIfaceEventCallback();
         mIWifiChipEventCallback = new ChipEventCallback();
@@ -2455,25 +2463,47 @@ public class WifiVendorHal {
                 WifiDebugRingBufferStatus status, java.util.ArrayList<Byte> data) {
             //TODO(b/35875078) Reinstate logging when execessive callbacks are fixed
             // mVerboseLog.d("onDebugRingBufferDataAvailable " + status);
-            WifiNative.WifiLoggerEventHandler eventHandler;
-            synchronized (sLock) {
-                if (mLogEventHandler == null || status == null || data == null) return;
-                eventHandler = mLogEventHandler;
-            }
-            eventHandler.onRingBufferData(
-                    ringBufferStatus(status), NativeUtil.byteArrayFromArrayList(data));
+            mHalEventHandler.post(() -> {
+                WifiNative.WifiLoggerEventHandler eventHandler;
+                synchronized (sLock) {
+                    if (mLogEventHandler == null || status == null || data == null) return;
+                    eventHandler = mLogEventHandler;
+                }
+                // Because |sLock| has been released, there is a chance that we'll execute
+                // a spurious callback (after someone has called resetLogHandler()).
+                //
+                // However, the alternative risks deadlock. Consider:
+                // [T1.1] WifiDiagnostics.captureBugReport()
+                // [T1.2] -- acquire WifiDiagnostics object's intrinsic lock
+                // [T1.3]    -> WifiVendorHal.getRingBufferData()
+                // [T1.4]       -- acquire WifiVendorHal.sLock
+                // [T2.1] <lambda>()
+                // [T2.2] -- acquire WifiVendorHal.sLock
+                // [T2.3]    -> WifiDiagnostics.onRingBufferData()
+                // [T2.4]       -- acquire WifiDiagnostics object's intrinsic lock
+                //
+                // The problem here is that the two threads acquire the locks in opposite order.
+                // If, for example, T2.2 executes between T1.2 and 1.4, then T1 and T2
+                // will be deadlocked.
+                eventHandler.onRingBufferData(
+                        ringBufferStatus(status), NativeUtil.byteArrayFromArrayList(data));
+            });
         }
 
         @Override
         public void onDebugErrorAlert(int errorCode, java.util.ArrayList<Byte> debugData) {
             mVerboseLog.d("onDebugErrorAlert " + errorCode);
-            WifiNative.WifiLoggerEventHandler eventHandler;
-            synchronized (sLock) {
-                if (mLogEventHandler == null || debugData == null) return;
-                eventHandler = mLogEventHandler;
-            }
-            eventHandler.onWifiAlert(
-                    errorCode, NativeUtil.byteArrayFromArrayList(debugData));
+            mHalEventHandler.post(() -> {
+                WifiNative.WifiLoggerEventHandler eventHandler;
+                synchronized (sLock) {
+                    if (mLogEventHandler == null || debugData == null) return;
+                    eventHandler = mLogEventHandler;
+                }
+                // See comment in onDebugRingBufferDataAvailable(), for an explanation
+                // of why this callback is invoked without |sLock| held.
+                eventHandler.onWifiAlert(
+                        errorCode, NativeUtil.byteArrayFromArrayList(debugData));
+            });
         }
     }
 
diff --git a/tests/wifitests/src/com/android/server/wifi/WifiVendorHalTest.java b/tests/wifitests/src/com/android/server/wifi/WifiVendorHalTest.java
index 66a55b51a..34ddf2378 100644
--- a/tests/wifitests/src/com/android/server/wifi/WifiVendorHalTest.java
+++ b/tests/wifitests/src/com/android/server/wifi/WifiVendorHalTest.java
@@ -1679,9 +1679,10 @@ public class WifiVendorHalTest {
         new Random().nextBytes(errorData);
 
         // Randomly raise the HIDL callback before we register for the log callback.
-        // This should be ignored.
+        // This should be safely ignored. (Not trigger NPE.)
         mIWifiChipEventCallback.onDebugErrorAlert(
                 errorCode, NativeUtil.byteArrayToArrayList(errorData));
+        mLooper.dispatchAll();
 
         WifiNative.WifiLoggerEventHandler eventHandler =
                 mock(WifiNative.WifiLoggerEventHandler.class);
@@ -1691,12 +1692,16 @@ public class WifiVendorHalTest {
         // Now raise the HIDL callback, this should be properly handled.
         mIWifiChipEventCallback.onDebugErrorAlert(
                 errorCode, NativeUtil.byteArrayToArrayList(errorData));
+        mLooper.dispatchAll();
         verify(eventHandler).onWifiAlert(eq(errorCode), eq(errorData));
 
         // Now stop the logging and invoke the callback. This should be ignored.
+        reset(eventHandler);
         assertTrue(mWifiVendorHal.resetLogHandler());
         mIWifiChipEventCallback.onDebugErrorAlert(
                 errorCode, NativeUtil.byteArrayToArrayList(errorData));
+        mLooper.dispatchAll();
+        verify(eventHandler, never()).onWifiAlert(anyInt(), anyObject());
     }
 
     /**
@@ -1714,9 +1719,10 @@ public class WifiVendorHalTest {
         new Random().nextBytes(errorData);
 
         // Randomly raise the HIDL callback before we register for the log callback.
-        // This should be ignored.
+        // This should be safely ignored. (Not trigger NPE.)
         mIWifiChipEventCallback.onDebugRingBufferDataAvailable(
                 new WifiDebugRingBufferStatus(), NativeUtil.byteArrayToArrayList(errorData));
+        mLooper.dispatchAll();
 
         WifiNative.WifiLoggerEventHandler eventHandler =
                 mock(WifiNative.WifiLoggerEventHandler.class);
@@ -1726,13 +1732,17 @@ public class WifiVendorHalTest {
         // Now raise the HIDL callback, this should be properly handled.
         mIWifiChipEventCallback.onDebugRingBufferDataAvailable(
                 new WifiDebugRingBufferStatus(), NativeUtil.byteArrayToArrayList(errorData));
+        mLooper.dispatchAll();
         verify(eventHandler).onRingBufferData(
                 any(WifiNative.RingBufferStatus.class), eq(errorData));
 
         // Now stop the logging and invoke the callback. This should be ignored.
+        reset(eventHandler);
         assertTrue(mWifiVendorHal.resetLogHandler());
         mIWifiChipEventCallback.onDebugRingBufferDataAvailable(
                 new WifiDebugRingBufferStatus(), NativeUtil.byteArrayToArrayList(errorData));
+        mLooper.dispatchAll();
+        verify(eventHandler, never()).onRingBufferData(anyObject(), anyObject());
     }
 
     /**