Skip to content

Commit

Permalink
Add a few CHECK()s to troubleshoot crbug.com/1342152
Browse files Browse the repository at this point in the history
This adds a few CHECK()s to enforce the assume invariants while
destroying browser contexts during headless shutdown.

Bug: 1342152
Change-Id: I496f1cb231bbe892e537afe694f73e45ad089536
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3827419
Reviewed-by: Peter Kvitek <kvitekp@chromium.org>
Commit-Queue: Peter Kvitek <kvitekp@chromium.org>
Auto-Submit: Andrey Kosyakov <caseq@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1034985}
  • Loading branch information
caseq authored and Chromium LUCI CQ committed Aug 15, 2022
1 parent d6c033e commit 39d2573
Show file tree
Hide file tree
Showing 4 changed files with 11 additions and 8 deletions.
10 changes: 3 additions & 7 deletions headless/lib/browser/headless_browser_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ HeadlessBrowserImpl::BrowserMainThread() const {

void HeadlessBrowserImpl::Shutdown() {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
did_shutdown_ = true;

weak_ptr_factory_.InvalidateWeakPtrs();
// Make sure GetAllBrowserContexts is sane if called after this point.
Expand Down Expand Up @@ -103,15 +104,10 @@ HeadlessBrowserContext* HeadlessBrowserImpl::CreateBrowserContext(
HeadlessBrowserContext::Builder* builder) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);

std::unique_ptr<HeadlessBrowserContextImpl> browser_context =
HeadlessBrowserContextImpl::Create(builder);

if (!browser_context) {
return nullptr;
}
CHECK(!did_shutdown_);

auto browser_context = HeadlessBrowserContextImpl::Create(builder);
HeadlessBrowserContext* result = browser_context.get();

browser_contexts_[browser_context->Id()] = std::move(browser_context);

return result;
Expand Down
4 changes: 3 additions & 1 deletion headless/lib/browser/headless_browser_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,8 @@ class HEADLESS_EXPORT HeadlessBrowserImpl : public HeadlessBrowser,
policy::PolicyService* GetPolicyService();
#endif

bool did_shutdown() const { return did_shutdown_; }

protected:
#if BUILDFLAG(IS_MAC)
std::unique_ptr<display::ScopedNativeScreen> screen_;
Expand All @@ -133,7 +135,7 @@ class HEADLESS_EXPORT HeadlessBrowserImpl : public HeadlessBrowser,
base::flat_map<std::string, std::unique_ptr<HeadlessBrowserContextImpl>>
browser_contexts_;
raw_ptr<HeadlessBrowserContext> default_browser_context_; // Not owned.

bool did_shutdown_ = false; // TODO(1342152): remove once the bug is fixed.
scoped_refptr<content::DevToolsAgentHost> agent_host_;
std::unique_ptr<HeadlessRequestContextManager>
system_request_context_manager_;
Expand Down
4 changes: 4 additions & 0 deletions headless/lib/browser/headless_browser_main_parts.cc
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@ void HeadlessBrowserMainParts::WillRunMainMessageLoop(
}

void HeadlessBrowserMainParts::PostMainMessageLoopRun() {
// HeadlessBrowserImpl::Shutdown() is supposed to remove all browser contexts
// and therefore all associated web contents, however crbug.com/1342152
// implies it may not be happening.
CHECK_EQ(0U, browser_->GetAllBrowserContexts().size());
if (devtools_http_handler_started_) {
StopLocalDevToolsHttpHandler();
devtools_http_handler_started_ = false;
Expand Down
1 change: 1 addition & 0 deletions headless/lib/headless_content_main_delegate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,7 @@ HeadlessContentMainDelegate::RunProcess(
"HeadlessContentMainDelegate::RunProcess";

browser_runner->Run();
CHECK(browser_->did_shutdown());
browser_runner->Shutdown();
browser_.reset();

Expand Down

0 comments on commit 39d2573

Please sign in to comment.