Don't try to report metrics when there's no URLRequest

components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java

Index: components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java
diff --git a/components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java b/components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java
index 1f671a8bff67a8d7c3e4ae8ba1683c6c1d443cbc..99367ea59ef5e5567cd23d527246104787f1fc27 100644
--- a/components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java
+++ b/components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java
@@ -292,6 +292,55 @@ public class RequestFinishedInfoTest extends CronetTestBase {
         mTestFramework.mCronetEngine.shutdown();
     }
 
+    // Checks that CronetURLRequestAdapter::DestroyOnNetworkThread() doesn't crash when metrics
+    // collection is enabled and the URLRequest hasn't been created. See http://crbug.com/675629.
+    @SmallTest
+    @OnlyRunNativeCronet
+    @Feature({"Cronet"})
+    public void testExceptionInRequestStart() throws Exception {
+        mTestFramework = startCronetTestFramework();
+        TestRequestFinishedListener requestFinishedListener = new TestRequestFinishedListener();
+        mTestFramework.mCronetEngine.addRequestFinishedListener(requestFinishedListener);
+        TestUrlRequestCallback callback = new TestUrlRequestCallback();
+        ExperimentalUrlRequest.Builder urlRequestBuilder =
+                mTestFramework.mCronetEngine.newUrlRequestBuilder(
+                        mUrl, callback, callback.getExecutor());
+        // Empty headers are invalid and will cause start() to throw an exception.
+        UrlRequest request = urlRequestBuilder.addHeader("", "")
+                                     .addRequestAnnotation("request annotation")
+                                     .addRequestAnnotation(this)
+                                     .build();
+        try {
+            request.start();
+            fail("UrlRequest.start() should throw IllegalArgumentException");
+        } catch (IllegalArgumentException e) {
+            assertEquals("Invalid header =", e.getMessage());
+        }
+
+        // Now run a normal request and check that the listener only hears about this one.
+        callback = new TestUrlRequestCallback();

[xunjieli, 2016/12/19 18:52:09]

Does the test not repro the crash if you only make one request? Why is the
second request needed?

[mgersh, 2016/12/19 19:06:11]

It does repro without this part. It's primarily here to test that we don't try
to call the listener in this case, by making sure that the listener is first
called by the second request. I was also worried about it being flaky rather
than always failing because the crash happens on the network thread and might
not happen until after the test has finished, and this makes sure that later
events on the network thread are still happening. But I'm not sure whether
either of those is actually a concern in practice, so if you don't think it is,
I could delete this.

[xunjieli, 2016/12/19 19:40:05]

One thing confuses me is that TestRequestFinishedListener doesn't handle
multiple RequestFinishedInfo -- it basically remembers the latest update. If we
get updates from both requests, we wouldn't be able to tell from this test case.

I'd suggest either get rid of the second request or change
TestRequestFinishedListener so it can accumulate all updates that it receives.

[mgersh, 2016/12/19 20:20:08]

Done.

+        urlRequestBuilder =
+                (ExperimentalUrlRequest.Builder) mTestFramework.mCronetEngine.newUrlRequestBuilder(
+                        mUrl, callback, callback.getExecutor());
+        Date startTime = new Date();
+        urlRequestBuilder.addRequestAnnotation("request annotation")
+                .addRequestAnnotation(this)
+                .build()
+                .start();
+        callback.blockForDone();
+        requestFinishedListener.blockUntilDone();
+        Date endTime = new Date();
+
+        RequestFinishedInfo requestInfo = requestFinishedListener.getRequestInfo();
+        MetricsTestUtil.checkRequestFinishedInfo(requestInfo, mUrl, startTime, endTime);
+        assertEquals(RequestFinishedInfo.SUCCEEDED, requestInfo.getFinishedReason());
+        MetricsTestUtil.checkHasConnectTiming(requestInfo.getMetrics(), startTime, endTime, false);
+        assertEquals(newHashSet("request annotation", this), // Use sets for unordered comparison.
+                new HashSet<Object>(requestInfo.getAnnotations()));
+
+        mTestFramework.mCronetEngine.shutdown();
+    }
+
     @SmallTest
     @Feature({"Cronet"})
     public void testMetricsGetters() throws Exception {