Commit c131daf4 authored by Nigel Tao's avatar Nigel Tao Committed by Commit Bot

Have ArcApps not cache its ArcAppListPrefs pointer

It is incorrect to cache the value of ArcAppListPrefs::Get(profile). The
value returned can change over the lifetime of the profile.

Also move observer clean-up from the ArcApps destructor to its
KeyedService::Shutdown method, since the ArcApps keyed service depends
on the AALP keyed service.

This fixes the ArcAppLauncherForDefaultAppTest.* unit_tests in debug
mode, when combined with --enable-features=AppServiceAsh. The tests
passed in release mode, both before and after this commit.

In the unit_tests, ArcAppModelBuilderRecreate::StartArc calls
ArcAppListPrefsFactory::GetInstance()->RecreateServiceInstanceForTesting,
which destroys the existing ArcAppListPrefs and creates a new one.
Meanwhile, ArcApps code continues to follow the old pointer.

In the release build, there's the (un)happy coincidence that the new
AALP's pointer value is exactly the same as the old one: the memory is
re-used and that 'dangling' pointer still points to a valid AALP object.
In the debug build, malloc/free behavior is presumably different (the
pointer value is certainly different), and the latent bug surfaces.

BUG=826982

Change-Id: Ie77fb2ab8c65662d66faff8947901414d9cb1892
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1732110
Commit-Queue: Nigel Tao <nigeltao@chromium.org>
Reviewed-by: default avatarDominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#683475}
parent f37f7fde
This diff is collapsed.
......@@ -50,6 +50,9 @@ class ArcApps : public KeyedService,
private:
ArcApps(Profile* profile, apps::AppServiceProxy* proxy);
// KeyedService overrides.
void Shutdown() override;
// apps::mojom::Publisher overrides.
void Connect(apps::mojom::SubscriberPtr subscriber,
apps::mojom::ConnectOptionsPtr opts) override;
......@@ -101,7 +104,8 @@ class ArcApps : public KeyedService,
IconEffects icon_effects,
LoadIconCallback callback);
apps::mojom::AppPtr Convert(const std::string& app_id,
apps::mojom::AppPtr Convert(ArcAppListPrefs* prefs,
const std::string& app_id,
const ArcAppListPrefs::AppInfo& app_info);
void Publish(apps::mojom::AppPtr app);
void ConvertAndPublishPackageApps(
......@@ -111,7 +115,6 @@ class ArcApps : public KeyedService,
mojo::InterfacePtrSet<apps::mojom::Subscriber> subscribers_;
Profile* profile_;
ArcAppListPrefs* prefs_;
std::vector<base::OnceCallback<void(AppConnectionHolder*)>>
pending_load_icon_calls_;
......
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