New issue
Advanced search Search tips

Issue 839024 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Launcher has dummy entry with text 'OEM folder' or 'Samsung' and no icon

Project Member Reported by mkarkada@chromium.org, May 2 2018

Issue description

Chrome OS Version: 10575.25.0, 67.0.3396.31 dev-channel caroline, kevin

Issue is same as the one mentioned in Issue 828209

What steps will reproduce the problem?
(1) Add user account from sign in screen (i.e Sign in screen> click on Add Person button) 
(2) Open launcher and observe the app list
(3)

What happens instead?
There is a dummy entry with text 'OEM folder' or sometimes as 'Samsung' and no icon. I can drag and move this empty icon.

Attached screenshots
 
Screenshot 2018-05-02 at 11.15.59 AM.png
662 KB View Download
Screenshot 2018-05-01 at 3.38.16 PM.png
1.3 MB View Download
Owner: khmel@chromium.org
This is a duplicate bug which sdantuluri@ marked as verfied fixed in ChromeOS 10575.12.0, 67.0.3396.16 beta-channel soraka. 

Which device is this for?
Oh, I see, caroline and kevin.
This issue got repro'd on Chrome OS beta: 10575.32.0, 67.0.3396.41 kevin.

Comment 4 by khmel@chromium.org, May 9 2018

#3 - is it repro'd in M68?
No repro'd on caroline M68 build (10658.0.0, 68.0.3423.0).
Labels: ReleaseBlock-Stable
Project Member

Comment 7 by bugdroid1@chromium.org, May 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4decb912159aee0557f3170703ce1bde01951d51

commit 4decb912159aee0557f3170703ce1bde01951d51
Author: khmel@google.com <khmel@google.com>
Date: Wed May 16 15:53:25 2018

app_launcher: Fix race condition creating OEM folder.

This fixes one more race condition for OEM folder creation where it is
created on demand on ash side first. This also fixes issue when OEM
folder may appear in ash as normal folder. Plus does refactoring by
discarding passing redundant oem_folder_id.

