Notify IpClient DHCP process success before configuring Interface.

This patch fixes the bug introduced by aosp/1169224 which moves
notifySuccess function to DhcpBoundState#enter. This behavior
is correct for DHCP reacquiring process (e.g. when client renews
or rebinds the previous leased IPv4 address, then enters Bound
state after receiving DHCPACK packet). However, this is incorrect
for DHCP solicit at the first time, since notifying IpClient DHCP
success after configuring interface causes mDhcpResult member is
still empty when handleLinkPropertiesUpdate is triggered by netd
completes configuring interface with new IPv4 address. Hence, new
Link Properties passed to wifi state machine doesn't contain the
available DNS servers received from DHCPACK.

Moving notifySuccess before sending CMD_CONFIGURE_LINKADDRESS cmd
to IpClient ensures mDhcpResult member has been initialize correctly
before assembling new LinkProperties.

Bug:146850745
Test: atest NetworkStackIntegrationTests NetworkStackTests
Change-Id: Ifb990ccc06c1374e616f71038b1811640821e954

2 files changed, 35 insertions, 14 deletions

diff --git a/src/android/net/dhcp/DhcpClient.java b/src/android/net/dhcp/DhcpClient.java
index b429923..fbcd0f6 100644
--- a/src/android/net/dhcp/DhcpClient.java
+++ b/src/android/net/dhcp/DhcpClient.java
@@ -1348,6 +1348,11 @@ public class DhcpClient extends StateMachine {
         @Override
         public void enter() {
             super.enter();
+            // We must call notifySuccess to apply the rest of the DHCP configuration (e.g., DNS
+            // servers) before adding the IP address to the interface. Otherwise, as soon as
+            // IpClient sees the IP address appear, it will enter provisioned state without any
+            // configuration information from DHCP. http://b/146850745.
+            notifySuccess();
             mController.sendMessage(CMD_CONFIGURE_LINKADDRESS, mDhcpLease.ipAddress);
         }
 
@@ -1632,7 +1637,6 @@ public class DhcpClient extends StateMachine {
                 transitionTo(mStoppedState);
             }
 
-            notifySuccess();
             scheduleLeaseTimers();
             logTimeToBoundState();
         }
@@ -1701,6 +1705,7 @@ public class DhcpClient extends StateMachine {
                     // the registered IpManager.Callback.  IP address changes
                     // are not supported here.
                     acceptDhcpResults(results, mLeaseMsg);
+                    notifySuccess();
                     transitionTo(mDhcpBoundState);
                 }
             } else if (packet instanceof DhcpNakPacket) {
diff --git a/tests/integration/src/android/net/ip/IpClientIntegrationTest.java b/tests/integration/src/android/net/ip/IpClientIntegrationTest.java
index d30b4e9..30b1b19 100644
--- a/tests/integration/src/android/net/ip/IpClientIntegrationTest.java
+++ b/tests/integration/src/android/net/ip/IpClientIntegrationTest.java
@@ -53,6 +53,7 @@ import static junit.framework.Assert.fail;
 
 import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
@@ -139,6 +140,7 @@ import java.net.NetworkInterface;
 import java.nio.ByteBuffer;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
@@ -636,8 +638,19 @@ public class IpClientIntegrationTest {
             } else {
                 fail("invalid DHCP packet");
             }
+
             // wait for reply to DHCPOFFER packet if disabling rapid commit option
             if (shouldReplyRapidCommitAck || !(packet instanceof DhcpDiscoverPacket)) {
+                if (!isDhcpIpConflictDetectEnabled && isSuccessLease) {
+                    // verify IPv4-only provisioning success before exiting loop.
+                    // 1. if it's a failure lease, onProvisioningSuccess() won't be called;
+                    // 2. if duplicated IPv4 address detection is enabled, verify TIMEOUT
+                    //    will affect ARP packet capture running in other test cases.
+                    ArgumentCaptor<LinkProperties> captor =
+                            ArgumentCaptor.forClass(LinkProperties.class);
+                    verifyProvisioningSuccess(captor, Collections.singletonList(CLIENT_ADDR));
+                }
+
                 return packetList;
             }
         }
@@ -692,6 +705,16 @@ public class IpClientIntegrationTest {
         verify(mCb, timeout(TEST_TIMEOUT_MS)).onLinkPropertiesChange(emptyLp);
     }
 
