Commit 210fb6b5 authored by nancylingwang's avatar nancylingwang Committed by Commit Bot

Fix the crash issue when loading more than 200 Arc app icons.

When more than 200 Arc apps are installed, due to different icon size,
scale, too many files are opened, causing the system crash. Add a
limitation to control icon loading requests to be less than 250. If
there are more icon loading requests, add them to the pending list, and
when some icon requests finished, continue loading icons from the
pending list.

Modify ArcAppIcon to observer_->OnIconUpdated to notify the observer for
all error cases, because the observer in AppService depends on the
notification to update the pending list and the in-flight list.

BUG=1123796

Change-Id: I00ab3be5326e21574c1757cedacd7701e8b66710
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2386688Reviewed-by: default avatarLong Cheng <lgcheng@google.com>
Reviewed-by: default avatarDominick Ng <dominickn@chromium.org>
Commit-Queue: Nancy Wang <nancylingwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#804048}
parent fe049488
...@@ -9,6 +9,8 @@ ...@@ -9,6 +9,8 @@
namespace apps { namespace apps {
constexpr size_t kMaxSimultaneousIconRequests = 250;
// A part of an ArcIconOnceLoader, for a specific size_in_dip and // A part of an ArcIconOnceLoader, for a specific size_in_dip and
// icon_type. This two-level structure (an ArcIconOnceLoader contains // icon_type. This two-level structure (an ArcIconOnceLoader contains
// multiple SizeSpecificLoader instances) is needed because each ArcAppIcon is // multiple SizeSpecificLoader instances) is needed because each ArcAppIcon is
...@@ -17,7 +19,8 @@ class ArcIconOnceLoader::SizeSpecificLoader : public ArcAppIcon::Observer { ...@@ -17,7 +19,8 @@ class ArcIconOnceLoader::SizeSpecificLoader : public ArcAppIcon::Observer {
public: public:
SizeSpecificLoader(Profile* profile, SizeSpecificLoader(Profile* profile,
int32_t size_in_dip, int32_t size_in_dip,
apps::mojom::IconType icon_type); apps::mojom::IconType icon_type,
ArcIconOnceLoader& host);
~SizeSpecificLoader() override; ~SizeSpecificLoader() override;
void LoadIcon(const std::string& app_id, void LoadIcon(const std::string& app_id,
...@@ -27,11 +30,13 @@ class ArcIconOnceLoader::SizeSpecificLoader : public ArcAppIcon::Observer { ...@@ -27,11 +30,13 @@ class ArcIconOnceLoader::SizeSpecificLoader : public ArcAppIcon::Observer {
// ArcAppIcon::Observer overrides. // ArcAppIcon::Observer overrides.
void OnIconUpdated(ArcAppIcon* icon) override; void OnIconUpdated(ArcAppIcon* icon) override;
void OnIconFailed(ArcAppIcon* icon) override;
private: private:
Profile* const profile_; Profile* const profile_;
const int32_t size_in_dip_; const int32_t size_in_dip_;
const apps::mojom::IconType icon_type_; const apps::mojom::IconType icon_type_;
ArcIconOnceLoader& host_;
// Maps App IDs to their icon loaders (for a specific size_in_dip and // Maps App IDs to their icon loaders (for a specific size_in_dip and
// icon_compression). // icon_compression).
...@@ -46,8 +51,12 @@ class ArcIconOnceLoader::SizeSpecificLoader : public ArcAppIcon::Observer { ...@@ -46,8 +51,12 @@ class ArcIconOnceLoader::SizeSpecificLoader : public ArcAppIcon::Observer {
ArcIconOnceLoader::SizeSpecificLoader::SizeSpecificLoader( ArcIconOnceLoader::SizeSpecificLoader::SizeSpecificLoader(
Profile* profile, Profile* profile,
int32_t size_in_dip, int32_t size_in_dip,
apps::mojom::IconType icon_type) apps::mojom::IconType icon_type,
: profile_(profile), size_in_dip_(size_in_dip), icon_type_(icon_type) {} ArcIconOnceLoader& host)
: profile_(profile),
size_in_dip_(size_in_dip),
icon_type_(icon_type),
host_(host) {}
ArcIconOnceLoader::SizeSpecificLoader::~SizeSpecificLoader() { ArcIconOnceLoader::SizeSpecificLoader::~SizeSpecificLoader() {
for (auto& kv_pair : callbacks_) { for (auto& kv_pair : callbacks_) {
...@@ -89,17 +98,26 @@ void ArcIconOnceLoader::SizeSpecificLoader::LoadIcon( ...@@ -89,17 +98,26 @@ void ArcIconOnceLoader::SizeSpecificLoader::LoadIcon(
icon_type = ArcAppIcon::IconType::kAdaptive; icon_type = ArcAppIcon::IconType::kAdaptive;
break; break;
} }
iter = icons_ iter = icons_
.insert(std::make_pair( .insert(std::make_pair(
app_id, std::make_unique<ArcAppIcon>( app_id, std::make_unique<ArcAppIcon>(
profile_, app_id, size_in_dip_, this, icon_type))) profile_, app_id, size_in_dip_, this, icon_type)))
.first; .first;
if (base::FeatureList::IsEnabled(features::kAppServiceAdaptiveIcon)) {
host_.MaybeStartIconRequest(iter->second.get(),
ui::ScaleFactor::NUM_SCALE_FACTORS);
return;
}
iter->second->LoadSupportedScaleFactors(); iter->second->LoadSupportedScaleFactors();
} }
void ArcIconOnceLoader::SizeSpecificLoader::Remove(const std::string& app_id) { void ArcIconOnceLoader::SizeSpecificLoader::Remove(const std::string& app_id) {
auto iter = icons_.find(app_id); auto iter = icons_.find(app_id);
if (iter != icons_.end()) { if (iter != icons_.end()) {
if (base::FeatureList::IsEnabled(features::kAppServiceAdaptiveIcon)) {
host_.RemoveArcAppIcon(iter->second.get());
}
icons_.erase(iter); icons_.erase(iter);
} }
} }
...@@ -109,14 +127,27 @@ void ArcIconOnceLoader::SizeSpecificLoader::Reload( ...@@ -109,14 +127,27 @@ void ArcIconOnceLoader::SizeSpecificLoader::Reload(
ui::ScaleFactor scale_factor) { ui::ScaleFactor scale_factor) {
auto iter = icons_.find(app_id); auto iter = icons_.find(app_id);
if (iter != icons_.end()) { if (iter != icons_.end()) {
if (base::FeatureList::IsEnabled(features::kAppServiceAdaptiveIcon)) {
host_.MaybeStartIconRequest(iter->second.get(), scale_factor);
return;
}
iter->second->LoadForScaleFactor(scale_factor); iter->second->LoadForScaleFactor(scale_factor);
} }
} }
void ArcIconOnceLoader::SizeSpecificLoader::OnIconUpdated(ArcAppIcon* icon) { void ArcIconOnceLoader::SizeSpecificLoader::OnIconUpdated(ArcAppIcon* icon) {
if (!icon || !icon->EverySupportedScaleFactorIsLoaded()) { if (!icon) {
return; return;
} }
if (base::FeatureList::IsEnabled(features::kAppServiceAdaptiveIcon)) {
host_.RemoveArcAppIcon(icon);
}
if (!icon->EverySupportedScaleFactorIsLoaded()) {
return;
}
auto range = callbacks_.equal_range(icon->app_id()); auto range = callbacks_.equal_range(icon->app_id());
auto count = std::distance(range.first, range.second); auto count = std::distance(range.first, range.second);
if (count <= 0) { if (count <= 0) {
...@@ -149,14 +180,18 @@ void ArcIconOnceLoader::SizeSpecificLoader::OnIconUpdated(ArcAppIcon* icon) { ...@@ -149,14 +180,18 @@ void ArcIconOnceLoader::SizeSpecificLoader::OnIconUpdated(ArcAppIcon* icon) {
} }
} }
void ArcIconOnceLoader::SizeSpecificLoader::OnIconFailed(ArcAppIcon* icon) {
OnIconUpdated(icon);
}
ArcIconOnceLoader::ArcIconOnceLoader(Profile* profile) ArcIconOnceLoader::ArcIconOnceLoader(Profile* profile)
: profile_(profile), stop_observing_called_(false) { : profile_(profile), stop_observing_called_(false) {
ArcAppListPrefs::Get(profile)->AddObserver(this); ArcAppListPrefs::Get(profile)->AddObserver(this);
} }
ArcIconOnceLoader::~ArcIconOnceLoader() { ArcIconOnceLoader::~ArcIconOnceLoader() {
// Check that somebody called StopObserving. We can't call StopObserving here // Check that somebody called StopObserving. We can't call StopObserving
// in the destructor, because we need a ArcAppListPrefs* prefs, and for // here in the destructor, because we need a ArcAppListPrefs* prefs, and for
// tests, the prefs pointer for a profile can change over time (e.g. by // tests, the prefs pointer for a profile can change over time (e.g. by
// ArcAppListPrefsFactory::RecreateServiceInstanceForTesting). // ArcAppListPrefsFactory::RecreateServiceInstanceForTesting).
// //
...@@ -179,11 +214,11 @@ void ArcIconOnceLoader::LoadIcon( ...@@ -179,11 +214,11 @@ void ArcIconOnceLoader::LoadIcon(
auto key = std::make_pair(size_in_dip, icon_type); auto key = std::make_pair(size_in_dip, icon_type);
auto iter = size_specific_loaders_.find(key); auto iter = size_specific_loaders_.find(key);
if (iter == size_specific_loaders_.end()) { if (iter == size_specific_loaders_.end()) {
iter = iter = size_specific_loaders_
size_specific_loaders_ .insert(std::make_pair(
.insert(std::make_pair(key, std::make_unique<SizeSpecificLoader>( key, std::make_unique<SizeSpecificLoader>(
profile_, size_in_dip, icon_type))) profile_, size_in_dip, icon_type, *this)))
.first; .first;
} }
iter->second->LoadIcon(app_id, std::move(callback)); iter->second->LoadIcon(app_id, std::move(callback));
} }
...@@ -207,4 +242,54 @@ void ArcIconOnceLoader::OnAppIconUpdated( ...@@ -207,4 +242,54 @@ void ArcIconOnceLoader::OnAppIconUpdated(
} }
} }
void ArcIconOnceLoader::MaybeStartIconRequest(ArcAppIcon* arc_app_icon,
ui::ScaleFactor scale_factor) {
DCHECK(arc_app_icon);
if (in_flight_requests_.size() < kMaxSimultaneousIconRequests) {
in_flight_requests_.insert(arc_app_icon);
if (scale_factor == ui::ScaleFactor::NUM_SCALE_FACTORS) {
arc_app_icon->LoadSupportedScaleFactors();
} else {
arc_app_icon->LoadForScaleFactor(scale_factor);
}
return;
}
pending_requests_[arc_app_icon].insert(scale_factor);
}
void ArcIconOnceLoader::RemoveArcAppIcon(ArcAppIcon* arc_app_icon) {
DCHECK(arc_app_icon);
in_flight_requests_.erase(arc_app_icon);
pending_requests_.erase(arc_app_icon);
MaybeLoadPendingIconRequest();
}
void ArcIconOnceLoader::MaybeLoadPendingIconRequest() {
while (!pending_requests_.empty() &&
in_flight_requests_.size() < kMaxSimultaneousIconRequests) {
auto it = pending_requests_.begin();
ArcAppIcon* arc_app_icon = it->first;
DCHECK(arc_app_icon);
std::set<ui::ScaleFactor>& scale_factors = it->second;
DCHECK(!scale_factors.empty());
// Handle all pending icon loading requests for |arc_app_icon|.
for (auto scale_factor : scale_factors) {
if (scale_factor == ui::ScaleFactor::NUM_SCALE_FACTORS) {
arc_app_icon->LoadSupportedScaleFactors();
} else {
arc_app_icon->LoadForScaleFactor(scale_factor);
}
}
in_flight_requests_.insert(arc_app_icon);
pending_requests_.erase(it);
}
}
} // namespace apps } // namespace apps
...@@ -58,11 +58,36 @@ class ArcIconOnceLoader : public ArcAppListPrefs::Observer { ...@@ -58,11 +58,36 @@ class ArcIconOnceLoader : public ArcAppListPrefs::Observer {
using SizeAndType = std::pair<int32_t, apps::mojom::IconType>; using SizeAndType = std::pair<int32_t, apps::mojom::IconType>;
// When loading many app icons, there could be many icon files opened at the
// same time, which might cause the system crash. So checking the current icon
// loading request number, and if there are too many requests, add the
// ArcAppIcon to |pending_requests_| to load the icon later, and return
// false. Otherwise add the ArcAppIcon to |in_flight_requests_| so that we
// can calculate how many in flight icon loading requests, and return true.
void MaybeStartIconRequest(ArcAppIcon* arc_app_icon,
ui::ScaleFactor scale_factor);
// When get the reply from |arc_app_icon| or remove the app, remove
// |arc_app_icon| from |in_flight_requests_| and |pending_requests_|, and
// start loading icon requests from |pending_requests_|.
void RemoveArcAppIcon(ArcAppIcon* arc_app_icon);
// If there are loading icon requests saved in |pending_requests_|,
// and not too many requests, get ArcAppIcon from
// |pending_requests_| to load icons.
void MaybeLoadPendingIconRequest();
Profile* const profile_; Profile* const profile_;
bool stop_observing_called_; bool stop_observing_called_;
std::map<SizeAndType, std::unique_ptr<SizeSpecificLoader>> std::map<SizeAndType, std::unique_ptr<SizeSpecificLoader>>
size_specific_loaders_; size_specific_loaders_;
// The current icon loading requests.
std::set<ArcAppIcon*> in_flight_requests_;
// The ArcAppIcon map to record the pending icon loading requests.
std::map<ArcAppIcon*, std::set<ui::ScaleFactor>> pending_requests_;
DISALLOW_COPY_AND_ASSIGN(ArcIconOnceLoader); DISALLOW_COPY_AND_ASSIGN(ArcIconOnceLoader);
}; };
......
...@@ -212,13 +212,13 @@ void ArcAppIcon::DecodeRequest::OnImageDecoded(const SkBitmap& bitmap) { ...@@ -212,13 +212,13 @@ void ArcAppIcon::DecodeRequest::OnImageDecoded(const SkBitmap& bitmap) {
incomplete_scale_factors_); incomplete_scale_factors_);
} }
host_.DiscardDecodeRequest(this); host_.DiscardDecodeRequest(this, true /* bool is_decode_success */);
} }
void ArcAppIcon::DecodeRequest::OnDecodeImageFailed() { void ArcAppIcon::DecodeRequest::OnDecodeImageFailed() {
VLOG(2) << "Failed to decode ARC icon."; VLOG(2) << "Failed to decode ARC icon.";
host_.MaybeRequestIcon(descriptor_.scale_factor); host_.MaybeRequestIcon(descriptor_.scale_factor);
host_.DiscardDecodeRequest(this); host_.DiscardDecodeRequest(this, false /* bool is_decode_success*/);
} }
//////////////////////////////////////////////////////////////////////////////// ////////////////////////////////////////////////////////////////////////////////
...@@ -617,8 +617,10 @@ void ArcAppIcon::OnIconRead( ...@@ -617,8 +617,10 @@ void ArcAppIcon::OnIconRead(
if (read_result->request_to_install) if (read_result->request_to_install)
MaybeRequestIcon(read_result->scale_factor); MaybeRequestIcon(read_result->scale_factor);
if (read_result->unsafe_icon_data.empty()) if (read_result->unsafe_icon_data.empty()) {
observer_->OnIconFailed(this);
return; return;
}
switch (icon_type_) { switch (icon_type_) {
case IconType::kUncompressed: { case IconType::kUncompressed: {
...@@ -741,7 +743,8 @@ void ArcAppIcon::UpdateCompressed(ui::ScaleFactor scale_factor, ...@@ -741,7 +743,8 @@ void ArcAppIcon::UpdateCompressed(ui::ScaleFactor scale_factor,
observer_->OnIconUpdated(this); observer_->OnIconUpdated(this);
} }
void ArcAppIcon::DiscardDecodeRequest(DecodeRequest* request) { void ArcAppIcon::DiscardDecodeRequest(DecodeRequest* request,
bool is_decode_success) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI); DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
auto it = std::find_if(decode_requests_.begin(), decode_requests_.end(), auto it = std::find_if(decode_requests_.begin(), decode_requests_.end(),
...@@ -750,4 +753,7 @@ void ArcAppIcon::DiscardDecodeRequest(DecodeRequest* request) { ...@@ -750,4 +753,7 @@ void ArcAppIcon::DiscardDecodeRequest(DecodeRequest* request) {
}); });
DCHECK(it != decode_requests_.end()); DCHECK(it != decode_requests_.end());
decode_requests_.erase(it); decode_requests_.erase(it);
if (!is_decode_success)
observer_->OnIconFailed(this);
} }
...@@ -47,6 +47,9 @@ class ArcAppIcon { ...@@ -47,6 +47,9 @@ class ArcAppIcon {
// is loaded and added to |image|. // is loaded and added to |image|.
virtual void OnIconUpdated(ArcAppIcon* icon) = 0; virtual void OnIconUpdated(ArcAppIcon* icon) = 0;
// Invoked when failed to generate an icon.
virtual void OnIconFailed(ArcAppIcon* icon) {}
protected: protected:
virtual ~Observer() {} virtual ~Observer() {}
}; };
...@@ -213,7 +216,7 @@ class ArcAppIcon { ...@@ -213,7 +216,7 @@ class ArcAppIcon {
gfx::ImageSkia& image_skia, gfx::ImageSkia& image_skia,
std::map<ui::ScaleFactor, base::Time>& incomplete_scale_factors); std::map<ui::ScaleFactor, base::Time>& incomplete_scale_factors);
void UpdateCompressed(ui::ScaleFactor scale_factor, std::string data); void UpdateCompressed(ui::ScaleFactor scale_factor, std::string data);
void DiscardDecodeRequest(DecodeRequest* request); void DiscardDecodeRequest(DecodeRequest* request, bool is_decode_success);
content::BrowserContext* const context_; content::BrowserContext* const context_;
const std::string app_id_; const std::string app_id_;
......
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