Closing a guest session clears all browsing history.

Closing a guest session clears all browsing history.

Note: This CL will be merged back to M41. Further work is underway
to completely delete the Guest Profile when all browser windows are
closed, but that will be too large to merge.

BUG=445036
TEST=Open a Guest profile from the User Manager. Perform some browsing, set some cookies, etc. (e.g. sign in to your email). Close all guest browsers (via Exit Guest, closing all the browsers, however you like). Navigate to the same page, and your cookies are disappeared!

Committed: https://crrev.com/67fa17a0ca78f16c8a28421d31093b26505f65c3
Cr-Commit-Position: refs/heads/master@{#316300}

[Mike Lerman, 2015-02-10 15:54:44]

mlerman@chromium.org changed reviewers:
+ battre@chromium.org, pkasting@chromium.org

[Mike Lerman, 2015-02-10 15:54:44]

Hi,

Please review this CL.

Dominic - privacy verification of clearing the browser history this way.
Peter - Owner for c/b/ui.

Thanks,
Mike

[Peter Kasting, 2015-02-10 21:43:37]

https://codereview.chromium.org/888843005/diff/20001/chrome/browser/browsing_...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/888843005/diff/20001/chrome/browser/browsing_...
chrome/browser/browsing_data/browsing_data_remover.cc:651: if
(EphemeralAppService::Get(profile_))
Get() is named as if it's expensive, implying we should use a temporary here
(or, if this isn't expensive, consider renaming to get()).

https://codereview.chromium.org/888843005/diff/20001/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/888843005/diff/20001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:516: if (profile_->IsGuestSession()) {
How come the incognito codepath doesn't actually work right?  The bug sounds
like this causes us to delete more state than before, but from the way the code
here reads I'd expect it to delete less state -- the incognito path destroys the
entire profile while this doesn't (and it even has a TODO to do so).

It seems like maybe there should be more comments here about why we can't do
what the incognito path does.

https://codereview.chromium.org/888843005/diff/20001/chrome/test/base/testing...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/888843005/diff/20001/chrome/test/base/testing...
chrome/test/base/testing_profile.cc:850: return GetRequestContext();
Does this change really belong in this CL?

[Mike Lerman, 2015-02-10 21:45:06]

mlerman@chromium.org changed reviewers:
+ bauerb@chromium.org, jcivelli@chromium.org

[Mike Lerman, 2015-02-10 21:45:06]

Adding some additional reviewers.

jcivelli - c/test
Bernhard - c/b/browsing_data

Thanks!

[Mike Lerman, 2015-02-10 21:53:32]

Thanks Peter. Responses below.

https://codereview.chromium.org/888843005/diff/20001/chrome/browser/browsing_...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/888843005/diff/20001/chrome/browser/browsing_...
chrome/browser/browsing_data/browsing_data_remover.cc:651: if
(EphemeralAppService::Get(profile_))
On 2015/02/10 21:43:36, Peter Kasting wrote:
> Get() is named as if it's expensive, implying we should use a temporary here
> (or, if this isn't expensive, consider renaming to get()).

It's a BrowserContextKeyedService, which follows the pattern of having a static
Get(Profile*) method. (It's an expensive transaction to access a BCKS that
exists, nor to check if one exists).

https://codereview.chromium.org/888843005/diff/20001/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/888843005/diff/20001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:516: if (profile_->IsGuestSession()) {
On 2015/02/10 21:43:36, Peter Kasting wrote:
> How come the incognito codepath doesn't actually work right?  The bug sounds
> like this causes us to delete more state than before, but from the way the
code
> here reads I'd expect it to delete less state -- the incognito path destroys
the
> entire profile while this doesn't (and it even has a TODO to do so).
> 
> It seems like maybe there should be more comments here about why we can't do
> what the incognito path does. 

I've modified the comment to explain why we're doing this. In short, the guest
profile has more setup than the incognito profile (such as
BrowserContextKeyedServices), and deleting it is hard, and won't get merged into
M41.

Ideally, we want to delete as much state as possible, but that requires profile
deletion, so for now we're going to clear browser data.

https://codereview.chromium.org/888843005/diff/20001/chrome/test/base/testing...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/888843005/diff/20001/chrome/test/base/testing...
chrome/test/base/testing_profile.cc:850: return GetRequestContext();
On 2015/02/10 21:43:36, Peter Kasting wrote:
> Does this change really belong in this CL?

If I don't add it here then the
BrowserCommandControllerTest.NewAvatarMenuEnabledInGuestMode test will fail, as
it exercises the new code path, and the BrowsingDataRemover runs on a
TestingProfile, and will fail without this URLRequestContextGetter. So Yes :)

[Peter Kasting, 2015-02-10 21:58:10]

https://codereview.chromium.org/888843005/diff/20001/chrome/browser/browsing_...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/888843005/diff/20001/chrome/browser/browsing_...
chrome/browser/browsing_data/browsing_data_remover.cc:651: if
(EphemeralAppService::Get(profile_))
On 2015/02/10 21:53:32, Mike Lerman wrote:
> On 2015/02/10 21:43:36, Peter Kasting wrote:
> > Get() is named as if it's expensive, implying we should use a temporary here
> > (or, if this isn't expensive, consider renaming to get()).
> 
> It's a BrowserContextKeyedService, which follows the pattern of having a
static
> Get(Profile*) method. (It's an expensive transaction to access a BCKS that
> exists, nor to check if one exists).

Did you mean to say, it's NOT an expensive transaction?

I might just use a temp here anyway to avoid giving the reader any pause.

https://codereview.chromium.org/888843005/diff/20001/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/888843005/diff/20001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:516: if (profile_->IsGuestSession()) {
On 2015/02/10 21:53:32, Mike Lerman wrote:
> On 2015/02/10 21:43:36, Peter Kasting wrote:
> > How come the incognito codepath doesn't actually work right?  The bug sounds
> > like this causes us to delete more state than before, but from the way the
> code
> > here reads I'd expect it to delete less state -- the incognito path destroys
> the
> > entire profile while this doesn't (and it even has a TODO to do so).
> > 
> > It seems like maybe there should be more comments here about why we can't do
> > what the incognito path does. 
> 
> I've modified the comment to explain why we're doing this. In short, the guest
> profile has more setup than the incognito profile (such as
> BrowserContextKeyedServices), and deleting it is hard, and won't get merged
into
> M41.
> 
> Ideally, we want to delete as much state as possible, but that requires
profile
> deletion, so for now we're going to clear browser data.

OK, the new comment is better, but I still don't really know what "it's harder
to just delete it" means.  Does that mean "DestroyProfileWhenAppropriate()
doesn't work right, and the fix for that is too big to merge, but we're going to
do that and then remove the conditional here again"?  Or something else?

[Mike Lerman, 2015-02-11 15:15:32]

https://codereview.chromium.org/888843005/diff/20001/chrome/browser/browsing_...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/888843005/diff/20001/chrome/browser/browsing_...
chrome/browser/browsing_data/browsing_data_remover.cc:651: if
(EphemeralAppService::Get(profile_))
On 2015/02/10 21:58:10, Peter Kasting wrote:
> On 2015/02/10 21:53:32, Mike Lerman wrote:
> > On 2015/02/10 21:43:36, Peter Kasting wrote:
> > > Get() is named as if it's expensive, implying we should use a temporary
here
> > > (or, if this isn't expensive, consider renaming to get()).
> > 
> > It's a BrowserContextKeyedService, which follows the pattern of having a
> static
> > Get(Profile*) method. (It's an expensive transaction to access a BCKS that
> > exists, nor to check if one exists).
> 
> Did you mean to say, it's NOT an expensive transaction?
> 
> I might just use a temp here anyway to avoid giving the reader any pause.

I did mean to say in-expensive. Serves me right for dashing out responses
quickly before I head out the door.
Ok, temp created.

https://codereview.chromium.org/888843005/diff/20001/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/888843005/diff/20001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:516: if (profile_->IsGuestSession()) {
On 2015/02/10 21:58:10, Peter Kasting wrote:
> On 2015/02/10 21:53:32, Mike Lerman wrote:
> > On 2015/02/10 21:43:36, Peter Kasting wrote:
> > > How come the incognito codepath doesn't actually work right?  The bug
sounds
> > > like this causes us to delete more state than before, but from the way the
> > code
> > > here reads I'd expect it to delete less state -- the incognito path
destroys
> > the
> > > entire profile while this doesn't (and it even has a TODO to do so).
> > > 
> > > It seems like maybe there should be more comments here about why we can't
do
> > > what the incognito path does. 
> > 
> > I've modified the comment to explain why we're doing this. In short, the
guest
> > profile has more setup than the incognito profile (such as
> > BrowserContextKeyedServices), and deleting it is hard, and won't get merged
> into
> > M41.
> > 
> > Ideally, we want to delete as much state as possible, but that requires
> profile
> > deletion, so for now we're going to clear browser data.
> 
> OK, the new comment is better, but I still don't really know what "it's harder
> to just delete it" means.  Does that mean "DestroyProfileWhenAppropriate()
> doesn't work right, and the fix for that is too big to merge, but we're going
to
> do that and then remove the conditional here again"?  Or something else?

You're exactly right. DestroyProfileWhenAppropriate, and the ProfileDestroyer,
just wrap and delay around deleting the Profile*. Deleting a Profile* (except
for an offTheRecordProfile) while Chrome's running doesn't currently work, and
that fix will be too big to merge.

I'm working on that fix (https://codereview.chromium.org/853743002/), and when I
lands that I can remove the BrowsingDataRemover here and just use the
ProfileDestroyer, as does the OffTheRecord.

Changed the comment a bit more.

[battre, 2015-02-11 15:19:25]

https://codereview.chromium.org/888843005/diff/80001/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/888843005/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:524: BrowsingDataRemover::REMOVE_DOWNLOADS,
Looking at
https://developer.chrome.com/extensions/browsingData#method-removeDownloads (I
did not check the code), REMOVE_DOWNLOADS seems to clear the list of downloaded
files but not to delete the files. Maybe REMOVE_ALL is sufficient here.

[Bernhard Bauer, 2015-02-11 16:04:28]

c/b/browsing_data/ LGTM.

https://codereview.chromium.org/888843005/diff/80001/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/888843005/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:524: BrowsingDataRemover::REMOVE_DOWNLOADS,
On 2015/02/11 15:19:25, battre wrote:
> Looking at
> https://developer.chrome.com/extensions/browsingData#method-removeDownloads (I
> did not check the code), REMOVE_DOWNLOADS seems to clear the list of
downloaded
> files but not to delete the files. Maybe REMOVE_ALL is sufficient here.

Yup, we never delete downloaded files.

[Peter Kasting, 2015-02-11 19:28:55]

Cool, thanks for the explanations and the comment improvement.  LGTM.

[Jay Civelli, 2015-02-12 00:41:38]

LGTM for chrome/test

[Mike Lerman, 2015-02-12 15:33:53]

New patchsets have been uploaded after l-g-t-m from
bauerb@chromium.org,pkasting@chromium.org,jcivelli@chromium.org

[Mike Lerman, 2015-02-12 15:36:24]

Hi Dominic,

Addressed your issue.

Still working on making the unit tests all happy. I had to move this to a
browser test, as the unit tests don't set up enough Chrome framework and I was
getting many errors (with NaCl, threads, and many more).

https://codereview.chromium.org/888843005/diff/80001/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/888843005/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:524: BrowsingDataRemover::REMOVE_DOWNLOADS,
On 2015/02/11 16:04:28, Bernhard Bauer wrote:
> On 2015/02/11 15:19:25, battre wrote:
> > Looking at
> > https://developer.chrome.com/extensions/browsingData#method-removeDownloads
(I
> > did not check the code), REMOVE_DOWNLOADS seems to clear the list of
> downloaded
> > files but not to delete the files. Maybe REMOVE_ALL is sufficient here.
> 
> Yup, we never delete downloaded files.

Okay. Done.

[Mike Lerman, 2015-02-12 22:14:26]

Hi Dominic, any further thoughts?

Peter,

Can you take a look at the browser test? I've made sufficiently significant
changes there to warrant a follow-up review.

Thanks,
Mike

[Peter Kasting, 2015-02-12 22:18:40]

LGTM

https://codereview.chromium.org/888843005/diff/140001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_command_controller_browsertest.cc (right):

https://codereview.chromium.org/888843005/diff/140001/chrome/browser/ui/brows...
chrome/browser/ui/browser_command_controller_browsertest.cc:57: // This test
also tests the BrowsingDataRemover that fires when all of a guest's
What about that BrowsingDataRemover does this test, since there's no visible
such test code?

https://codereview.chromium.org/888843005/diff/140001/chrome/browser/ui/brows...
chrome/browser/ui/browser_command_controller_browsertest.cc:71:
chrome::NOTIFICATION_BROWSER_WINDOW_READY,
Nit: Indent 4, not 6

https://codereview.chromium.org/888843005/diff/140001/chrome/browser/ui/brows...
chrome/browser/ui/browser_command_controller_browsertest.cc:94:
browser->command_controller();
Nit: Just inline this into the next line, pulling out into its own temp just
adds verbosity rather than clarity

[battre, 2015-02-13 08:49:43]

The existing code LGTM. I agree with Peter that it would be nice to have some
test coverage that closing the browsing session actually deletes the browsing
data.

https://codereview.chromium.org/888843005/diff/140001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_command_controller_browsertest.cc (right):

https://codereview.chromium.org/888843005/diff/140001/chrome/browser/ui/brows...
chrome/browser/ui/browser_command_controller_browsertest.cc:69:
scoped_ptr<content::WindowedNotificationObserver> browser_creation_observer;
opt/nit: you could also create this on the stack.

[Mike Lerman, 2015-02-13 14:05:54]

New patchsets have been uploaded after l-g-t-m from
pkasting@chromium.org,battre@chromium.org

[Mike Lerman, 2015-02-13 14:07:29]

Comments addressed - thanks everyone!

battre@, I have another CL where I'm working on building out robust tests for
guest mode, including launching, closing, and some of the guarantees we're
supposed to have.

It's started here: https://codereview.chromium.org/871893003/
for this bug:
crbug.com/360118

https://codereview.chromium.org/888843005/diff/140001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_command_controller_browsertest.cc (right):

https://codereview.chromium.org/888843005/diff/140001/chrome/browser/ui/brows...
chrome/browser/ui/browser_command_controller_browsertest.cc:57: // This test
also tests the BrowsingDataRemover that fires when all of a guest's
On 2015/02/12 22:18:40, Peter Kasting wrote:
> What about that BrowsingDataRemover does this test, since there's no visible
> such test code?

I tried rephrasing my comment here. I'm referencing the change introduced in
this CL within browser.cc, where ~Browser() can create and execute a
BrowsingDataRemover.

https://codereview.chromium.org/888843005/diff/140001/chrome/browser/ui/brows...
chrome/browser/ui/browser_command_controller_browsertest.cc:69:
scoped_ptr<content::WindowedNotificationObserver> browser_creation_observer;
On 2015/02/13 08:49:43, battre wrote:
> opt/nit: you could also create this on the stack.

Done.

https://codereview.chromium.org/888843005/diff/140001/chrome/browser/ui/brows...
chrome/browser/ui/browser_command_controller_browsertest.cc:71:
chrome::NOTIFICATION_BROWSER_WINDOW_READY,
On 2015/02/12 22:18:40, Peter Kasting wrote:
> Nit: Indent 4, not 6

Done.

https://codereview.chromium.org/888843005/diff/140001/chrome/browser/ui/brows...
chrome/browser/ui/browser_command_controller_browsertest.cc:94:
browser->command_controller();
On 2015/02/12 22:18:40, Peter Kasting wrote:
> Nit: Just inline this into the next line, pulling out into its own temp just
> adds verbosity rather than clarity

Done.

[Mike Lerman, 2015-02-13 14:07:37]

The CQ bit was checked by mlerman@chromium.org

[Mike Lerman, 2015-02-13 14:08:06]

The CQ bit was unchecked by mlerman@chromium.org

[battre, 2015-02-13 15:27:59]

Still LGTM. Thanks.

[Mike Lerman, 2015-02-13 21:17:02]

New patchsets have been uploaded after l-g-t-m from battre@chromium.org

[Mike Lerman, 2015-02-13 21:17:39]

Patchset #12 (id:220001) has been deleted

[Mike Lerman, 2015-02-13 21:17:46]

Patchset #11 (id:200001) has been deleted

[Mike Lerman, 2015-02-13 21:49:20]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2015-02-13 21:50:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/888843005/240001

[commit-bot: I haz the power, 2015-02-13 21:59:37]

Committed patchset #11 (id:240001)

[commit-bot: I haz the power, 2015-02-13 22:00:06]

Patchset 11 (id:??) landed as
https://crrev.com/67fa17a0ca78f16c8a28421d31093b26505f65c3
Cr-Commit-Position: refs/heads/master@{#316300}