Commit 44a597b3 authored by Lutz Justen's avatar Lutz Justen Committed by Commit Bot

Fix flaky LoadAndLaunchExtension

Fixes the flaky LoadAndLaunchExtensionBrowserTest.LoadAndLaunchExtension
test on Linux. The test tries to run an extension as an app and fails
(expectedly) very quickly. This triggers a race condition in
NotificationPlatformBridgeLinuxImpl where the cleanup code is run before
the init code finishes.

In particular, InitOnTaskRunner() kicks off
notification_proxy_->ConnectToSignal(), which runs a task on another
thread. If CleanUpOnTaskRunner() runs before that task is executed,
the bus is shut down. ConnectToSignal() then reconnects the bus and
nothing shuts it down again, causing a DCHECK(!connection_) in
Bus::~Bus().

BUG=chromium:988160
TEST=Test works flakelessly on my workstation

Change-Id: I57148b8f51c6927582bc69577260078e6cf1e491
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1722959
Commit-Queue: Lutz Justen <ljusten@chromium.org>
Reviewed-by: default avatarLei Zhang <thestig@chromium.org>
Reviewed-by: default avatarThomas Anderson <thomasanderson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682699}
parent 4c370147
......@@ -487,6 +487,8 @@ class NotificationPlatformBridgeLinuxImpl
server_version_ = base::Version(server_version);
}
DCHECK(!connect_signals_in_progress_);
connect_signals_in_progress_ = true;
connected_signals_barrier_ = base::BarrierClosure(
2, base::Bind(&NotificationPlatformBridgeLinuxImpl::
OnConnectionInitializationFinishedOnTaskRunner,
......@@ -505,6 +507,12 @@ class NotificationPlatformBridgeLinuxImpl
}
void CleanUpOnTaskRunner() {
if (connect_signals_in_progress_) {
// Connecting to a signal is still in progress. Defer cleanup task.
should_cleanup_on_signal_connected_ = true;
return;
}
DCHECK(task_runner_->RunsTasksInCurrentSequence());
if (bus_)
bus_->ShutdownAndBlock();
......@@ -889,12 +897,21 @@ class NotificationPlatformBridgeLinuxImpl
"Notifications.Linux.BridgeInitializationStatus",
static_cast<int>(status),
static_cast<int>(ConnectionInitializationStatusCode::NUM_ITEMS));
bool success = status == ConnectionInitializationStatusCode::SUCCESS;
// Note: Not all code paths set connect_signals_in_progress_ to true!
connect_signals_in_progress_ = false;
if (should_cleanup_on_signal_connected_) {
// Mark as fail, so that observers don't think we're initialized.
success = false;
CleanUpOnTaskRunner();
}
base::PostTaskWithTraits(
FROM_HERE, {content::BrowserThread::UI},
base::BindOnce(&NotificationPlatformBridgeLinuxImpl::
OnConnectionInitializationFinishedOnUiThread,
this,
status == ConnectionInitializationStatusCode::SUCCESS));
this, success));
}
void OnSignalConnected(const std::string& interface_name,
......@@ -995,6 +1012,13 @@ class NotificationPlatformBridgeLinuxImpl
base::Closure connected_signals_barrier_;
// Whether ConnectToSignal() is in progress.
bool connect_signals_in_progress_ = false;
// Calling CleanUp() while ConnectToSignal() is in progress leads to a crash.
// This flag is used to defer the cleanup task until signals are connected.
bool should_cleanup_on_signal_connected_ = false;
scoped_refptr<base::RefCountedMemory> product_logo_png_bytes_;
std::unique_ptr<ResourceFile> product_logo_file_;
std::unique_ptr<base::FilePathWatcher> product_logo_file_watcher_;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment