Skip to content

Commit

Permalink
arc: Fix crash on item name update in app list.
Browse files Browse the repository at this point in the history
This fixes 2 issues. First one when name is set it sends whole metadata
to ash side. That data might included invalid position (true for OEM
folder, when it is created from scratch it contains default invalid
position which is corrected on ash side). As a result correct position
is overwritten by initial invalid position and this put app list model
to "crash ready" state. Crash is triggered when another code accesses
this position for own purpose.

Second problem is flakiness in setting OEM folder name. It may happen
when app_list_controller_ is not active and this call is just ignored
and folder appears in app list with name "OEM Folder" instead of
Samsung. To fix this we need make sure that ChromeAppListItem itself is
also updated.

Test: Locally
Bug: b/78473786
Change-Id: If15efc8103e56b04a9da3bc6de4664eb5676d12a
Reviewed-on: https://chromium-review.googlesource.com/1029174
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Yury Khmel <khmel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554119}
  • Loading branch information
yurykhmel authored and Commit Bot committed Apr 26, 2018
1 parent e828005 commit bb3bab6
Show file tree
Hide file tree
Showing 6 changed files with 36 additions and 15 deletions.
6 changes: 5 additions & 1 deletion ash/app_list/app_list_controller_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -181,8 +181,12 @@ void AppListControllerImpl::PublishSearchResults(
void AppListControllerImpl::SetItemMetadata(const std::string& id,
AppListItemMetadataPtr data) {
app_list::AppListItem* item = model_.FindItem(id);
if (item)
if (item) {
// data may not contain valid position. Preserve it in this case.
if (!data->position.IsValid())
data->position = item->position();
item->SetMetadata(std::move(data));
}
}

void AppListControllerImpl::SetItemIcon(const std::string& id,
Expand Down
7 changes: 6 additions & 1 deletion chrome/browser/ui/app_list/app_list_syncable_service.cc
Original file line number Diff line number Diff line change
Expand Up @@ -439,7 +439,12 @@ AppListSyncableService::GetSyncItem(const std::string& id) const {

void AppListSyncableService::SetOemFolderName(const std::string& name) {
oem_folder_name_ = name;
model_updater_->SetItemName(ash::kOemFolderId, oem_folder_name_);
// Update OEM folder item if it was already created. If it is not created yet
// then on creation it will take right name.
ChromeAppListItem* oem_folder_item =
model_updater_->FindItem(ash::kOemFolderId);
if (oem_folder_item)
oem_folder_item->SetName(oem_folder_name_);
}

AppListModelUpdater* AppListSyncableService::GetModelUpdater() {
Expand Down
25 changes: 16 additions & 9 deletions chrome/browser/ui/app_list/chrome_app_list_item.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,14 @@ namespace {

AppListControllerDelegate* g_controller_for_test = nullptr;

ash::mojom::AppListItemMetadataPtr CreateDefaultMetadata(
const std::string& app_id) {
return ash::mojom::AppListItemMetadata::New(
app_id, std::string() /* name */, std::string() /* short_name */,
std::string() /* folder_id */, syncer::StringOrdinal(),
false /* is_folder */, gfx::ImageSkia() /* icon */);
}

} // namespace

// static
Expand Down Expand Up @@ -49,15 +57,14 @@ void ChromeAppListItem::TestApi::SetPosition(
// ChromeAppListItem
ChromeAppListItem::ChromeAppListItem(Profile* profile,
const std::string& app_id)
: metadata_(
ash::mojom::AppListItemMetadata::New(app_id,
std::string() /* name */,
std::string() /* short_name */,
std::string() /* folder_id */,
syncer::StringOrdinal(),
false /* is_folder */,
gfx::ImageSkia() /* icon */)),
profile_(profile) {}
: metadata_(CreateDefaultMetadata(app_id)), profile_(profile) {}

ChromeAppListItem::ChromeAppListItem(Profile* profile,
const std::string& app_id,
AppListModelUpdater* model_updater)
: metadata_(CreateDefaultMetadata(app_id)),
profile_(profile),
model_updater_(model_updater) {}

ChromeAppListItem::~ChromeAppListItem() {
}
Expand Down
6 changes: 5 additions & 1 deletion chrome/browser/ui/app_list/chrome_app_list_item.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ class ChromeAppListItem {
ChromeAppListItem* const item_;
};

ChromeAppListItem(Profile* profile, const std::string& app_id);
ChromeAppListItem(Profile* profile,
const std::string& app_id,
AppListModelUpdater* model_updater);
virtual ~ChromeAppListItem();

// AppListControllerDelegate is not properly implemented in tests. Use mock
Expand Down Expand Up @@ -106,6 +108,8 @@ class ChromeAppListItem {
std::string ToDebugString() const;

protected:
ChromeAppListItem(Profile* profile, const std::string& app_id);

Profile* profile() const { return profile_; }

extensions::AppSorting* GetAppSorting();
Expand Down
5 changes: 3 additions & 2 deletions chrome/browser/ui/app_list/chrome_app_list_model_updater.cc
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,7 @@ void ChromeAppListModelUpdater::AddItemToOemFolder(
ChromeAppListItem* oem_folder = FindFolderItem(oem_folder_id);
if (!oem_folder) {
std::unique_ptr<ChromeAppListItem> new_oem_folder =
std::make_unique<ChromeAppListItem>(profile_, oem_folder_id);
std::make_unique<ChromeAppListItem>(profile_, oem_folder_id, this);
oem_folder = AddChromeItem(std::move(new_oem_folder));
oem_folder->SetChromeIsFolder(true);
}
Expand Down Expand Up @@ -504,9 +504,10 @@ void ChromeAppListModelUpdater::OnFolderCreated(
// Otherwise, we detect an item is created in Ash which is not added into our
// Chrome list yet. This only happens when a folder is created.
std::unique_ptr<ChromeAppListItem> new_item =
std::make_unique<ChromeAppListItem>(profile_, item->id);
std::make_unique<ChromeAppListItem>(profile_, item->id, this);
chrome_item = AddChromeItem(std::move(new_item));
chrome_item->SetMetadata(std::move(item));

if (delegate_)
delegate_->OnAppListItemAdded(chrome_item);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ FakeAppListModelUpdater::FindOrCreateOemFolder(
oem_folder->SetMetadata(std::move(folder_data));
} else {
std::unique_ptr<ChromeAppListItem> new_folder =
std::make_unique<ChromeAppListItem>(nullptr, oem_folder_id);
std::make_unique<ChromeAppListItem>(nullptr, oem_folder_id, nullptr);
oem_folder = new_folder.get();
AddItem(std::move(new_folder));
ash::mojom::AppListItemMetadataPtr folder_data =
Expand Down

0 comments on commit bb3bab6

Please sign in to comment.