[Cronet] Leak NetLog instance used by CronetUrlRequestContextAdapter

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

 // Copyright 2014 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.content.Context;
 import android.content.ContextWrapper;
@@ skipping 27 matching lines @@
 public class CronetUrlRequestContextTest extends CronetTestBase {
     // URLs used for tests.
     private static final String MOCK_CRONET_TEST_FAILED_URL =
             "http://mock.failed.request/-2";
     private static final String MOCK_CRONET_TEST_SUCCESS_URL =
             "http://mock.http/success.txt";
 
     private EmbeddedTestServer mTestServer;
     private String mUrl;
     private String mUrl404;
+    private String mUrl500;
     CronetTestFramework mTestFramework;
 
     @Override
     protected void setUp() throws Exception {
         super.setUp();
         mTestServer = EmbeddedTestServer.createAndStartDefaultServer(getContext());
         mUrl = mTestServer.getURL("/echo?status=200");
         mUrl404 = mTestServer.getURL("/echo?status=404");
+        mUrl500 = mTestServer.getURL("/echo?status=500");
     }
 
     @Override
     protected void tearDown() throws Exception {
         mTestServer.stopAndDestroyServer();
         super.tearDown();
     }
 
     static class RequestThread extends Thread {
         public TestUrlRequestCallback mCallback;
@@ skipping 547 matching lines @@
         cronetEngine.stopNetLog();
         assertTrue(file.exists());
         assertTrue(file.length() != 0);
         assertFalse(hasBytesInNetLog(file));
         assertTrue(file.delete());
         assertTrue(!file.exists());
     }
 
     @SmallTest
     @Feature({"Cronet"})
+    @OnlyRunNativeCronet
+    // Tests that NetLog contains events emitted by all live CronetEngines.
+    public void testNetLogContainEventsFromAllLiveEngines() throws Exception {
+        Context context = getContext();
+        File directory = new File(PathUtils.getDataDirectory(context));
+        File file1 = File.createTempFile("cronet1", "json", directory);
+        File file2 = File.createTempFile("cronet2", "json", directory);
+        CronetEngine cronetEngine1 = new CronetUrlRequestContext(
+                new CronetEngine.Builder(context).setLibraryName("cronet_tests"));
+        CronetEngine cronetEngine2 = new CronetUrlRequestContext(
+                new CronetEngine.Builder(context).setLibraryName("cronet_tests"));
+
+        cronetEngine1.startNetLogToFile(file1.getPath(), false);
+        cronetEngine2.startNetLogToFile(file2.getPath(), false);
+
+        // Warm CronetEngine and make sure both CronetUrlRequestContexts are
+        // initialized before testing the logs.
+        makeRequestAndCheckStatus(cronetEngine1, mUrl, 200);
+        makeRequestAndCheckStatus(cronetEngine2, mUrl, 200);
+
+        // Use cronetEngine1 to make a request to mUrl404.
+        makeRequestAndCheckStatus(cronetEngine1, mUrl404, 404);
+
+        // Use cronetEngine2 to make a request to mUrl500.
+        makeRequestAndCheckStatus(cronetEngine2, mUrl500, 500);
+
+        cronetEngine1.stopNetLog();
+        cronetEngine2.stopNetLog();
+        assertTrue(file1.exists());
+        assertTrue(file2.exists());
+        // Make sure both files contain the two requests made separately using
+        // different engines.
+        assertTrue(containsStringInNetLog(file1, mUrl404));
+        assertTrue(containsStringInNetLog(file1, mUrl500));
+        assertTrue(containsStringInNetLog(file2, mUrl404));
+        assertTrue(containsStringInNetLog(file2, mUrl500));
+        assertTrue(file1.delete());
+        assertTrue(file2.delete());
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet
+    // Tests that if CronetEngine is shut down when reading from disk cache,
+    // there isn't a crash. See crbug.com/486120.
+    public void testShutDownEngineWhenReadingFromDiskCache() throws Exception {
+        enableCache(CronetEngine.Builder.HTTP_CACHE_DISK);
+        String url = NativeTestServer.getFileURL("/cacheable.txt");
+        // Make a request to a cacheable resource.
+        checkRequestCaching(url, false);
+
+        // Shut down the server.
+        NativeTestServer.shutdownNativeTestServer();
+        class CancelUrlRequestCallback extends TestUrlRequestCallback {
+            @Override
+            public void onResponseStarted(UrlRequest request, UrlResponseInfo info) {
+                super.onResponseStarted(request, info);
+                request.cancel();

[mmenke, 2016/04/07 19:55:13]

Can we shut down the engine here?

Also, if you revert the fix, does this test often crash?  Just curious if it
semi-consistently fails on regression.

[xunjieli, 2016/04/07 21:34:01]

Done. 
I was hoping it would crash, but it looks like my device can't repro the crash
on the other known flaky tests (DiskStorageTest) either. So I don't really
know.. But I think in theory, it should crash often without this change.

+            }
+
+            @Override
+            public void onSucceeded(UrlRequest request, UrlResponseInfo info) {
+                throw new RuntimeException("Unexpected");

[mmenke, 2016/04/07 19:55:13]

May want to explain why these aren't racy (Namely, they both depend on a single
threaded executor that runs both onResponseStarted and onSucceeded / onFailed)

[xunjieli, 2016/04/07 21:34:01]

Done.

+            }
+
+            @Override
+            public void onFailed(
+                    UrlRequest request, UrlResponseInfo info, UrlRequestException error) {
+                throw new RuntimeException("Unexpected");
+            }
+        }
+        CancelUrlRequestCallback callback = new CancelUrlRequestCallback();
+        UrlRequest.Builder urlRequestBuilder = new UrlRequest.Builder(
+                url, callback, callback.getExecutor(), mTestFramework.mCronetEngine);
+        urlRequestBuilder.build().start();
+        callback.blockForDone();
+        // Shut down CronetEngine immediately after request is destroyed.
+        mTestFramework.mCronetEngine.shutdown();
+        assertEquals(200, callback.mResponseInfo.getHttpStatusCode());
+        assertTrue(callback.mResponseInfo.wasCached());
+        assertTrue(callback.mOnCanceledCalled);
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet
     public void testNetLogAfterShutdown() throws Exception {
         mTestFramework = startCronetTestFramework();
         TestUrlRequestCallback callback = new TestUrlRequestCallback();
         UrlRequest.Builder urlRequestBuilder = new UrlRequest.Builder(
                 mUrl, callback, callback.getExecutor(), mTestFramework.mCronetEngine);
         urlRequestBuilder.build().start();
         callback.blockForDone();
         mTestFramework.mCronetEngine.shutdown();
 
         File directory = new File(PathUtils.getDataDirectory(getContext()));
@@ skipping 79 matching lines @@
         callback.blockForDone();
         cronetEngine.stopNetLog();
         assertTrue(file.exists());
         assertTrue(file.length() != 0);
         assertTrue(hasBytesInNetLog(file));
         assertTrue(file.delete());
         assertTrue(!file.exists());
     }
 
     private boolean hasBytesInNetLog(File logFile) throws Exception {
+        return containsStringInNetLog(logFile, "\"hex_encoded_bytes\"");
+    }
+
+    private boolean containsStringInNetLog(File logFile, String content) throws Exception {
         BufferedReader logReader = new BufferedReader(new FileReader(logFile));
         try {
             String logLine;
             while ((logLine = logReader.readLine()) != null) {
-                if (logLine.contains("\"hex_encoded_bytes\"")) {
+                if (logLine.contains(content)) {
                     return true;
                 }
             }
             return false;
         } finally {
             logReader.close();
         }
     }
 
+    /**
+     * Helper method to make a request to {@code url}, wait for it to
+     * complete, and check that the status code is the same as {@code expectedStatusCode}.
+     */
+    private void makeRequestAndCheckStatus(
+            CronetEngine engine, String url, int expectedStatusCode) {
+        TestUrlRequestCallback callback = new TestUrlRequestCallback();
+        UrlRequest request =
+                new UrlRequest.Builder(url, callback, callback.getExecutor(), engine).build();
+        request.start();
+        callback.blockForDone();
+        assertEquals(expectedStatusCode, callback.mResponseInfo.getHttpStatusCode());
+    }
+
     private void enableCache(int cacheType) throws Exception {
         String cacheTypeString = "";
         if (cacheType == CronetEngine.Builder.HTTP_CACHE_DISK) {
             cacheTypeString = CronetTestFramework.CACHE_DISK;
         } else if (cacheType == CronetEngine.Builder.HTTP_CACHE_DISK_NO_HTTP) {
             cacheTypeString = CronetTestFramework.CACHE_DISK_NO_HTTP;
         } else if (cacheType == CronetEngine.Builder.HTTP_CACHE_IN_MEMORY) {
             cacheTypeString = CronetTestFramework.CACHE_IN_MEMORY;
         }
         String[] commandLineArgs = {CronetTestFramework.CACHE_KEY, cacheTypeString};
         mTestFramework = startCronetTestFrameworkWithUrlAndCommandLineArgs(null, commandLineArgs);
         assertTrue(NativeTestServer.startNativeTestServer(getContext()));
     }
 
     private void checkRequestCaching(String url, boolean expectCached) {
         checkRequestCaching(url, expectCached, false);
     }
 
     private void checkRequestCaching(String url, boolean expectCached,
             boolean disableCache) {
         TestUrlRequestCallback callback = new TestUrlRequestCallback();
         UrlRequest.Builder urlRequestBuilder = new UrlRequest.Builder(
                 url, callback, callback.getExecutor(), mTestFramework.mCronetEngine);
         if (disableCache) {
             urlRequestBuilder.disableCache();
         }
         urlRequestBuilder.build().start();
         callback.blockForDone();
         assertEquals(expectCached, callback.mResponseInfo.wasCached());
+        assertEquals("this is a cacheable file\n", callback.mResponseAsString);
     }
 
     @SmallTest
     @Feature({"Cronet"})
     @OnlyRunNativeCronet
     public void testEnableHttpCacheDisabled() throws Exception {
         enableCache(CronetEngine.Builder.HTTP_CACHE_DISABLED);
         String url = NativeTestServer.getFileURL("/cacheable.txt");
         checkRequestCaching(url, false);
         checkRequestCaching(url, false);
@@ skipping 238 matching lines @@
         try {
             // ensureInitialized() calls native code to check the version right after library load
             // and will error with the message below if library loading was skipped
             CronetLibraryLoader.ensureInitialized(getContext(), builder);
             fail("Native library should not be loaded");
         } catch (UnsatisfiedLinkError e) {
             assertTrue(loader.wasCalled());
         }
     }
 }