Clear session-only data when all browser windows are closed.

Clear session-only data when all browser windows are closed.

This clears session-only cookies and cookies and local storage for
origins that are set to clear on exit. With this change, closing all
browser windows clears session data regardless of whether an app is
keeping the browser process alive.

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

[benwells, 2013-10-09 00:38:06]

https://codereview.chromium.org/25414005/diff/120001/chrome/browser/sessions/...
File chrome/browser/sessions/session_service.cc (right):

https://codereview.chromium.org/25414005/diff/120001/chrome/browser/sessions/...
chrome/browser/sessions/session_service.cc:342: if
(!has_open_trackable_browsers_)
How does this interact with background mode? Background mode should keep the
session data alive.

[Sam McNally, 2013-10-14 01:44:23]

https://codereview.chromium.org/25414005/diff/120001/chrome/browser/sessions/...
File chrome/browser/sessions/session_service.cc (right):

https://codereview.chromium.org/25414005/diff/120001/chrome/browser/sessions/...
chrome/browser/sessions/session_service.cc:342: if
(!has_open_trackable_browsers_)
On 2013/10/09 00:38:06, benwells wrote:
> How does this interact with background mode? Background mode should keep the
> session data alive.

Done.

[benwells, 2013-10-15 08:28:51]

Just nits and questions...

https://codereview.chromium.org/25414005/diff/216001/chrome/browser/sessions/...
File chrome/browser/sessions/better_session_restore_browsertest.cc (right):

https://codereview.chromium.org/25414005/diff/216001/chrome/browser/sessions/...
chrome/browser/sessions/better_session_restore_browsertest.cc:322:
g_browser_process->AddRefModule();
Is there a reason to use addrefmodule instead of keep alive?

https://codereview.chromium.org/25414005/diff/216001/chrome/browser/sessions/...
chrome/browser/sessions/better_session_restore_browsertest.cc:691:
StoreDataWithPage(new_browser, "session_cookies.html");
Should this be a check something or other?

https://codereview.chromium.org/25414005/diff/216001/chrome/browser/sessions/...
chrome/browser/sessions/better_session_restore_browsertest.cc:714:
StoreDataWithPage(new_browser, "cookies.html");
Same here.

https://codereview.chromium.org/25414005/diff/216001/chrome/browser/sessions/...
File chrome/browser/sessions/session_data_deleter.cc (right):

