Fix Profile* lifetime issues in Chrome's AppListViewDelegate

Fix Profile* lifetime issues in Chrome's AppListViewDelegate

Currently AppListViewDelegate can hold on to references to a destroyed
Profile*. It's managed to escape crashing in most cases so far because
the LocalState pref is updated for the next time the app launcher is
shown. However, if the profile the app launcher is first created for is
ever deleted in the same session, then a crash usually follows (but
doesn't always create a crash dump, due to a corrupt stack).

This decouples the Profile from the AppListViewDelegate constructor to
make it clear the lifetimes are not in step. Then "SetProfile" correctly
tears down any references to an old profile, before setting a new one.

When the app list's active profile is deleted, the AppListViewDelegate
is destroyed by forcibly closing/destroying the AppList's widget via a
new method AppListServiceImpl::DestroyAppList().

BUG=392763, 403647, 373689, 405827
TEST=(windows) Show the app list, right-click an app and "uninstall".
Leave the uninstall dialog open. Switch to a chrome://settings in a
browser and delete the profile being shown in the app list. App list
should close.

Committed: https://crrev.com/adcde472fce55457f2dff391d96e2ae0992d0362
Cr-Commit-Position: refs/heads/master@{#291852}

[tapted, 2014-08-22 04:26:53]

I think this is ready for a Matt - PTAL

[Matt Giuca, 2014-08-25 04:48:46]

OK, I've read over it and I think I've made all the comments I want to make.

But this CL is pretty hard to wrap your head around as it seems to be doing
several (related) changes. If you can think of a way to split it up that would
be appreciated: in particular if you could split up some of the interface
changes (like the changes to AppListViewDelegate) from the behavioural changes.

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_impl.cc (right):

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_impl.cc:296: // Before
ProfileInfoCache::DeleteProfileFromCache() calls this function,
// Switch the app list over to a valid profile. <followed by the existing text>.

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_impl.cc:303: // Now, the Chrome
AppListViewDelegate needs to be torn down, since it has
It might be simpler to lay this out as a bullet list:

// The AppListViewDelegate needs to be torn down, because:
1. many references
2. might not be loaded yet.
// Currently, it might be owned by ....

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_impl.cc:305: // the last used
profile might not be loaded yet, since that's sometimes done
nit: "sometimes done *by*"

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc (right):

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc:286:
EXPECT_FALSE(service->IsAppListVisible());
Q: Does this happen on the *scheduling* of the profile deletion? (As opposed to
asynchronously?)

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_mac.mm (right):

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_mac.mm:399:
setDelegate:delegate.PassAs<app_list::AppListViewDelegate>()];
Can you explain this line? (To me: not necessarily in a comment.)

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:199: // destroyed first.
Should you duplicate this comment where |search_controller_| is declared in the
header file?

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.h:57: explicit
AppListViewDelegate(AppListControllerDelegate* controller_delegate);
I don't see why you had to delete the Profile argument. Since every single
instantiation of ALVD is now followed by a call to SetProfile, it seems better
to just leave this as-is and call SetProfile at the end of the constructor.

This would let you revert a lot of the changes throughout this CL which appear
to just be adding a call to SetProfile (I've noticed you did a lot of "misc"
cleanup nearby those changes, which is good but probably best to leave out of
this already confusing CL).

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/app_list/app_list_service_ash.cc (right):

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_service_ash.cc:67: // On Ash, the app
list is torn down whenever it is dismissed. So, just ensure
Nit: ". So, " -> ", so "

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_service_ash.cc:68: // that is the case.
Nit: "that it is dismissed."

[tapted, 2014-08-25 06:17:01]

Thanks matt! Comments addressed. I also put a contrived diff in patchset 9 that
just reorders functions (versus the final patchset). It makes the
AppListViewDelegate .cc diff less spooky, but the function order is atrocious:
https://codereview.chromium.org/492163002/diff/160001/chrome/browser/ui/app_l...

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_impl.cc (right):

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_impl.cc:296: // Before
ProfileInfoCache::DeleteProfileFromCache() calls this function,
On 2014/08/25 04:48:46, Matt Giuca wrote:
> // Switch the app list over to a valid profile. <followed by the existing
text>.

Done.

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_impl.cc:303: // Now, the Chrome
AppListViewDelegate needs to be torn down, since it has
On 2014/08/25 04:48:46, Matt Giuca wrote:
> It might be simpler to lay this out as a bullet list:
> 
> // The AppListViewDelegate needs to be torn down, because:
> 1. many references
> 2. might not be loaded yet.
> // Currently, it might be owned by ....

Done.

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_impl.cc:305: // the last used
profile might not be loaded yet, since that's sometimes done
On 2014/08/25 04:48:46, Matt Giuca wrote:
> nit: "sometimes done *by*"

Done.

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc (right):

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc:286:
EXPECT_FALSE(service->IsAppListVisible());
On 2014/08/25 04:48:46, Matt Giuca wrote:
> Q: Does this happen on the *scheduling* of the profile deletion? (As opposed
to
> asynchronously?)

Yep - it happens straight away. The "scheduling" refers to the disk operation
that actually deletes the files from disk and happens on the FILE thread. But
OnProfileWillBeRemoved is triggered in 
ProfileInfoCache::DeleteProfileFromCache() which is called by
FinishDeletingProfile which (confusingly) is called synchronously from
ScheduleProfileForDeletion (and, in that case, isn't referring to the disk
operation).

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_mac.mm (right):

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_mac.mm:399:
setDelegate:delegate.PassAs<app_list::AppListViewDelegate>()];
On 2014/08/25 04:48:46, Matt Giuca wrote:
> Can you explain this line? (To me: not necessarily in a comment.)

