Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(3)

Issue 592983002: Fix Mac App Launcher observer lifetime on AppListItemList (Closed)

Created:
6 years, 3 months ago by tapted
Modified:
6 years, 3 months ago
Reviewers:
jackhou1
CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, tfarina
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Fix Mac App Launcher observer lifetime on AppListItemList r295646 moved ownership of the AppListViewDelegate out of the AppListViewController which meant that the Mac AppList began removing itself as an observer from the wrong model. This happens because, in Chrome, the AppListViewDelegate changes the return from GetModel() when the profile is switched, but doesn't currently have a way of notifying the AppList that this is happening. To fix, AppListServiceMac needs to explicitly clear the delegate on the Chrome side before asking the view delegate to switch out its model (which happens via AppListServiceImpl::GetViewDelegate(Profile*). Views platforms are not affected, because they retain a raw pointer to the AppListModel* separately from AppListViewDelegate::GetModel(). This only works because the AppListModel* sticks around until Chrome shutdown, when it's torn down with the other keyed services via the ~ProfileManager() destructor. This delayed destruction means that verifying observers in a test needs a lot of plumbing in non-test code (see patchset1). Instead, a DCHECK is added in AppListItemList to ensure RemoveObserver always removes something, then test coverage is given by AppListServiceImplBrowserTest.ShowLoadedProfiles. This CL fixes part of the flakiness in the app list interactive uitests on Mac (http://crbug.com/415264), but I've discovered other sources of flakiness to fix before re-enabling those tests. It should, however, fix the recent crashes (http://crbug.com/416345). BUG=415264, 416345 TEST=AppListServiceImplBrowserTest.ShowLoadedProfiles TEST=crbug/416345: have multiple profiles and switch between them in the app launcher on mac many times. Chrome shouldn't crash. Committed: https://crrev.com/14123e73e132d1cf6fa0d70d65aa703246778df2 Cr-Commit-Position: refs/heads/master@{#296152}

Patch Set 1 : Epic crazytest #

Patch Set 2 : More sensible #

Patch Set 3 : leave disabled due to other flakiness #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2 lines, -0 lines) Patch
M chrome/browser/ui/app_list/app_list_service_mac.mm View 1 chunk +1 line, -0 lines 0 comments Download
M ui/app_list/app_list_item_list.cc View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 7 (2 generated)
tapted
6 years, 3 months ago (2014-09-23 05:54:48 UTC) #2
jackhou1
lgtm
6 years, 3 months ago (2014-09-23 07:08:42 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/592983002/40001
6 years, 3 months ago (2014-09-23 07:10:09 UTC) #5
commit-bot: I haz the power
Committed patchset #3 (id:40001) as 1ab3ed5cd84d3e6bacc912655d5a943bc7540c6a
6 years, 3 months ago (2014-09-23 07:14:55 UTC) #6
commit-bot: I haz the power
6 years, 3 months ago (2014-09-23 07:15:35 UTC) #7
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/14123e73e132d1cf6fa0d70d65aa703246778df2
Cr-Commit-Position: refs/heads/master@{#296152}

Powered by Google App Engine
This is Rietveld 408576698