ceftests: Simplify completion for tests that don't just create browsers

Some test cases don't create browsers at all, or require additional signals
such as request context destruction.

To test: Run `ceftests --gtest_filter=URLRequestTest.*`

tests/ceftests/message_router_multi_query_unittest.cc

@@ -957,7 +957,7 @@ class MultiQuerySingleFrameTestHandler : public SingleLoadTestHandler,
         AssertQueryCount(nullptr, nullptr, 0);
       } else if (cancel_type_ == CANCEL_BY_CLOSING_BROWSER) {
         // Change the expected behavior in the handler.
-        SetSignalCompletionWhenAllBrowsersClose(false);
+        SetSignalTestCompletionCount(1U);
         CloseBrowser(GetBrowser(), false);
       }
     }
@@ -971,11 +971,12 @@ class MultiQuerySingleFrameTestHandler : public SingleLoadTestHandler,
 
     DestroyTest();
 
-    if (!SignalCompletionWhenAllBrowsersClose()) {
+    if (!AllowTestCompletionWhenAllBrowsersClose()) {
       // Complete asynchronously so the call stack has a chance to unwind.
-      CefPostTask(TID_UI,
+      CefPostTask(
-                  base::BindOnce(
+          TID_UI,
-                      &MultiQuerySingleFrameTestHandler::TestComplete, this));
+          base::BindOnce(
+              &MultiQuerySingleFrameTestHandler::SignalTestCompletion, this));
     }
   }
 

tests/ceftests/resource_request_handler_unittest.cc

@@ -500,7 +500,7 @@ class BasicResponseTest : public TestHandler {
     TestHandler::OnAfterCreated(browser);
 
     if (mode_ == ABORT_AFTER_CREATED) {
-      SetSignalCompletionWhenAllBrowsersClose(false);
+      SetSignalTestCompletionCount(1U);
       CloseBrowser(browser, false);
     }
   }