Yeah - we really should rename ::AppListViewDelegate to
::ChromeAppListViewDelegate, since it makes things like this extra confusing.

SetProfile doesn't (and shouldn't) exist on app_list::AppListViewDelegate --
it's only on ::AppListViewDelegate, hence the change on line 395.

Then the mac AppListViewController is using scoped_ptr to indicate an ownership
transferral, but scoped_ptr<B> can't implicitly upcast to scoped_ptr<A> (when B
inherits from A). So this has to be done with PassAs, which does the implicit
cast so that it can be passed to a scoped_ptr of its parent type.

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:199: // destroyed first.
On 2014/08/25 04:48:46, Matt Giuca wrote:
> Should you duplicate this comment where |search_controller_| is declared in
the
> header file?

Done.

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.h:57: explicit
AppListViewDelegate(AppListControllerDelegate* controller_delegate);
On 2014/08/25 04:48:46, Matt Giuca wrote:
> I don't see why you had to delete the Profile argument. Since every single
> instantiation of ALVD is now followed by a call to SetProfile, it seems better
> to just leave this as-is and call SetProfile at the end of the constructor.
> 
> This would let you revert a lot of the changes throughout this CL which appear
> to just be adding a call to SetProfile (I've noticed you did a lot of "misc"
> cleanup nearby those changes, which is good but probably best to leave out of
> this already confusing CL).

Done.

I think splitting the constructor is still an improvement. It goes with the
http://go/cppguide#Doing_Work_in_Constructors style guide. But it can be done
orthogonally.

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/app_list/app_list_service_ash.cc (right):

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_service_ash.cc:67: // On Ash, the app
list is torn down whenever it is dismissed. So, just ensure
On 2014/08/25 04:48:46, Matt Giuca wrote:
> Nit: ". So, " -> ", so "

Done.

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_service_ash.cc:68: // that is the case.
On 2014/08/25 04:48:46, Matt Giuca wrote:
> Nit: "that it is dismissed."

Done.

[Matt Giuca, 2014-08-26 01:29:45]

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_mac.mm (right):

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_mac.mm:399:
setDelegate:delegate.PassAs<app_list::AppListViewDelegate>()];
But you reverted the use of PassAs now?

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.h:57: explicit
AppListViewDelegate(AppListControllerDelegate* controller_delegate);
Fair enough. Can you put back your comment about taking ownership, and your
TODO?

Yeah I'm happy to split out SetProfile in another CL (to satisfy "doing work in
constructors" rule).

[Matt Giuca, 2014-08-26 01:29:52]

lgtm

[tapted, 2014-08-26 02:38:20]

Patchset #12 (id:220001) has been deleted

[tapted, 2014-08-26 02:43:57]

tapted@chromium.org changed reviewers:
+ xiyuan@chromium.org

[tapted, 2014-08-26 02:43:57]

+xiyuan for OWNERS in chrome/browser/ui/ash/app_list/  - please take a look

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_mac.mm (right):

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_mac.mm:399:
setDelegate:delegate.PassAs<app_list::AppListViewDelegate>()];
On 2014/08/26 01:29:45, Matt Giuca wrote:
> But you reverted the use of PassAs now?

Yep - since the constructor is calling SetProfile, there's no need for a
::AppListViewDelegate instead of an app_list::AppListViewDelegate here

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/492163002/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.h:57: explicit
AppListViewDelegate(AppListControllerDelegate* controller_delegate);
On 2014/08/26 01:29:45, Matt Giuca wrote:
> Fair enough. Can you put back your comment about taking ownership, and your
> TODO?
> 
> Yeah I'm happy to split out SetProfile in another CL (to satisfy "doing work
in
> constructors" rule).

Done.

[xiyuan, 2014-08-26 02:58:24]

chrome/browser/ui/ash/app_list/ LGTM

[tapted, 2014-08-26 02:59:37]

wow! that was quick. Thanks all!

[tapted, 2014-08-26 02:59:53]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-08-26 03:00:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tapted@chromium.org/492163002/240001

[commit-bot: I haz the power, 2014-08-26 05:55:03]

Committed patchset #12 (240001) as 0cb217b650b73017769f50d271b25e597d7622e8

[commit-bot: I haz the power, 2014-09-10 02:40:53]

Patchset 12 (id:??) landed as
https://crrev.com/adcde472fce55457f2dff391d96e2ae0992d0362
Cr-Commit-Position: refs/heads/master@{#291852}