From b9235023ae196f4a49570390b7cba0ede6438219 Mon Sep 17 00:00:00 2001
From: Rob Pitkin <[email protected]>
Date: Fri, 28 Aug 2026 18:00:59 -0700
Subject: [PATCH] [WebNFC] Maintain visibility suspension state across rebinds on Android

On initial bind, NFCHost::GetNFC invoked MaybeResumeOrSuspendOperations
before GetNFCForHost. Due to Mojo FIFO message ordering, the suspension
signal was dispatched before the receiver was bound, causing
NfcProviderImpl to drop the signal because mNfcImpl was null. On
subsequent GetNFC rebinds, the visibility check was skipped entirely,
re-creating NfcImpl with operations unsuspended while invisible.

This CL fixes the race and rebind gaps by re-applying visibility
suspension after GetNFCForHost on every GetNFC call, and persisting
mOperationsSuspended in NfcProviderImpl across NfcImpl instances.

Bug: 517417437
Test: services_junit_tests --gtest_filter="NFCTest*"
Change-Id: I5c28f3ece27b0b008a3ae9f9ad37bdeb455f8201
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8274769
Reviewed-by: Matt Reynolds <[email protected]>
Commit-Queue: Rob Pitkin <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1688467}
---
diff --git a/services/device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java b/services/device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java
index 01c155ac..104fdbe 100644
--- a/services/device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java
+++ b/services/device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java
@@ -116,10 +116,14 @@
     /** Last time in milliseconds when a Tag was discovered. */
     private long mTagDiscoveredLastTimeMs = -1;
 