@@ -547,7 +547,7 @@ class BasicResponseTest : public TestHandler {
     VerifyState(kOnBeforeBrowse, request, nullptr);
 
     if (mode_ == ABORT_BEFORE_BROWSE) {
-      SetSignalCompletionWhenAllBrowsersClose(false);
+      SetSignalTestCompletionCount(1U);
       CloseBrowser(browser, false);
     }
 
@@ -994,10 +994,10 @@ class BasicResponseTest : public TestHandler {
 
     TestHandler::DestroyTest();
 
-    if (!SignalCompletionWhenAllBrowsersClose()) {
+    if (!AllowTestCompletionWhenAllBrowsersClose()) {
       // Complete asynchronously so the call stack has a chance to unwind.
-      CefPostTask(TID_UI,
+      CefPostTask(TID_UI, base::BindOnce(
-                  base::BindOnce(&BasicResponseTest::TestComplete, this));
+                              &BasicResponseTest::SignalTestCompletion, this));
     }
   }
 
@@ -1337,7 +1337,7 @@ class BasicResponseTest : public TestHandler {
 
   void CloseBrowserAsync() {
     EXPECT_TRUE(IsIncomplete());
-    SetSignalCompletionWhenAllBrowsersClose(false);
+    SetSignalTestCompletionCount(1U);
     CefPostDelayedTask(
         TID_UI, base::BindOnce(&TestHandler::CloseBrowser, GetBrowser(), false),
         100);
@@ -2131,10 +2131,11 @@ class SubresourceResponseTest : public RoutingTestHandler {
 
     TestHandler::DestroyTest();
 
-    if (!SignalCompletionWhenAllBrowsersClose()) {
+    if (!AllowTestCompletionWhenAllBrowsersClose()) {
       // Complete asynchronously so the call stack has a chance to unwind.
-      CefPostTask(TID_UI,
+      CefPostTask(
-                  base::BindOnce(&SubresourceResponseTest::TestComplete, this));
+          TID_UI,
+          base::BindOnce(&SubresourceResponseTest::SignalTestCompletion, this));
     }
   }
 
@@ -2544,7 +2545,7 @@ class SubresourceResponseTest : public RoutingTestHandler {
 
   void CloseBrowserAsync() {
     EXPECT_TRUE(IsIncomplete());
-    SetSignalCompletionWhenAllBrowsersClose(false);
+    SetSignalTestCompletionCount(1U);
     CefPostDelayedTask(
         TID_UI, base::BindOnce(&TestHandler::CloseBrowser, GetBrowser(), false),
         100);

tests/ceftests/run_all_unittests.cc

@@ -74,8 +74,8 @@ void RunTestsOnTestThread() {
   // Run the test suite.
   CefTestSuite::GetInstance()->Run();
 
-  // Wait for all browsers to exit.
+  // Wait for all TestHandlers to be destroyed.
-  while (TestHandler::HasBrowser()) {
+  while (TestHandler::HasTestHandler()) {
     sleep(100);
   }
 

tests/ceftests/test_handler.cc

@@ -233,10 +233,13 @@ void TestHandler::UIThreadHelper::TaskHelper(base::OnceClosure task) {
 
 // TestHandler
 
-int TestHandler::browser_count_ = 0;
+// static
+std::atomic<size_t> TestHandler::test_handler_count_{0U};
 
 TestHandler::TestHandler(CompletionState* completion_state)
     : debug_string_prefix_(MakeDebugStringPrefix()) {
+  test_handler_count_++;
+
   if (completion_state) {
     completion_state_ = completion_state;
     completion_state_owned_ = false;
@@ -254,7 +257,6 @@ TestHandler::~TestHandler() {
     EXPECT_FALSE(destroy_test_called_);
   }
   EXPECT_TRUE(browser_map_.empty());
-  EXPECT_EQ(0U, window_count_);
   EXPECT_TRUE(browser_status_map_.empty());
 
   if (completion_state_owned_) {
@@ -264,6 +266,8 @@ TestHandler::~TestHandler() {
   if (destroy_event_) {
     destroy_event_->Signal();
   }
+
+  test_handler_count_--;
 }
 
 void TestHandler::OnAfterCreated(CefRefPtr<CefBrowser> browser) {
@@ -300,26 +304,25 @@ void TestHandler::OnBeforeClose(CefRefPtr<CefBrowser> browser) {
 void TestHandler::OnWindowCreated(int browser_id) {
   CHECK(UseViews());
   EXPECT_UI_THREAD();
-  window_count_++;
   OnCreated(browser_id, NT_WINDOW);
 }
 
 void TestHandler::OnWindowDestroyed(int browser_id) {
   CHECK(UseViews());
   EXPECT_UI_THREAD();
-  window_count_--;
   OnClosed(browser_id, NT_WINDOW);
 }
 
 void TestHandler::OnCreated(int browser_id, NotifyType type) {
-  bool creation_complete = false;
-
   auto& browser_status = browser_status_map_[browser_id];
   EXPECT_FALSE(browser_status.got_created[type])
       << "Duplicate call to OnCreated(" << browser_id << ", "
       << (type == NT_BROWSER ? "BROWSER" : "WINDOW") << ")";
   browser_status.got_created[type].yes();
 
+#if VERBOSE_DEBUGGING
+  bool creation_complete = false;
+
   // When using Views, wait for both Browser and Window notifications.
   if (UseViews()) {
     creation_complete = browser_status.got_created[NT_BROWSER] &&
@@ -328,15 +331,10 @@ void TestHandler::OnCreated(int browser_id, NotifyType type) {
     creation_complete = browser_status.got_created[NT_BROWSER];
   }
 
-#if VERBOSE_DEBUGGING
   LOG(INFO) << debug_string_prefix_ << browser_id << ": OnCreated type="
             << (type == NT_BROWSER ? "BROWSER" : "WINDOW")
             << " creation_complete=" << creation_complete;
-#endif
+#endif  // VERBOSE_DEBUGGING
-
-  if (creation_complete) {
-    browser_count_++;
-  }
 }
 
 void TestHandler::OnClosed(int browser_id, NotifyType type) {
@@ -356,29 +354,23 @@ void TestHandler::OnClosed(int browser_id, NotifyType type) {
     close_complete = browser_status.got_closed[NT_BROWSER];
   }
 
-  // Test is complete if no Browsers/Windows are remaining.
+  if (close_complete) {
-  const bool test_complete =
+    browser_status_map_.erase(browser_id);
-      close_complete &&
+  }
-      (UseViews() ? window_count_ == 0 : browser_map_.empty());
+
+  // Test may be complete if no Browsers/Windows are remaining.
+  const bool all_browsers_closed = AllBrowsersClosed();
 
 #if VERBOSE_DEBUGGING
   LOG(INFO) << debug_string_prefix_ << browser_id
             << ": OnClosed type=" << (type == NT_BROWSER ? "BROWSER" : "WINDOW")
             << " close_complete=" << close_complete
-            << " test_complete=" << test_complete;
+            << " all_browsers_closed=" << all_browsers_closed;
 #endif
 
-  if (close_complete) {
+  if (all_browsers_closed) {
-    browser_status_map_.erase(browser_id);
+    // May result in |this| being deleted.
-  }
+    MaybeTestComplete();
-
-  if (test_complete && signal_completion_when_all_browsers_close_) {
-    // Signal that the test is now complete. May result in |this| being deleted.
-    TestComplete();
-  }
-
-  if (close_complete) {
-    browser_count_--;
   }
 }
 
@@ -491,18 +483,30 @@ void TestHandler::DestroyTest() {
       CloseBrowser(it->second, false);
     }
   }
-
-  if (ui_thread_helper_.get()) {
-    ui_thread_helper_.reset(nullptr);
-  }
 }
 
 void TestHandler::OnTestTimeout(int timeout_ms, bool treat_as_error) {
   EXPECT_UI_THREAD();
+
+  EXPECT_FALSE(test_timeout_called_);
+  test_timeout_called_ = true;
+
   if (treat_as_error) {
     EXPECT_TRUE(false) << "Test timed out after " << timeout_ms << "ms";
   }
+
+  EXPECT_FALSE(AllBrowsersClosed() && AllowTestCompletionWhenAllBrowsersClose())
+      << "Test timed out unexpectedly; should be complete";
+
+  // Close any remaining browsers.
   DestroyTest();
+
+  // Reset signal completion count.
+  if (signal_completion_count_ > 0) {
+    signal_completion_count_ = 0;
+    // May result in |this| being deleted.
+    MaybeTestComplete();
+  }
 }
 
 void TestHandler::CreateBrowser(const CefString& url,
@@ -606,17 +610,79 @@ void TestHandler::SetTestTimeout(int timeout_ms, bool treat_as_error) {
       *timeout);
 }
 
-void TestHandler::TestComplete() {
+void TestHandler::SetSignalTestCompletionCount(size_t count) {
+#if VERBOSE_DEBUGGING
+  LOG(INFO) << debug_string_prefix_
+            << "SetSignalTestCompletionCount count=" << count;
+#endif
+  signal_completion_count_ = count;
+}
+
+void TestHandler::SignalTestCompletion() {
   if (!CefCurrentlyOn(TID_UI)) {
-    CefPostTask(TID_UI, base::BindOnce(&TestHandler::TestComplete, this));
+    CefPostTask(TID_UI,
+                base::BindOnce(&TestHandler::SignalTestCompletion, this));
     return;
   }
 
+  if (test_timeout_called_) {
+    // Ignore any signals that arrive after test timeout.
+    return;
+  }
+
+  CHECK_GT(signal_completion_count_, 0U);
+  signal_completion_count_--;
+
+#if VERBOSE_DEBUGGING
+  LOG(INFO) << debug_string_prefix_
+            << "SignalTestComplete remaining=" << signal_completion_count_;
+#endif
+
+  if (signal_completion_count_ == 0) {
+    // May result in |this| being deleted.
+    MaybeTestComplete();
+  }
+}
+
+bool TestHandler::AllowTestCompletionWhenAllBrowsersClose() const {
+  EXPECT_UI_THREAD();
+  return signal_completion_count_ == 0U;
+}
+
+bool TestHandler::AllBrowsersClosed() const {
+  EXPECT_UI_THREAD();
+  return browser_status_map_.empty();
+}
+
+void TestHandler::MaybeTestComplete() {
+  EXPECT_UI_THREAD();
+
+  const bool all_browsers_closed = AllBrowsersClosed();
+  const bool allow_test_completion = AllowTestCompletionWhenAllBrowsersClose();
+
+#if VERBOSE_DEBUGGING
+  LOG(INFO) << debug_string_prefix_
+            << "MaybeTestComplete all_browsers_closed=" << all_browsers_closed
+            << " allow_test_completion=" << allow_test_completion;
+#endif
+
+  if (all_browsers_closed && allow_test_completion) {
+    TestComplete();
+  }
+}
+
+void TestHandler::TestComplete() {
+  EXPECT_UI_THREAD();
+  EXPECT_TRUE(AllBrowsersClosed());
+  EXPECT_TRUE(AllowTestCompletionWhenAllBrowsersClose());
+
 #if VERBOSE_DEBUGGING
   LOG(INFO) << debug_string_prefix_ << "TestComplete";
 #endif
 
-  EXPECT_TRUE(browser_map_.empty());
+  // Cancel any pending tasks posted via UIThreadHelper.
+  ui_thread_helper_.reset();
+
   completion_state_->TestComplete();
 }
 

tests/ceftests/test_handler.h

@@ -6,6 +6,7 @@
 #define CEF_TESTS_CEFTESTS_TEST_HANDLER_H_
 #pragma once
 
+#include <atomic>
 #include <list>
 #include <map>
 #include <memory>
@@ -210,8 +211,10 @@ class TestHandler : public CefClient,
   void OnWindowCreated(int browser_id);
   void OnWindowDestroyed(int browser_id);
 
-  // Returns true if a browser currently exists.
+  // Returns true if a TestHandler currently exists.
-  static bool HasBrowser() { return browser_count_ > 0; }
+  static bool HasTestHandler() {
+    return test_handler_count_.load(std::memory_order_relaxed) > 0U;
+  }
 
   std::string debug_string_prefix() const { return debug_string_prefix_; }
 
@@ -220,11 +223,8 @@ class TestHandler : public CefClient,
   // Collection.
   virtual void SetupComplete();
 
-  // Close any existing non-popup browsers. Test completion will be signaled
+  // Close any remaining browsers. This may result in a call to TestComplete(),
-  // once all the browsers have closed if
+  // depending on the configuration of SetSignalCompletionCount().
-  // |signal_completion_when_all_browsers_close_| is true (default value).
-  // If no browsers exist then this method will do nothing and
-  // TestComplete() must be called manually.
   virtual void DestroyTest();
 
   // Called on the UI thread if the test times out as a result of calling
@@ -245,27 +245,47 @@ class TestHandler : public CefClient,
 
   void ClearResources();
 
-  void SetSignalCompletionWhenAllBrowsersClose(bool val) {
+  // Specify the number of times that SignalTestCompletion() needs to be
-    signal_completion_when_all_browsers_close_ = val;
+  // explicitly called for test completion. Must be configured during test
-  }
+  // initialization before any browsers are created.
-  bool SignalCompletionWhenAllBrowsersClose() const {
+  // - If the test creates browsers and does not explicitly call
-    return signal_completion_when_all_browsers_close_;
+  //   SignalTestCompletion() then the default value (0) can be used.
-  }
+  // - If the test creates browsers and explicitly calls SignalTestCompletion()
+  //   then set a value >= 1.
+  // - If the test does not create browsers then it must explicitly call
+  //   SignalTestCompletion() and set a value >= 1.
+  void SetSignalTestCompletionCount(size_t count);
+
+  // Explicitly signal test completion a single time. Used in combination with
+  // SetSignalTestCompletionCount(). Results in a call to TestComplete() if
+  // all browsers have closed and this method has been called the expected
+  // number of times.
+  void SignalTestCompletion();
+
+  // Reuturns true if SignalTestCompletion() has been called the necessary
+  // number of times (may be 0), in which case TestComplete() will be called
+  // automatically when all browsers have closed. Must be called on the
+  // UI thread.
+  bool AllowTestCompletionWhenAllBrowsersClose() const;
+
+  // Returns true if all browsers have closed. Must be called on the UI
+  // thread.
+  bool AllBrowsersClosed() const;
 
   // Call OnTestTimeout() after the specified amount of time.
   void SetTestTimeout(int timeout_ms = 5000, bool treat_as_error = true);
 
-  // Signal that the test is complete. This will be called automatically when
-  // all existing non-popup browsers are closed if
-  // |signal_completion_when_all_browsers_close_| is true (default value). It
-  // is an error to call this method before all browsers have closed.
-  void TestComplete();
-
   // Returns the single UIThreadHelper instance, creating it if necessary. Must
   // be called on the UI thread.
   UIThreadHelper* GetUIThreadHelper();
 
  private:
+  void MaybeTestComplete();
+
+  // Complete the test. It is an error to call this method before all browsers
+  // have closed.
+  void TestComplete();
+
   enum NotifyType {
     NT_BROWSER,
     NT_WINDOW,
@@ -283,9 +303,6 @@ class TestHandler : public CefClient,
   // Map of browser ID to browser object. Only accessed on the UI thread.
   BrowserMap browser_map_;
 
-  // Count of window objects. Only accessed on the UI thread.
-  size_t window_count_ = 0;
-
   struct NotifyStatus {
     // Keyed by NotifyType.
     TrackCallback got_created[2];
@@ -305,19 +322,22 @@ class TestHandler : public CefClient,
   typedef std::map<std::string, ResourceContent> ResourceMap;
   ResourceMap resource_map_;
 
-  // If true test completion will be signaled when all browsers have closed.
+  // Number of times that SignalTestCompletion() must be called.
-  bool signal_completion_when_all_browsers_close_ = true;
+  size_t signal_completion_count_ = 0U;
 
   CefRefPtr<CefWaitableEvent> destroy_event_;
 
+  // Tracks whether OnTestTimeout() has been called.
+  bool test_timeout_called_ = false;
+
   // Tracks whether DestroyTest() is expected or has been called.
   bool destroy_test_expected_ = true;
   bool destroy_test_called_ = false;
 
   std::unique_ptr<UIThreadHelper> ui_thread_helper_;
 
-  // Used to track the number of currently existing browser windows.
+  // Used to track the number of currently existing TestHandlers.
-  static int browser_count_;
+  static std::atomic<size_t> test_handler_count_;
 
   DISALLOW_COPY_AND_ASSIGN(TestHandler);
 };

tests/ceftests/urlrequest_unittest.cc

@@ -2628,6 +2628,18 @@ class RequestTestHandler : public TestHandler {
         base::BindOnce(&RequestTestHandler::OnIncompleteRequest, this));
     test_runner_->Initialize();
 
+    // Configure the number of times that SignalTestCompletion() will be called.
+    // We need to call it at least 1 time if we don't create a browser.
+    size_t completion_count = test_frame_method_ ? 0U : 1U;
+    if (context_mode_ != CONTEXT_GLOBAL) {
+      // Don't end the test until the temporary request context has been
+      // destroyed.
+      completion_count++;
+    }
+    if (completion_count > 0U) {
+      SetSignalTestCompletionCount(completion_count);
+    }
+
     // Get or create the request context.
     if (context_mode_ == CONTEXT_GLOBAL) {
       CefRefPtr<CefRequestContext> request_context =
@@ -2637,10 +2649,6 @@ class RequestTestHandler : public TestHandler {
 
       PreSetupComplete();
     } else {
-      // Don't end the test until the temporary request context has been
-      // destroyed.
-      SetSignalCompletionWhenAllBrowsersClose(false);
-
       CefRequestContextSettings settings;
 
       if (context_mode_ == CONTEXT_ONDISK) {
@@ -2768,7 +2776,6 @@ class RequestTestHandler : public TestHandler {
 
     // TestComplete will eventually be called from DestroyTest instead of being
     // triggered by browser destruction.
-    SetSignalCompletionWhenAllBrowsersClose(false);
     CefPostDelayedTask(
         TID_UI, base::BindOnce(&TestHandler::CloseBrowser, GetBrowser(), false),
         1000);
@@ -2826,24 +2833,11 @@ class RequestTestHandler : public TestHandler {
 
     TestHandler::DestroyTest();
 
-    // For non-global contexts OnTestComplete() will be called when the
-    // RequestContextHandler is destroyed.
-    bool call_test_complete = false;
-    if (context_mode_ == CONTEXT_GLOBAL) {
-      if (!test_frame_method_) {
-        // These tests don't create a browser that would signal implicitly.
-        call_test_complete = true;
-      } else if (!SignalCompletionWhenAllBrowsersClose()) {
-        // These tests close the browser to terminate in-progress requests
-        // before test completion.
-        call_test_complete = true;
-      }
-    }
-
     // Release references to the context and handler.
     test_runner_->Destroy();
 
-    if (call_test_complete) {
+    // These tests don't create a browser that would signal implicitly.
+    if (!test_frame_method_) {
       OnTestComplete();
     }
   }
@@ -2855,15 +2849,12 @@ class RequestTestHandler : public TestHandler {
       return;
     }
 
-    EXPECT_FALSE(got_on_test_complete_);
-    got_on_test_complete_.yes();
-
     if (!context_tmpdir_.IsEmpty()) {
       // Temp directory will be deleted on application shutdown.
       context_tmpdir_.Take();
     }
 
-    TestComplete();
+    SignalTestCompletion();
   }
 
  private:
@@ -2904,7 +2895,6 @@ class RequestTestHandler : public TestHandler {
 
  public:
   int auth_credentials_ct_ = 0;
-  TrackCallback got_on_test_complete_;
 
   IMPLEMENT_REFCOUNTING(RequestTestHandler);
 };