https://codereview.chromium.org/25414005/diff/216001/chrome/browser/sessions/...
chrome/browser/sessions/session_data_deleter.cc:114:
cookie_monster->GetCookieMonster()->GetAllCookiesAsync(
Nit: You don't need this GetCookieMonster, I think.

https://codereview.chromium.org/25414005/diff/216001/chrome/browser/sessions/...
File chrome/browser/sessions/session_service_factory.cc (right):

https://codereview.chromium.org/25414005/diff/216001/chrome/browser/sessions/...
chrome/browser/sessions/session_service_factory.cc:37:
DeleteSessionOnlyData(profile);
Why is this needed here?

[marja, 2013-10-15 10:07:45]

Hmm, doens't this break the "Continue where I left off" feature, where we keep
session-only data on purpose...?

[marja, 2013-10-15 10:08:21]

On 2013/10/15 10:07:45, marja wrote:
> Hmm, doens't this break the "Continue where I left off" feature, where we keep
> session-only data on purpose...?

Hmm2, or is the decision to not delete in that case made on some lower layer?

[Sam McNally, 2013-10-15 22:53:05]

On 2013/10/15 10:08:21, marja wrote:
> On 2013/10/15 10:07:45, marja wrote:
> > Hmm, doens't this break the "Continue where I left off" feature, where we
keep
> > session-only data on purpose...?
> 
> Hmm2, or is the decision to not delete in that case made on some lower layer?

That setting is passed in and used in
SessionDataDeleter::DeleteSessionCookiesOnIOThread.

https://codereview.chromium.org/25414005/diff/216001/chrome/browser/sessions/...
File chrome/browser/sessions/better_session_restore_browsertest.cc (right):

https://codereview.chromium.org/25414005/diff/216001/chrome/browser/sessions/...
chrome/browser/sessions/better_session_restore_browsertest.cc:322:
g_browser_process->AddRefModule();
On 2013/10/15 08:28:51, benwells wrote:
> Is there a reason to use addrefmodule instead of keep alive?

Done.

https://codereview.chromium.org/25414005/diff/216001/chrome/browser/sessions/...
chrome/browser/sessions/better_session_restore_browsertest.cc:691:
StoreDataWithPage(new_browser, "session_cookies.html");
On 2013/10/15 08:28:51, benwells wrote:
> Should this be a check something or other?

StoreDataWithPage checks that the value was not set before setting it.

https://codereview.chromium.org/25414005/diff/216001/chrome/browser/sessions/...
File chrome/browser/sessions/session_data_deleter.cc (right):

https://codereview.chromium.org/25414005/diff/216001/chrome/browser/sessions/...
chrome/browser/sessions/session_data_deleter.cc:114:
cookie_monster->GetCookieMonster()->GetAllCookiesAsync(
On 2013/10/15 08:28:51, benwells wrote:
> Nit: You don't need this GetCookieMonster, I think.

Done.

https://codereview.chromium.org/25414005/diff/216001/chrome/browser/sessions/...
File chrome/browser/sessions/session_service_factory.cc (right):

https://codereview.chromium.org/25414005/diff/216001/chrome/browser/sessions/...
chrome/browser/sessions/session_service_factory.cc:37:
DeleteSessionOnlyData(profile);
On 2013/10/15 08:28:51, benwells wrote:
> Why is this needed here?

This is hit when doing a wrench menu quit. In that case the session service will
have been shut down before the browser windows close so the other case where
DeleteSessionOnlyData is called won't get hit.

In practice this won't do anything until wrench menu quit only closes browsers
so this could go in the cl that does that instead.

[Sam McNally, 2013-10-15 22:57:52]

+atwilson for chrome/browser/background

[benwells, 2013-10-15 22:58:40]

lgtm

https://codereview.chromium.org/25414005/diff/216001/chrome/browser/sessions/...
File chrome/browser/sessions/session_service_factory.cc (right):

https://codereview.chromium.org/25414005/diff/216001/chrome/browser/sessions/...
chrome/browser/sessions/session_service_factory.cc:37:
DeleteSessionOnlyData(profile);
On 2013/10/15 22:53:05, Sam McNally wrote:
> On 2013/10/15 08:28:51, benwells wrote:
> > Why is this needed here?
> 
> This is hit when doing a wrench menu quit. In that case the session service
will
> have been shut down before the browser windows close so the other case where
> DeleteSessionOnlyData is called won't get hit.
> 
> In practice this won't do anything until wrench menu quit only closes browsers
> so this could go in the cl that does that instead.

OK, that's what my theory was but wanted to get verification. Seems reasonable
to leave it in.

[Andrew T Wilson (Slow), 2013-10-16 07:33:57]

browser/background lgtm.

[marja, 2013-10-16 08:01:28]

(K. Removing myself from reviewer list, my drive-by commenting got me added...)

[Sam McNally, 2013-10-16 08:26:50]

On 2013/10/16 08:01:28, marja wrote:
> (K. Removing myself from reviewer list, my drive-by commenting got me
added...)

I was actually going to add you after Ben was done for chrome/browser/sessions.
PTAL.

[marja, 2013-10-16 14:18:08]

Alright :) It has been a while since I've worked on this code the last time, pls
bear with me.

https://codereview.chromium.org/25414005/diff/235001/chrome/browser/sessions/...
File chrome/browser/sessions/session_data_deleter.cc (right):

https://codereview.chromium.org/25414005/diff/235001/chrome/browser/sessions/...
chrome/browser/sessions/session_data_deleter.cc:32: bool
delete_all_session_cookies);
Naming of this variable is a bit unclear to me. From the name, tt's not clear
what the other option is if we're not deleting all session cookies. Reading the
code, it looks like this

