[Extensions] Clean up Page capture tests and check for flakiness.

Bug: 942499
Change-Id: I21c4f5dd3e03419f3d272f3ffe6c38d764bd3328
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2333063Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: David Bertoni <dbertoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#795694}

chrome/browser/extensions/api/page_capture/page_capture_api.cc

@@ -226,6 +226,11 @@ void PageCaptureSaveAsMHTMLFunction::TemporaryFileCreatedOnIO(bool success) {
              base::TaskShutdownBehavior::BLOCK_SHUTDOWN})
             .get());
   }
+
+  // Let the delegate know the reference has been created.
+  if (test_delegate_)
+    test_delegate_->OnTemporaryFileCreated(mhtml_file_);
+
   content::GetUIThreadTaskRunner({})->PostTask(
       FROM_HERE,
       base::BindOnce(&PageCaptureSaveAsMHTMLFunction::TemporaryFileCreatedOnUI,
@@ -239,9 +244,6 @@ void PageCaptureSaveAsMHTMLFunction::TemporaryFileCreatedOnUI(bool success) {
     return;
   }
 
-  if (test_delegate_)
-    test_delegate_->OnTemporaryFileCreated(mhtml_path_);
-
   WebContents* web_contents = GetWebContents();
   if (!web_contents) {
     ReturnFailure(kTabClosedError);

chrome/browser/extensions/api/page_capture/page_capture_api.h

@@ -35,9 +35,10 @@ class PageCaptureSaveAsMHTMLFunction : public ExtensionFunction {
   // Test specific delegate used to test that the temporary file gets deleted.
   class TestDelegate {
    public:
-    // Called on the UI thread when the temporary file that contains the
+    // Called on the IO thread when the temporary file that contains the
     // generated data has been created.
-    virtual void OnTemporaryFileCreated(const base::FilePath& temp_file) = 0;
+    virtual void OnTemporaryFileCreated(
+        scoped_refptr<storage::ShareableFileReference> temp_file) = 0;
   };
   static void SetTestDelegate(TestDelegate* delegate);
 

chrome/browser/extensions/api/page_capture/page_capture_apitest.cc

@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <atomic>
+
 #include "base/base_switches.h"
 #include "base/command_line.h"
 #include "base/threading/thread_restrictions.h"
@@ -60,62 +62,47 @@ class PageCaptureSaveAsMHTMLDelegate
     PageCaptureSaveAsMHTMLFunction::SetTestDelegate(NULL);
   }
 
-  void OnTemporaryFileCreated(const base::FilePath& temp_file) override {
-    temp_file_ = temp_file;
+  void OnTemporaryFileCreated(
+      scoped_refptr<storage::ShareableFileReference> file) override {
+    file->AddFinalReleaseCallback(
+        base::BindOnce(&PageCaptureSaveAsMHTMLDelegate::OnReleaseCallback,
+                       base::Unretained(this)));
+    ++temp_file_count_;
+  }
+
+  void WaitForFinalRelease() {
+    if (temp_file_count_ > 0)
+      run_loop_.Run();
   }
 
-  base::FilePath temp_file_;
+  int temp_file_count() const { return temp_file_count_; }
+
+ private:
+  void OnReleaseCallback(const base::FilePath& path) {
+    if (--temp_file_count_ == 0)
+      release_closure_.Run();
+  }
+
+  base::RunLoop run_loop_;
+  base::RepeatingClosure release_closure_ = run_loop_.QuitClosure();
+  std::atomic<int> temp_file_count_{0};
 };
 
-// TODO(crbug.com/961017): Fix memory leaks in tests and re-enable on LSAN.
-#ifdef LEAK_SANITIZER
-#define MAYBE_SaveAsMHTMLWithActiveTabWithFileAccess \
-  DISABLED_SaveAsMHTMLWithActiveTabWithFileAccess
-#else
-#define MAYBE_SaveAsMHTMLWithActiveTabWithFileAccess \
-  SaveAsMHTMLWithActiveTabWithFileAccess
-#endif
-
-// TODO(crbug.com/961017): Fix memory leaks in tests and re-enable on LSAN.
-// Also flaky-failing on slow (debug) bots: https://crbug.com/1017305
-#if defined(LEAK_SANITIZER) || !defined(NDEBUG) || \
-    defined(ADDRESS_SANITIZER) || defined(OS_MAC) || defined(OS_WIN)
-#define MAYBE_SaveAsMHTML DISABLED_SaveAsMHTML
-#else
-#define MAYBE_SaveAsMHTML SaveAsMHTML
-#endif
-
-IN_PROC_BROWSER_TEST_F(ExtensionPageCaptureApiTest, MAYBE_SaveAsMHTML) {
+IN_PROC_BROWSER_TEST_F(ExtensionPageCaptureApiTest,
+                       SaveAsMHTMLWithoutFileAccess) {
   ASSERT_TRUE(StartEmbeddedTestServer());
   PageCaptureSaveAsMHTMLDelegate delegate;
-  ASSERT_TRUE(
-      RunExtensionTestWithArg("page_capture", "ONLY_PAGE_CAPTURE_PERMISSION"))
+  ASSERT_TRUE(RunExtensionTestWithFlagsAndArg(
+      "page_capture", "ONLY_PAGE_CAPTURE_PERMISSION", kFlagNone, kFlagNone))
       << message_;
-  // Make sure the MHTML data gets written to the temporary file.
-  ASSERT_FALSE(delegate.temp_file_.empty());
-  // Flush the message loops to make sure the delete happens.
-  content::RunAllTasksUntilIdle();
-  content::RunAllPendingInMessageLoop(content::BrowserThread::IO);
-  // Make sure the temporary file is destroyed once the javascript side reads
-  // the contents.
-  base::ScopedAllowBlockingForTesting allow_blocking;
-  ASSERT_FALSE(base::PathExists(delegate.temp_file_));
+  EXPECT_EQ(0, delegate.temp_file_count());
 }
 
-IN_PROC_BROWSER_TEST_F(ExtensionPageCaptureApiTest,
-                       MAYBE_SaveAsMHTMLWithActiveTabWithFileAccess) {
+IN_PROC_BROWSER_TEST_F(ExtensionPageCaptureApiTest, SaveAsMHTMLWithFileAccess) {
   ASSERT_TRUE(StartEmbeddedTestServer());
   PageCaptureSaveAsMHTMLDelegate delegate;
   ASSERT_TRUE(RunExtensionTest("page_capture")) << message_;
-  // Make sure the MHTML data gets written to the temporary file.
-  ASSERT_FALSE(delegate.temp_file_.empty());
-  // Flush the message loops to make sure the delete happens.
-  content::RunAllTasksUntilIdle();
-  content::RunAllPendingInMessageLoop(content::BrowserThread::IO);
-  // Make sure the temporary file is destroyed once the javascript side reads
-  // the contents.
-  base::ScopedAllowBlockingForTesting allow_blocking;
-  ASSERT_FALSE(base::PathExists(delegate.temp_file_));
+  delegate.WaitForFinalRelease();
 }
 
 #if defined(OS_CHROMEOS)
@@ -127,20 +114,18 @@ IN_PROC_BROWSER_TEST_F(ExtensionPageCaptureApiTest,
   // Resolve Permission dialog with Allow.
   ScopedTestDialogAutoConfirm auto_confirm(ScopedTestDialogAutoConfirm::ACCEPT);
   ASSERT_TRUE(RunExtensionTest("page_capture")) << message_;
-  ASSERT_FALSE(delegate.temp_file_.empty());
-  content::RunAllTasksUntilIdle();
-  content::RunAllPendingInMessageLoop(content::BrowserThread::IO);
-  base::ScopedAllowBlockingForTesting allow_blocking;
-  ASSERT_FALSE(base::PathExists(delegate.temp_file_));
+  delegate.WaitForFinalRelease();
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionPageCaptureApiTest,
                        PublicSessionRequestDenied) {
   ASSERT_TRUE(StartEmbeddedTestServer());
+  PageCaptureSaveAsMHTMLDelegate delegate;
   chromeos::ScopedTestPublicSessionLoginState login_state;
   // Resolve Permission dialog with Deny.
   ScopedTestDialogAutoConfirm auto_confirm(ScopedTestDialogAutoConfirm::CANCEL);
   ASSERT_TRUE(RunExtensionTestWithArg("page_capture", "REQUEST_DENIED"))
       << message_;
+  EXPECT_EQ(0, delegate.temp_file_count());
 }
 #endif  // defined(OS_CHROMEOS)

chrome/test/data/extensions/api_test/page_capture/test.js

@@ -7,24 +7,10 @@
 
 const assertEq = chrome.test.assertEq;
 const assertTrue = chrome.test.assertTrue;
-const fail = chrome.test.callbackFail;
-const pass = chrome.test.callbackPass;
 
 var testUrl = 'http://www.a.com:PORT' +
     '/extensions/api_test/page_capture/google.html';
 
-function waitForCurrentTabLoaded(callback) {
-  chrome.tabs.getSelected(null, function(tab) {
-    if (tab.status == 'complete') {
-      callback();
-      return;
-    }
-    window.setTimeout(function() {
-      waitForCurrentTabLoaded(callback);
-    }, 100);
-  });
-}
-
 function testPageCapture(data, isFile) {
   assertEq(undefined, chrome.runtime.lastError);
   assertTrue(data != null);
@@ -39,11 +25,11 @@ function testPageCapture(data, isFile) {
       assertTrue(text.indexOf('logo.png') != -1);
     }
     // Run the GC so the blob is deleted.
-    window.setTimeout(function() {
-      window.gc();
+    setTimeout(function() {
+      gc();
     });
-    window.setTimeout(function() {
-      chrome.test.notifyPass();
+    setTimeout(function() {
+      chrome.test.succeed();
     }, 0);
   };
   reader.readAsText(data);
@@ -54,52 +40,46 @@ chrome.test.getConfig(function(config) {
 
   chrome.test.runTests([
     function saveAsMHTML() {
-      chrome.tabs.getSelected(null, function(tab) {
-        chrome.tabs.update(null, { "url": testUrl });
-        waitForCurrentTabLoaded(pass(function() {
+      chrome.tabs.onUpdated.addListener(function listener(
+          tabId, changeInfo, tab) {
+        if (tab.status == 'complete') {
+          chrome.tabs.onUpdated.removeListener(listener);
           chrome.pageCapture.saveAsMHTML(
-              {'tabId': tab.id}, pass(function(data) {
+              {tabId: tab.id}, function(data) {
                 if (config.customArg == 'REQUEST_DENIED') {
                   chrome.test.assertLastError('User denied request.');
-                  chrome.test.notifyPass();
+                  chrome.test.succeed();
                   return;
                 }
-                testPageCapture(data, false /* isFile */);
-              }));
-        }));
+                testPageCapture(data, false);
+              });
+        }
       });
+      chrome.tabs.create({url: testUrl});
     },
 
-    function saveAsMHTMLFile() {
-      chrome.tabs.getSelected(null, function(tab) {
-        chrome.tabs.update(null, {'url': config.testDataDirectory});
-        waitForCurrentTabLoaded(function() {
-          if (config.customArg == 'REQUEST_DENIED') {
+    function saveFileUrlAsMHTML() {
+      chrome.tabs.onUpdated.addListener(function listener(
+          tabId, changeInfo, tab) {
+        if (tab.status == 'complete') {
+          chrome.tabs.onUpdated.removeListener(listener);
           chrome.pageCapture.saveAsMHTML(
-                {'tabId': tab.id}, fail('User denied request.', function(data) {
+              {tabId: tab.id}, function(data) {
+            if (config.customArg == 'REQUEST_DENIED') {
               chrome.test.assertLastError('User denied request.');
-                  chrome.test.notifyPass();
-                  return;
-                }));
+              chrome.test.succeed();
             } else if (config.customArg == 'ONLY_PAGE_CAPTURE_PERMISSION') {
-            chrome.pageCapture.saveAsMHTML(
-                {'tabId': tab.id},
-                fail(
-                    'Don\'t have permissions required to capture this page.',
-                    function(data) {
               chrome.test.assertLastError(
                   'Don\'t have permissions required to capture this ' +
                     'page.');
-                      return;
-                    }));
+              chrome.test.succeed();
             } else {
-            chrome.pageCapture.saveAsMHTML(
-                {'tabId': tab.id}, pass(function(data) {
               testPageCapture(data, true /* isFile */);
-                }));
             }
           });
+        }
       });
+      chrome.tabs.create({url: config.testDataDirectory});
     }
   ]);
 });