Address comments on ag/11204387.

Add comments and slightly increase test coverage.

Bug: 153694684
Test: new test coverage in IpClientIntegrationTest
Change-Id: I160a0801449cbe9e66976eaacdd3a914adc3d341

2 files changed, 19 insertions, 3 deletions

diff --git a/src/android/net/ip/IpClientLinkObserver.java b/src/android/net/ip/IpClientLinkObserver.java
index 66a4038..dc58e7b 100644
--- a/src/android/net/ip/IpClientLinkObserver.java
+++ b/src/android/net/ip/IpClientLinkObserver.java
@@ -282,6 +282,12 @@ public class IpClientLinkObserver implements NetworkObserver {
         }
 
         private final AlarmManager.OnAlarmListener mExpirePref64Alarm = () -> {
+            // TODO: in the rare case where the alarm fires and posts the lambda to the handler
+            // thread while we are processing an RA that changes the lifetime of the same prefix,
+            // this code will run anyway even if the alarm is rescheduled or cancelled. If the
+            // lifetime in the RA is zero this doesn't matter (we just harmlessly cancel the alarm
+            // one extra time) but if the lifetime is nonzero then the prefix will be added and
+            // immediately removed by this code.
             updatePref64(mShim.getNat64Prefix(mLinkProperties),
                     mNat64PrefixExpiry, mNat64PrefixExpiry);
         };
@@ -321,9 +327,11 @@ public class IpClientLinkObserver implements NetworkObserver {
 
             // If we already have a prefix, continue using it and ignore the new one. Stopping and
             // restarting clatd is disruptive because it will break existing IPv4 connections.
-            // TODO: this means that if we receive an RA that adds a new prefix and deletes the old
+            // Note: this means that if we receive an RA that adds a new prefix and deletes the old
             // prefix, we might receive and ignore the new prefix, then delete the old prefix, and
-            // have no prefix until the next RA is received.
+            // have no prefix until the next RA is received. This is because the kernel returns ND
+            // user options one at a time even if they are in the same RA.
+            // TODO: keep track of the last few prefixes seen, like DnsServerRepository does.
             if (mNat64PrefixExpiry > now) return;
 
             // The current prefix has expired. Either replace it with the new one or delete it.
diff --git a/tests/integration/src/android/net/ip/IpClientIntegrationTest.java b/tests/integration/src/android/net/ip/IpClientIntegrationTest.java
index 6a5aff3..2db553e 100644
--- a/tests/integration/src/android/net/ip/IpClientIntegrationTest.java
+++ b/tests/integration/src/android/net/ip/IpClientIntegrationTest.java
@@ -1431,6 +1431,14 @@ public class IpClientIntegrationTest {
         expectNoNat64PrefixUpdate(inOrder, prefix);
         reset(mCb, mAlarm);
 
+        // Reduce the lifetime and expect to reschedule expiry.
+        pref64 = new StructNdOptPref64(prefix, 1500).toByteBuffer();
+        ra = buildRaPacket(pio, rdnss, pref64);
+        mPacketReader.sendResponse(ra);
+        pref64Alarm = expectAlarmSet(inOrder, "PREF64", 1496);
+        expectNoNat64PrefixUpdate(inOrder, prefix);
+        reset(mCb, mAlarm);
+
         // Withdraw the prefix and expect it to be set to null.
         pref64 = new StructNdOptPref64(prefix, 0).toByteBuffer();
         ra = buildRaPacket(pio, rdnss, pref64);
@@ -1456,7 +1464,7 @@ public class IpClientIntegrationTest {
         expectNoNat64PrefixUpdate(inOrder, prefix);
         reset(mCb, mAlarm);
 
-        // Withdraw the prefix and expect to switch to the new prefix.
+        // Withdraw the old prefix and continue to announce the new one. Expect a prefix change.
         pref64 = new StructNdOptPref64(prefix, 0).toByteBuffer();
         ra = buildRaPacket(pio, rdnss, pref64, otherPref64);
         mPacketReader.sendResponse(ra);