Fix bug that let responsePayloadBytes get set to -1 (#6721)

Fix a bug in `InstrHttpInputStream` that let
`NetworkRequestMetric.responsePayloadBytes` get set to -1 in some
conditions.

While investigating [b/398063523](http://b/398063523), I found that
`inputStream.read(...)` can return 0 in some cases, for example, when
the byte buffer length is 0. When this happens, it was possible to set
`responsePayloadBytes` to -1 because `-1 + 0 = -1`. I didn't just have
`bytesRead` initialize to 0 because there is a difference between 0
bytes read, and no read happened. Tested manually by hacking a test app
to force this to happen, and by unit tests.

Author: mrober

firebase-perf/CHANGELOG.md

@@ -1,5 +1,6 @@
 # Unreleased
 * [changed] Updated `protolite-well-known-types` dependency to `18.0.1`. [#6716]
+* [fixed] Fixed a bug that allowed invalid payload bytes value in network request metrics.
 
 
 # 21.0.4

firebase-perf/src/main/java/com/google/firebase/perf/network/InstrHttpInputStream.java

@@ -30,13 +30,7 @@ public final class InstrHttpInputStream extends InputStream {
   private long timeToResponseInitiated;
   private long timeToResponseLastRead = -1;
 
-  /**
-   * Instrumented inputStream object
-   *
-   * @param inputStream
-   * @param builder
-   * @param timer
-   */
+  /** Instrumented inputStream object */
   public InstrHttpInputStream(
       final InputStream inputStream, final NetworkRequestMetricBuilder builder, Timer timer) {
     this.timer = timer;
@@ -99,12 +93,13 @@ public int read() throws IOException {
       if (timeToResponseInitiated == -1) {
         timeToResponseInitiated = tempTime;
       }
-      if (bytesRead == -1 && timeToResponseLastRead == -1) {
+      boolean endOfStream = bytesRead == -1;
+      if (endOfStream && timeToResponseLastRead == -1) {
         timeToResponseLastRead = tempTime;
         networkMetricBuilder.setTimeToResponseCompletedMicros(timeToResponseLastRead);
         networkMetricBuilder.build();
       } else {
-        this.bytesRead++;
+        incrementBytesRead(1);
         networkMetricBuilder.setResponsePayloadBytes(this.bytesRead);
       }
       return bytesRead;
@@ -124,12 +119,13 @@ public int read(final byte[] buffer, final int byteOffset, final int byteCount)
       if (timeToResponseInitiated == -1) {
         timeToResponseInitiated = tempTime;
       }
-      if (bytesRead == -1 && timeToResponseLastRead == -1) {
+      boolean endOfStream = bytesRead == -1;
+      if (endOfStream && timeToResponseLastRead == -1) {
         timeToResponseLastRead = tempTime;
         networkMetricBuilder.setTimeToResponseCompletedMicros(timeToResponseLastRead);
         networkMetricBuilder.build();
       } else {
-        this.bytesRead += bytesRead;
+        incrementBytesRead(bytesRead);
         networkMetricBuilder.setResponsePayloadBytes(this.bytesRead);
       }
       return bytesRead;
@@ -148,12 +144,13 @@ public int read(final byte[] buffer) throws IOException {
       if (timeToResponseInitiated == -1) {
         timeToResponseInitiated = tempTime;
       }
-      if (bytesRead == -1 && timeToResponseLastRead == -1) {
+      boolean endOfStream = bytesRead == -1;
+      if (endOfStream && timeToResponseLastRead == -1) {
         timeToResponseLastRead = tempTime;
         networkMetricBuilder.setTimeToResponseCompletedMicros(timeToResponseLastRead);
         networkMetricBuilder.build();
       } else {
-        this.bytesRead += bytesRead;
+        incrementBytesRead(bytesRead);
         networkMetricBuilder.setResponsePayloadBytes(this.bytesRead);
       }
       return bytesRead;
@@ -183,11 +180,13 @@ public long skip(final long byteCount) throws IOException {
       if (timeToResponseInitiated == -1) {
         timeToResponseInitiated = tempTime;
       }
-      if (skipped == -1 && timeToResponseLastRead == -1) {
+      // InputStream.skip will return 0 for both end of stream and for 0 bytes skipped.
+      boolean endOfStream = (skipped == 0 && byteCount != 0);
+      if (endOfStream && timeToResponseLastRead == -1) {
         timeToResponseLastRead = tempTime;
         networkMetricBuilder.setTimeToResponseCompletedMicros(timeToResponseLastRead);
       } else {
-        bytesRead += skipped;
+        incrementBytesRead(skipped);
         networkMetricBuilder.setResponsePayloadBytes(bytesRead);
       }
       return skipped;
@@ -197,4 +196,12 @@ public long skip(final long byteCount) throws IOException {
       throw e;
     }
   }
+
+  private void incrementBytesRead(long bytesRead) {
+    if (this.bytesRead == -1) {
+      this.bytesRead = bytesRead;
+    } else {
+      this.bytesRead += bytesRead;
+    }
+  }
 }

firebase-perf/src/test/java/com/google/firebase/perf/network/InstrHttpInputStreamTest.java

@@ -30,6 +30,7 @@
 import com.google.firebase.perf.v1.NetworkRequestMetric.NetworkClientErrorReason;
 import java.io.IOException;
 import java.io.InputStream;
+import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -40,10 +41,14 @@
 import org.mockito.MockitoAnnotations;
 import org.robolectric.RobolectricTestRunner;
 
-/** Unit tests for {@link com.google.firebase.perf.network.InstrHttpInputStream}. */
+/**
+ * Unit tests for {@link com.google.firebase.perf.network.InstrHttpInputStream}.
+ *
+ * @noinspection ResultOfMethodCallIgnored
+ */
 @RunWith(RobolectricTestRunner.class)
 public class InstrHttpInputStreamTest extends FirebasePerformanceTestBase {
-
+  private AutoCloseable closeable;
   @Mock InputStream mInputStream;
   @Mock TransportManager transportManager;
   @Mock Timer timer;
@@ -53,12 +58,17 @@ public class InstrHttpInputStreamTest extends FirebasePerformanceTestBase {
 
   @Before
   public void setUp() {
-    MockitoAnnotations.initMocks(this);
+    closeable = MockitoAnnotations.openMocks(this);
     when(timer.getMicros()).thenReturn((long) 1000);
     when(timer.getDurationMicros()).thenReturn((long) 2000);
     networkMetricBuilder = NetworkRequestMetricBuilder.builder(transportManager);
   }
 
+  @After
+  public void releaseMocks() throws Exception {
+    closeable.close();
+  }
+
   @Test
   public void testAvailable() throws IOException {
     int availableVal = 7;
@@ -80,7 +90,7 @@ public void testClose() throws IOException {
   }
 
   @Test
-  public void testMark() throws IOException {
+  public void testMark() {
     int markInput = 256;
 
     new InstrHttpInputStream(mInputStream, networkMetricBuilder, timer).mark(markInput);
@@ -89,7 +99,7 @@ public void testMark() throws IOException {
   }
 
   @Test
-  public void testMarkSupported() throws IOException {
+  public void testMarkSupported() {
     when(mInputStream.markSupported()).thenReturn(true);
     boolean ret =
         new InstrHttpInputStream(mInputStream, networkMetricBuilder, timer).markSupported();
@@ -108,6 +118,20 @@ public void testRead() throws IOException {
     verify(mInputStream).read();
   }
 
+  @Test
+  public void testReadBufferOffsetZero() throws IOException {
+    byte[] b = new byte[0];
+    int off = 0;
+    int len = 0;
+    when(mInputStream.read(b, off, len)).thenReturn(len);
+    int ret = new InstrHttpInputStream(mInputStream, networkMetricBuilder, timer).read(b, off, len);
+
+    NetworkRequestMetric metric = networkMetricBuilder.build();
+    assertThat(ret).isEqualTo(0);
+    assertThat(metric.getResponsePayloadBytes()).isEqualTo(0);
+    verify(mInputStream).read(b, off, len);
+  }
+
   @Test
   public void testReadBufferOffsetCount() throws IOException {
     byte[] buffer = new byte[] {(byte) 0xe0};