From dc0861f5e39b1496208e999f8fbe2f587d8c69e5 Mon Sep 17 00:00:00 2001 From: Matthew Robertson Date: Tue, 25 Feb 2025 11:54:54 -0500 Subject: [PATCH 1/2] Fix bug that let responsePayloadBytes get set to -1 --- .../perf/network/InstrHttpInputStream.java | 37 +++++++++++-------- .../network/InstrHttpInputStreamTest.java | 34 ++++++++++++++--- 2 files changed, 51 insertions(+), 20 deletions(-) diff --git a/firebase-perf/src/main/java/com/google/firebase/perf/network/InstrHttpInputStream.java b/firebase-perf/src/main/java/com/google/firebase/perf/network/InstrHttpInputStream.java index fc660c70426..5ffff6c0d2f 100644 --- a/firebase-perf/src/main/java/com/google/firebase/perf/network/InstrHttpInputStream.java +++ b/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; + } + } } diff --git a/firebase-perf/src/test/java/com/google/firebase/perf/network/InstrHttpInputStreamTest.java b/firebase-perf/src/test/java/com/google/firebase/perf/network/InstrHttpInputStreamTest.java index 8a7ecb2131b..e1f45c45329 100644 --- a/firebase-perf/src/test/java/com/google/firebase/perf/network/InstrHttpInputStreamTest.java +++ b/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}; From 529bb6cb17f95826248d6f3326c7c1c59855e015 Mon Sep 17 00:00:00 2001 From: Matthew Robertson Date: Mon, 3 Mar 2025 15:06:05 -0500 Subject: [PATCH 2/2] Changelog --- firebase-perf/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/firebase-perf/CHANGELOG.md b/firebase-perf/CHANGELOG.md index 58bdd0f1d29..9cfa4e6537a 100644 --- a/firebase-perf/CHANGELOG.md +++ b/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