Fix use-after-free when switching profiles in the experimental app list.

Fix use-after-free when switching profiles in the experimental app list.

This CL fixes an issue where the ContentsSwitcherView would dereference a
pointer to a freed ContentsView.

BUG=384957
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278054

[Matt Giuca, 2014-06-17 05:49:26]

Be more direct with the CL description.

https://codereview.chromium.org/334293005/diff/20001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/334293005/diff/20001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.cc:113: DCHECK_EQ(1, child_count());
This doesn't seem robust. Is this assuming that there is always exactly one
child view of AppListMainView at the time the contents view is added?

Why is it needed?

https://codereview.chromium.org/334293005/diff/20001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.cc:171: contents_switcher_view_ = NULL;
nit: May as well put this line inside the if statement.

https://codereview.chromium.org/334293005/diff/20001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_main_view.h (right):

https://codereview.chromium.org/334293005/diff/20001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.h:80: void AddContentsView();
Might be good to comment that this also adds the contents switcher view. Maybe
even rename to AddChildViews since it adds more than just ContentsView now.

https://codereview.chromium.org/334293005/diff/20001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.h:116: ContentsSwitcherView*
contents_switcher_view_;  // Owned by views hierarchy.
Add that it can be NULL.

[calamity, 2014-06-17 09:15:39]

https://codereview.chromium.org/334293005/diff/20001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/334293005/diff/20001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.cc:113: DCHECK_EQ(1, child_count());
On 2014/06/17 05:49:26, Matt Giuca wrote:
> This doesn't seem robust. Is this assuming that there is always exactly one
> child view of AppListMainView at the time the contents view is added?
> 
> Why is it needed?

It's not. AddChildViewAt was there to fix a bug that is now resolved. Removed.

https://codereview.chromium.org/334293005/diff/20001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.cc:171: contents_switcher_view_ = NULL;
On 2014/06/17 05:49:25, Matt Giuca wrote:
> nit: May as well put this line inside the if statement.

Done.

https://codereview.chromium.org/334293005/diff/20001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_main_view.h (right):

https://codereview.chromium.org/334293005/diff/20001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.h:80: void AddContentsView();
On 2014/06/17 05:49:26, Matt Giuca wrote:
> Might be good to comment that this also adds the contents switcher view. Maybe
> even rename to AddChildViews since it adds more than just ContentsView now.

Renamed to AddContentsViews(). I don't think AddChildViews is correct since the
search box isn't added here.

https://codereview.chromium.org/334293005/diff/20001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.h:116: ContentsSwitcherView*
contents_switcher_view_;  // Owned by views hierarchy.
On 2014/06/17 05:49:26, Matt Giuca wrote:
> Add that it can be NULL.

Done.

[Matt Giuca, 2014-06-18 00:06:07]

https://codereview.chromium.org/334293005/diff/20001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/334293005/diff/20001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.cc:113: DCHECK_EQ(1, child_count());
I'm a little unsure what you mean here. Does "It's not" mean "the DCHECK is not
needed", and "Removed" refer to the DCHECK? If so, you didn't remove it.

Otherwise, I don't know what you mean.

[calamity, 2014-06-18 01:00:02]

https://codereview.chromium.org/334293005/diff/20001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/334293005/diff/20001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.cc:113: DCHECK_EQ(1, child_count());
On 2014/06/18 00:06:06, Matt Giuca wrote:
> I'm a little unsure what you mean here. Does "It's not" mean "the DCHECK is
not
> needed", and "Removed" refer to the DCHECK? If so, you didn't remove it.
> 
> Otherwise, I don't know what you mean.

Oops. Actually removed.

[Matt Giuca, 2014-06-18 01:18:42]

lgtm

[calamity, 2014-06-18 04:11:29]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-06-18 04:14:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/calamity@chromium.org/334293005/80001

[calamity, 2014-06-18 07:42:37]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-06-18 07:46:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/calamity@chromium.org/334293005/100001

[commit-bot: I haz the power, 2014-06-18 13:25:39]

Change committed as 278054