Commit e7104328 authored by Daniel Murphy's avatar Daniel Murphy Committed by Chromium LUCI CQ

[WebAppProvider] Prevent browser process crash during WebApp migration

If the user starts chrome by launching the WebApp that is being migrated
from, then Chrome crashed due to shutting down while the browser object
is still being used by the migration code. This patch ensures that the
browser doesn't exit during migration, and only keeps the browser alive
during the migration itself.

Note: This WebAppMover is not a permanent class, and will be removed
after the Chat app is successfully migrated.

Design Doc (Googlers only):
https://docs.google.com/document/d/1GOek_Q4O_jTewHHx0MLnbYXvlTdEKT-KmkPZ5R0VJ3c/edit?resourcekey=0-K35fuzesohP36-qKSGbp0g

Bug: 1165603
Change-Id: I3a38402513e3dacdd05b5d516b01f371043d5206
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2622584
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: default avatarColin Blundell <blundell@chromium.org>
Reviewed-by: default avatarChase Phillips <cmp@chromium.org>
Auto-Submit: Daniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843680}
parent 29f69353
......@@ -85,6 +85,7 @@ source_set("web_applications") {
"//chrome/common",
"//components/content_settings/core/browser",
"//components/device_event_log",
"//components/keep_alive_registry:keep_alive_registry",
"//components/keyed_service/content",
"//components/performance_manager",
"//components/pref_registry",
......
......@@ -18,6 +18,7 @@
#include "chrome/browser/web_applications/components/install_finalizer.h"
#include "chrome/browser/web_applications/components/install_manager.h"
#include "chrome/common/chrome_features.h"
#include "components/keep_alive_registry/keep_alive_types.h"
#include "components/webapps/browser/installable/installable_metrics.h"
#include "content/public/browser/web_contents.h"
......@@ -117,12 +118,16 @@ void WebAppMover::Start() {
void WebAppMover::Shutdown() {
weak_ptr_factory_.InvalidateWeakPtrs();
sync_observer_.Reset();
migration_keep_alive_.reset();
}
void WebAppMover::OnSyncCycleCompleted(syncer::SyncService* sync_service) {
DCHECK_EQ(sync_service_, sync_service);
if (sync_ready_callback_)
std::move(sync_ready_callback_).Run();
// Only the first cycle cycle matters, as this triggers the WebAppMover logic,
// and |sync_ready_callback_| is never set again. Thus we can stop observing.
sync_observer_.Reset();
}
void WebAppMover::OnSyncShutdown(syncer::SyncService* sync_service) {
......@@ -193,6 +198,10 @@ void WebAppMover::OnInstallManifestFetched(
}
DCHECK(!apps_to_uninstall_.empty());
migration_keep_alive_ = std::make_unique<ScopedKeepAlive>(
KeepAliveOrigin::APP_START_URL_MIGRATION,
KeepAliveRestartOption::DISABLED);
scoped_refptr<base::RefCountedData<bool>> success_accumulator =
base::MakeRefCounted<base::RefCountedData<bool>>(true);
......@@ -261,6 +270,7 @@ void WebAppMover::OnInstallCompleted(
} else {
LOG(WARNING) << "Installation in app move operation failed: " << code;
}
migration_keep_alive_.reset();
}
} // namespace web_app
......@@ -15,6 +15,7 @@
#include "base/scoped_observation.h"
#include "chrome/browser/web_applications/components/install_manager.h"
#include "chrome/browser/web_applications/components/web_app_id.h"
#include "components/keep_alive_registry/scoped_keep_alive.h"
#include "components/sync/driver/sync_service.h"
#include "components/sync/driver/sync_service_observer.h"
#include "url/gurl.h"
......@@ -99,6 +100,7 @@ class WebAppMover final : public syncer::SyncServiceObserver {
bool new_app_open_as_window_ = false;
std::vector<AppId> apps_to_uninstall_;
std::unique_ptr<ScopedKeepAlive> migration_keep_alive_;
base::ScopedObservation<syncer::SyncService, syncer::SyncServiceObserver>
sync_observer_{this};
......
......@@ -44,6 +44,8 @@ std::ostream& operator<<(std::ostream& out, const KeepAliveOrigin& origin) {
return out << "APP_LIST_SERVICE_VIEWS";
case KeepAliveOrigin::APP_LIST_SHOWER:
return out << "APP_LIST_SHOWER";
case KeepAliveOrigin::APP_START_URL_MIGRATION:
return out << "APP_START_URL_MIGRATION";
case KeepAliveOrigin::APP_UNINSTALLATION_FROM_OS_SETTINGS:
return out << "APP_UNINSTALLATION_FROM_OS_SETTINGS";
case KeepAliveOrigin::CHROME_APP_DELEGATE:
......
......@@ -61,6 +61,9 @@ enum class KeepAliveOrigin {
PROFILE_LOADER,
USER_MANAGER_VIEW,
CREDENTIAL_PROVIDER_SIGNIN_DIALOG,
// c/b/web_applications
APP_START_URL_MIGRATION,
};
// Restart: Allow Chrome to restart when all the registered KeepAlives allow
......
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