+    private void verifyProvisioningSuccess(ArgumentCaptor<LinkProperties> captor,
+            final Collection<InetAddress> addresses) throws Exception {
+        verify(mCb, timeout(TEST_TIMEOUT_MS)).onProvisioningSuccess(captor.capture());
+        LinkProperties lp = captor.getValue();
+        assertNotNull(lp);
+        assertNotEquals(0, lp.getDnsServers().size());
+        assertEquals(addresses.size(), lp.getAddresses().size());
+        assertTrue(lp.getAddresses().containsAll(addresses));
+    }
+
     private void doRestoreInitialMtuTest(final boolean shouldChangeMtu,
             final boolean shouldRemoveTapInterface) throws Exception {
         final long currentTime = System.currentTimeMillis();
@@ -862,6 +885,8 @@ public class IpClientIntegrationTest {
             assertTrue(packet instanceof DhcpDeclinePacket);
             assertEquals(packet.mServerIdentifier, SERVER_ADDR);
             assertEquals(packet.mRequestedIp, CLIENT_ADDR);
+
+            verify(mCb, never()).onProvisioningFailure(any());
             assertIpMemoryNeverStoreNetworkAttributes();
         } else if (isDhcpIpConflictDetectEnabled) {
             int arpPacketCount = 0;
@@ -875,6 +900,8 @@ public class IpClientIntegrationTest {
             assertArpProbe(packetList.get(0));
             assertArpAnnounce(packetList.get(3));
 
+            ArgumentCaptor<LinkProperties> captor = ArgumentCaptor.forClass(LinkProperties.class);
+            verifyProvisioningSuccess(captor, Collections.singletonList(CLIENT_ADDR));
             assertIpMemoryStoreNetworkAttributes(TEST_LEASE_DURATION_S, currentTime,
                     TEST_DEFAULT_MTU);
         }
@@ -903,6 +930,8 @@ public class IpClientIntegrationTest {
         performDhcpHandshake(false /* isSuccessLease */, TEST_LEASE_DURATION_S,
                 true /* isDhcpLeaseCacheEnabled */, false /* shouldReplyRapidCommitAck */,
                 TEST_DEFAULT_MTU, false /* isDhcpIpConflictDetectEnabled */);
+
+        verify(mCb, never()).onProvisioningSuccess(any());
         assertIpMemoryNeverStoreNetworkAttributes();
     }
 
@@ -1232,13 +1261,6 @@ public class IpClientIntegrationTest {
                 TEST_DEFAULT_MTU, false /* isDhcpIpConflictDetectEnabled */);
         assertIpMemoryStoreNetworkAttributes(TEST_LEASE_DURATION_S, currentTime, TEST_DEFAULT_MTU);
 
-        ArgumentCaptor<LinkProperties> captor = ArgumentCaptor.forClass(LinkProperties.class);
-        verify(mCb, timeout(TEST_TIMEOUT_MS)).onProvisioningSuccess(captor.capture());
-        LinkProperties lp = captor.getValue();
-        assertNotNull(lp);
-        assertEquals(1, lp.getAddresses().size());
-        assertTrue(lp.getAddresses().contains(InetAddress.getByName(CLIENT_ADDR.getHostAddress())));
-
         // Stop IpClient and expect a final LinkProperties callback with an empty LP.
         mIpc.stop();
         verify(mCb, timeout(TEST_TIMEOUT_MS)).onLinkPropertiesChange(argThat(
@@ -1257,12 +1279,6 @@ public class IpClientIntegrationTest {
         performDhcpHandshake(true /* isSuccessLease */, TEST_LEASE_DURATION_S,
                 true /* isDhcpLeaseCacheEnabled */, false /* shouldReplyRapidCommitAck */,
                 TEST_DEFAULT_MTU, false /* isDhcpIpConflictDetectEnabled */);
-
-        verify(mCb, timeout(TEST_TIMEOUT_MS)).onProvisioningSuccess(captor.capture());
-        lp = captor.getValue();
-        assertNotNull(lp);
-        assertEquals(1, lp.getAddresses().size());
-        assertTrue(lp.getAddresses().contains(InetAddress.getByName(CLIENT_ADDR.getHostAddress())));
     }
 
     @Test