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

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

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.net;
 
 import static org.chromium.base.CollectionUtil.newHashSet;
 
 import android.os.ConditionVariable;
 import android.test.suitebuilder.annotation.SmallTest;
@@ skipping 274 matching lines @@
         RequestFinishedInfo requestInfo = requestFinishedListener.getRequestInfo();
         MetricsTestUtil.checkRequestFinishedInfo(requestInfo, mUrl, startTime, endTime);
         assertEquals(RequestFinishedInfo.CANCELED, 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();
     }
 
+    // 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 {
         long requestStart = 1;
         long dnsStart = 2;
         long dnsEnd = -1;
         long connectStart = 4;
         long connectEnd = 5;
         long sslStart = 6;
         long sslEnd = 7;
@@ skipping 20 matching lines @@
         assertEquals(new Date(sslEnd), metrics.getSslEnd());
         assertEquals(new Date(pushStart), metrics.getPushStart());
         assertEquals(new Date(pushEnd), metrics.getPushEnd());
         assertEquals(new Date(responseStart), metrics.getResponseStart());
         assertEquals(new Date(requestEnd), metrics.getRequestEnd());
         assertEquals(socketReused, metrics.getSocketReused());
         assertEquals(sentByteCount, (long) metrics.getSentByteCount());
         assertEquals(receivedByteCount, (long) metrics.getReceivedByteCount());
     }
 }