1) continue where I left off -> delete_all = false -> delete the cookies where
the policy says the origin is session only, ignore if the cookies in question
are "session only" in the HTTP sense or not.
2) not continue where I left off -> delete_all = true -> delete the cookies
which are "session cookies" (in the HTTP sense).

I think the naming would be clearer if it had "policy" in it...
delete_only_by_session_only_policy? That's not a great name either, maybe you
have a better suggestion :)

https://codereview.chromium.org/25414005/diff/235001/chrome/browser/sessions/...
chrome/browser/sessions/session_data_deleter.cc:111:
base::Bind(&SessionDataDeleter::DeleteSessionCookiesDone, this));
Inside the if branch: Shouldn't we also delete "normal" (not session only in the
HTTP sense) cookies for origins which are session only by policy?

https://codereview.chromium.org/25414005/diff/235001/chrome/browser/sessions/...
File chrome/browser/sessions/session_service.cc (right):

https://codereview.chromium.org/25414005/diff/235001/chrome/browser/sessions/...
chrome/browser/sessions/session_service.cc:344: #if !defined(OS_MACOSX) &&
!defined(OS_CHROMEOS)
Why not for mac and chromeos? Because of some "how sessions should look like"
differences? Because for mac and chromeos, chrome keeps running even though
windows are closed? (Maybe add a comment; this looks familiar but I cannot
exactly remember what this was about.)

[Sam McNally, 2013-10-16 23:30:59]

https://codereview.chromium.org/25414005/diff/235001/chrome/browser/sessions/...
File chrome/browser/sessions/session_data_deleter.cc (right):

https://codereview.chromium.org/25414005/diff/235001/chrome/browser/sessions/...
chrome/browser/sessions/session_data_deleter.cc:32: bool
delete_all_session_cookies);
On 2013/10/16 14:18:08, marja wrote:
> Naming of this variable is a bit unclear to me. From the name, tt's not clear
> what the other option is if we're not deleting all session cookies. Reading
the
> code, it looks like this
> 
> 1) continue where I left off -> delete_all = false -> delete the cookies where
> the policy says the origin is session only, ignore if the cookies in question
> are "session only" in the HTTP sense or not.
> 2) not continue where I left off -> delete_all = true -> delete the cookies
> which are "session cookies" (in the HTTP sense).
> 
> I think the naming would be clearer if it had "policy" in it...
> delete_only_by_session_only_policy? That's not a great name either, maybe you
> have a better suggestion :)

Done.

https://codereview.chromium.org/25414005/diff/235001/chrome/browser/sessions/...
chrome/browser/sessions/session_data_deleter.cc:111:
base::Bind(&SessionDataDeleter::DeleteSessionCookiesDone, this));
On 2013/10/16 14:18:08, marja wrote:
> Inside the if branch: Shouldn't we also delete "normal" (not session only in
the
> HTTP sense) cookies for origins which are session only by policy?

That's done later in DeleteSessionCookiesDone.

https://codereview.chromium.org/25414005/diff/235001/chrome/browser/sessions/...
File chrome/browser/sessions/session_service.cc (right):

https://codereview.chromium.org/25414005/diff/235001/chrome/browser/sessions/...
chrome/browser/sessions/session_service.cc:344: #if !defined(OS_MACOSX) &&
!defined(OS_CHROMEOS)
On 2013/10/16 14:18:08, marja wrote:
> Why not for mac and chromeos? Because of some "how sessions should look like"
> differences? Because for mac and chromeos, chrome keeps running even though
> windows are closed? (Maybe add a comment; this looks familiar but I cannot
> exactly remember what this was about.)

Correct. Switched to kBrowserAliveWithNoWindows and added a comment.

[marja, 2013-10-17 08:09:17]

lgtm

[commit-bot: I haz the power, 2013-10-17 09:00:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sammc@chromium.org/25414005/142008

[commit-bot: I haz the power, 2013-10-17 12:40:46]

Retried try job too often on mac_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-10-17 23:17:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sammc@chromium.org/25414005/142008

[commit-bot: I haz the power, 2013-10-18 01:20:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sammc@chromium.org/25414005/142008

[commit-bot: I haz the power, 2013-10-18 02:21:03]

Change committed as 229281