-    public NfcImpl(int hostId, NfcDelegate delegate, InterfaceRequest<Nfc> request) {
+    public NfcImpl(
+            int hostId,
+            NfcDelegate delegate,
+            InterfaceRequest<Nfc> request,
+            boolean operationsSuspended) {
         mHostId = hostId;
         mDelegate = delegate;
-        mOperationsSuspended = false;
+        mOperationsSuspended = operationsSuspended;
 
         // |request| may be null in tests.
         if (request != null) {
@@ -455,10 +459,16 @@
 
     /**
      * Enables reader mode, allowing NFC device to read / write / make read-only NFC tags.
+     *
      * @see android.nfc.NfcAdapter#enableReaderMode
      */
     private void enableReaderModeIfNeeded() {
-        if (mReaderCallbackHandler != null || mActivity == null || mNfcAdapter == null) return;
+        if (mOperationsSuspended
+                || mReaderCallbackHandler != null
+                || mActivity == null
+                || mNfcAdapter == null) {
+            return;
+        }
 
         if (!hasActiveOperations()) return;
 
@@ -732,4 +742,8 @@
         processPendingPushOperation();
         processPendingMakeReadOnlyOperation();
     }
+
+    boolean isOperationsSuspendedForTesting() {
+        return mOperationsSuspended;
+    }
 }
diff --git a/services/device/nfc/android/java/src/org/chromium/device/nfc/NfcProviderImpl.java b/services/device/nfc/android/java/src/org/chromium/device/nfc/NfcProviderImpl.java
index fd05be8..c7e01c9d 100644
--- a/services/device/nfc/android/java/src/org/chromium/device/nfc/NfcProviderImpl.java
+++ b/services/device/nfc/android/java/src/org/chromium/device/nfc/NfcProviderImpl.java
@@ -18,6 +18,7 @@
     private static final String TAG = "NfcProviderImpl";
     private final NfcDelegate mDelegate;
     private @Nullable NfcImpl mNfcImpl;
+    private boolean mOperationsSuspended;
 
     public NfcProviderImpl(NfcDelegate delegate) {
         mDelegate = delegate;
@@ -33,6 +34,7 @@
             mNfcImpl.closeMojoConnection();
             mNfcImpl = null;
         }
+        mOperationsSuspended = false;
     }
 
     @Override
@@ -49,12 +51,13 @@
         if (mNfcImpl != null) {
             mNfcImpl.closeMojoConnection();
         }
-        mNfcImpl = new NfcImpl(hostId, mDelegate, request);
+        mNfcImpl = new NfcImpl(hostId, mDelegate, request, mOperationsSuspended);
     }
 
     /** Suspends the NFC usage. Should be called when web page visibility is lost. */
     @Override
     public void suspendNfcOperations() {
+        mOperationsSuspended = true;
         if (mNfcImpl != null) {
             mNfcImpl.suspendNfcOperations();
         }
@@ -63,11 +66,20 @@
     /** Resumes the NFC usage. Should be called when web page becomes visible. */
     @Override
     public void resumeNfcOperations() {
+        mOperationsSuspended = false;
         if (mNfcImpl != null) {
             mNfcImpl.resumeNfcOperations();
         }
     }
 
+    @Nullable NfcImpl getNfcImplForTesting() {
+        return mNfcImpl;
+    }
+
+    boolean getOperationsSuspendedForTesting() {
+        return mOperationsSuspended;
+    }
+
     /** A factory for implementations of the NfcProvider interface. */
     public static class Factory implements InterfaceFactory<@Nullable NfcProvider> {
         private final NfcDelegate mDelegate;
diff --git a/services/device/nfc/android/junit/src/org/chromium/device/nfc/NFCTest.java b/services/device/nfc/android/junit/src/org/chromium/device/nfc/NFCTest.java
index 4c38088ee..165e35e 100644
--- a/services/device/nfc/android/junit/src/org/chromium/device/nfc/NFCTest.java
+++ b/services/device/nfc/android/junit/src/org/chromium/device/nfc/NFCTest.java
@@ -6,8 +6,10 @@
 
 import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyInt;
 import static org.mockito.ArgumentMatchers.anyString;
@@ -99,7 +101,7 @@
     /** Class that is used test NfcImpl implementation */
     private static class TestNfcImpl extends NfcImpl {
         public TestNfcImpl(Context context, NfcDelegate delegate) {
-            super(0, delegate, null);
+            super(0, delegate, null, false);
         }
 
         public void processPendingOperationsForTesting(NfcTagHandler handler) {
@@ -935,7 +937,7 @@
         localMojoNdefRecord.data = ApiCompatibilityUtils.getBytesUtf8(TEST_TEXT);
         {
             // Must start with ':'.
-            localMojoNdefRecord.recordType = "dummyLocalTypeNotStartingwith:";
+            localMojoNdefRecord.recordType = "localTypeNotStartingWith:";
             localMojoNdefRecord.data = ApiCompatibilityUtils.getBytesUtf8(TEST_TEXT);
             NdefMessage localMojoNdefMessage = createMojoNdefMessage(localMojoNdefRecord);
             android.nfc.NdefMessage localNdefMessage = null;
@@ -1172,6 +1174,14 @@
         verify(mockCallback).call(mErrorCaptor.capture());
         assertNull(mErrorCaptor.getValue());
 
+        // Check that reader mode was NOT enabled while operations are suspended.
+        verify(mNfcAdapter, never())
+                .enableReaderMode(
+                        any(Activity.class),
+                        any(ReaderCallback.class),
+                        anyInt(),
+                        (Bundle) isNull());
+
         // Check that watch is not triggered when NFC tag is in proximity.
         nfc.processPendingOperationsForTesting(mNfcTagHandler);
         verify(mNfcClient, times(0))
@@ -1978,4 +1988,83 @@
                 ApiCompatibilityUtils.getBytesUtf8(DUMMY_RECORD_ID),
                 payloadMessage.toByteArray());
     }
+
+    /**
+     * Test that NfcProviderImpl.suspendNfcOperations() persists the suspended state when called
+     * before getNfcForHost(), and applies it when NfcImpl is instantiated.
+     */
+    @Test
+    @Feature({"NFCTest"})
+    public void testProviderSuspendsBeforeGetNfcForHost() {
+        NfcProviderImpl provider = new NfcProviderImpl(mDelegate);
+        provider.suspendNfcOperations();
+        assertTrue(provider.getOperationsSuspendedForTesting());
+
+        provider.getNfcForHost(1, null);
+        NfcImpl nfc = provider.getNfcImplForTesting();
+        assertNotNull(nfc);
+        assertTrue(nfc.isOperationsSuspendedForTesting());
+
+        mDelegate.invokeCallback();
+        Push_Response mockCallback = mock(Push_Response.class);
+        nfc.push(createMojoNdefMessage(), createNdefWriteOptions(), mockCallback);
+
+        // Check that push request was cancelled immediately due to operations being suspended.
+        verify(mockCallback).call(mErrorCaptor.capture());
+        assertNotNull(mErrorCaptor.getValue());
+        assertEquals(NdefErrorType.OPERATION_CANCELLED, mErrorCaptor.getValue().errorType);
+    }
+
+    /**
+     * Test that NfcProviderImpl preserves the suspended state when a new NfcImpl is requested
+     * (rebind) while suspended.
+     */
+    @Test
+    @Feature({"NFCTest"})
+    public void testProviderRebindPreservesSuspendedState() {
+        NfcProviderImpl provider = new NfcProviderImpl(mDelegate);
+        provider.getNfcForHost(1, null);
+        NfcImpl nfc1 = provider.getNfcImplForTesting();
+        assertNotNull(nfc1);
+        assertFalse(nfc1.isOperationsSuspendedForTesting());
+
+        provider.suspendNfcOperations();
+        assertTrue(nfc1.isOperationsSuspendedForTesting());
+        assertTrue(provider.getOperationsSuspendedForTesting());
+
+        // Rebind with a new host/request
+        provider.getNfcForHost(2, null);
+        NfcImpl nfc2 = provider.getNfcImplForTesting();
+        assertNotNull(nfc2);
+        assertTrue(nfc2.isOperationsSuspendedForTesting());
+
+        mDelegate.invokeCallback();
+        Push_Response mockCallback = mock(Push_Response.class);
+        nfc2.push(createMojoNdefMessage(), createNdefWriteOptions(), mockCallback);
+
+        // Check that push request was cancelled immediately on the new instance.
+        verify(mockCallback).call(mErrorCaptor.capture());
+        assertNotNull(mErrorCaptor.getValue());
+        assertEquals(NdefErrorType.OPERATION_CANCELLED, mErrorCaptor.getValue().errorType);
+    }
+
+    /**
+     * Test that NfcProviderImpl.resumeNfcOperations() clears the suspended state and resumes
+     * operations on the underlying NfcImpl.
+     */
+    @Test
+    @Feature({"NFCTest"})
+    public void testProviderResumeClearsSuspendedState() {
+        NfcProviderImpl provider = new NfcProviderImpl(mDelegate);
+        provider.suspendNfcOperations();
+        assertTrue(provider.getOperationsSuspendedForTesting());
+
+        provider.resumeNfcOperations();
+        assertFalse(provider.getOperationsSuspendedForTesting());
+
+        provider.getNfcForHost(1, null);
+        NfcImpl nfc = provider.getNfcImplForTesting();
+        assertNotNull(nfc);
+        assertFalse(nfc.isOperationsSuspendedForTesting());
+    }
 }