Test: Manually
Bug:  839024 
Change-Id: I3844e8fbf24fc12c806f25f7c87973eebe167d02
Reviewed-on: https://chromium-review.googlesource.com/1060166
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Yury Khmel <khmel@google.com>
Cr-Commit-Position: refs/heads/master@{#559122}
[modify] https://crrev.com/4decb912159aee0557f3170703ce1bde01951d51/ash/app_list/app_list_controller_impl.cc
[modify] https://crrev.com/4decb912159aee0557f3170703ce1bde01951d51/ash/app_list/app_list_controller_impl.h
[modify] https://crrev.com/4decb912159aee0557f3170703ce1bde01951d51/ash/app_list/model/app_list_folder_item.cc
[modify] https://crrev.com/4decb912159aee0557f3170703ce1bde01951d51/ash/app_list/model/app_list_folder_item.h
[modify] https://crrev.com/4decb912159aee0557f3170703ce1bde01951d51/ash/app_list/model/app_list_model.cc
[modify] https://crrev.com/4decb912159aee0557f3170703ce1bde01951d51/ash/app_list/model/app_list_model_unittest.cc
[modify] https://crrev.com/4decb912159aee0557f3170703ce1bde01951d51/ash/public/interfaces/app_list.mojom
[modify] https://crrev.com/4decb912159aee0557f3170703ce1bde01951d51/chrome/browser/ui/app_list/app_list_model_updater.h
[modify] https://crrev.com/4decb912159aee0557f3170703ce1bde01951d51/chrome/browser/ui/app_list/app_list_syncable_service.cc
[modify] https://crrev.com/4decb912159aee0557f3170703ce1bde01951d51/chrome/browser/ui/app_list/chrome_app_list_model_updater.cc
[modify] https://crrev.com/4decb912159aee0557f3170703ce1bde01951d51/chrome/browser/ui/app_list/chrome_app_list_model_updater.h
[modify] https://crrev.com/4decb912159aee0557f3170703ce1bde01951d51/chrome/browser/ui/app_list/test/fake_app_list_model_updater.cc
[modify] https://crrev.com/4decb912159aee0557f3170703ce1bde01951d51/chrome/browser/ui/app_list/test/fake_app_list_model_updater.h
[modify] https://crrev.com/4decb912159aee0557f3170703ce1bde01951d51/ui/app_list/test/app_list_test_model.cc
[modify] https://crrev.com/4decb912159aee0557f3170703ce1bde01951d51/ui/app_list/test/app_list_test_model.h
[modify] https://crrev.com/4decb912159aee0557f3170703ce1bde01951d51/ui/app_list/views/folder_header_view_unittest.cc

Comment 8 by khmel@chromium.org, May 16 2018

Labels: Merge-Request-67
Status: Started (was: Untriaged)
Project Member

Comment 9 by sheriffbot@chromium.org, May 16 2018

Labels: -Merge-Request-67 Merge-Review-67 Hotlist-Merge-Review
This bug requires manual review: We are only 12 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
This is a fairly large change this late into M67.  Please provide testing details to verify this is fixed and no unanticipated impact.  Also, do you know why it didn't get resolved per earlier comments?


Comment 11 by khmel@chromium.org, May 16 2018

chrome->ash IPC was introducing recently (66 IIRC) and we had significant amount of race conditions, edge cases. So many problems may look similar but caused by difference issues.

This change actually is not big, It just remove redundant parameter from mojom and this casued mechanical files changes. 
Key changes are:
https://chromium-review.googlesource.com/c/chromium/src/+/1060166/5/ash/app_list/model/app_list_folder_item.cc#16

https://chromium-review.googlesource.com/c/chromium/src/+/1060166/5/chrome/browser/ui/app_list/app_list_syncable_service.cc#256

So I would consider risk as low.
Thanks for the details. Testing passed, no issues?

Comment 13 by khmel@chromium.org, May 18 2018

Locally verified with official build R68-10688.0.0
Labels: -Merge-Review-67 Merge-Approved-67
Merge approved, M67.
Project Member

Comment 15 by sheriffbot@chromium.org, May 22 2018

Cc: cindyb@chromium.org
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 16 by bugdroid1@chromium.org, May 22 2018

Labels: -merge-approved-67 merge-merged-3396
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f8d8f00c3de844782ae4c9da1db8a3e0a19bac7a

commit f8d8f00c3de844782ae4c9da1db8a3e0a19bac7a
Author: khmel@google.com <khmel@google.com>
Date: Tue May 22 16:29:02 2018

[Merge M67] app_launcher: Fix race condition creating OEM folder.

This fixes one more race condition for OEM folder creation where it is
created on demand on ash side first. This also fixes issue when OEM
folder may appear in ash as normal folder. Plus does refactoring by
discarding passing redundant oem_folder_id.

TBR=khmel@google.com

(cherry picked from commit 4decb912159aee0557f3170703ce1bde01951d51)

Test: Manually
Bug:  839024 
Change-Id: I3844e8fbf24fc12c806f25f7c87973eebe167d02
Reviewed-on: https://chromium-review.googlesource.com/1060166
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Yury Khmel <khmel@google.com>
Cr-Original-Commit-Position: refs/heads/master@{#559122}
Reviewed-on: https://chromium-review.googlesource.com/1066464
Reviewed-by: Yury Khmel <khmel@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#675}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/f8d8f00c3de844782ae4c9da1db8a3e0a19bac7a/ash/app_list/app_list_controller_impl.cc
[modify] https://crrev.com/f8d8f00c3de844782ae4c9da1db8a3e0a19bac7a/ash/app_list/app_list_controller_impl.h
[modify] https://crrev.com/f8d8f00c3de844782ae4c9da1db8a3e0a19bac7a/ash/app_list/model/app_list_folder_item.cc
[modify] https://crrev.com/f8d8f00c3de844782ae4c9da1db8a3e0a19bac7a/ash/app_list/model/app_list_folder_item.h
[modify] https://crrev.com/f8d8f00c3de844782ae4c9da1db8a3e0a19bac7a/ash/app_list/model/app_list_model.cc
[modify] https://crrev.com/f8d8f00c3de844782ae4c9da1db8a3e0a19bac7a/ash/app_list/model/app_list_model_unittest.cc
[modify] https://crrev.com/f8d8f00c3de844782ae4c9da1db8a3e0a19bac7a/ash/public/interfaces/app_list.mojom
[modify] https://crrev.com/f8d8f00c3de844782ae4c9da1db8a3e0a19bac7a/chrome/browser/ui/app_list/app_list_model_updater.h
[modify] https://crrev.com/f8d8f00c3de844782ae4c9da1db8a3e0a19bac7a/chrome/browser/ui/app_list/app_list_syncable_service.cc
[modify] https://crrev.com/f8d8f00c3de844782ae4c9da1db8a3e0a19bac7a/chrome/browser/ui/app_list/chrome_app_list_model_updater.cc
[modify] https://crrev.com/f8d8f00c3de844782ae4c9da1db8a3e0a19bac7a/chrome/browser/ui/app_list/chrome_app_list_model_updater.h
[modify] https://crrev.com/f8d8f00c3de844782ae4c9da1db8a3e0a19bac7a/chrome/browser/ui/app_list/test/fake_app_list_model_updater.cc
[modify] https://crrev.com/f8d8f00c3de844782ae4c9da1db8a3e0a19bac7a/chrome/browser/ui/app_list/test/fake_app_list_model_updater.h
[modify] https://crrev.com/f8d8f00c3de844782ae4c9da1db8a3e0a19bac7a/ui/app_list/test/app_list_test_model.cc
[modify] https://crrev.com/f8d8f00c3de844782ae4c9da1db8a3e0a19bac7a/ui/app_list/test/app_list_test_model.h
[modify] https://crrev.com/f8d8f00c3de844782ae4c9da1db8a3e0a19bac7a/ui/app_list/views/folder_header_view_unittest.cc

Comment 17 by khmel@chromium.org, May 22 2018

Status: Fixed (was: Started)

Sign in to add a comment