Resolve new GAIA flow's infinite loop.

Resolve new GAIA flow's infinite loop.

When a previously added (existing) user is removed from the white list,
authentication fails and reloads with the same cookies, which causes it to
retry a sign-in, thus causing an infinite loop. We must clear cookies both
from C++ code and in JS code.

BUG=464143
Committed: https://crrev.com/74a79da40a413af2d4f470603d90eb02e15fc171
Cr-Commit-Position: refs/heads/master@{#321754}

[Ivan Podogov, 2015-03-17 14:29:39]

ginkage@chromium.org changed reviewers:
+ rsorokin@chromium.org, xiyuan@chromium.org

[Ivan Podogov, 2015-03-17 14:29:49]

PTAL

[xiyuan, 2015-03-17 16:05:53]

https://codereview.chromium.org/1012083002/diff/1/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right):

https://codereview.chromium.org/1012083002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:650: if
(StartupUtils::IsWebviewSigninEnabled()) {
Can we move the webview data clearing code into
ProfileHelper::ClearSigninProfile?

Also, since we are doing this, can we also update ProfileAuthData::Transfer?
That is, changing it to take a net::URLRequestContextGetter* instead of a
BrowserContext* and pass in the webview's context when webview is used.

https://codereview.chromium.org/1012083002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:655:
contents->GetLastCommittedURL().GetContent());
This is not a reliable way. I happened to chat with Fady about this yesterday.
He suggested to do the following the get the webview storage partition:
1. GuestViewManager::ForEachGuest with embedder WebContents [1], this gives us
the guest WebContents of the webview;
2. Get SiteInstance* from the guest WebContents;
3. Get site URL using SiteInstance::GetSiteURL;

Step 3 should be the the |guest_url| we created manually here but this way would
be more proper and does not need to know the details of how the guest url is
constructed.

[1]:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[Roman Sorokin (ftl), 2015-03-17 17:00:41]

https://codereview.chromium.org/1012083002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/1012083002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:697:
login_display_->ShowSigninUI("");
Why is call here with an empty string? Could we just don't make call at all?

https://codereview.chromium.org/1012083002/diff/1/chrome/browser/resources/ga...
File chrome/browser/resources/gaia_auth_host/authenticator.js (right):

https://codereview.chromium.org/1012083002/diff/1/chrome/browser/resources/ga...
chrome/browser/resources/gaia_auth_host/authenticator.js:131:
Authenticator.prototype.clearCredentials_ = function() {
Could we call this function in the constructor? We make the same init there.

https://codereview.chromium.org/1012083002/diff/1/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right):

https://codereview.chromium.org/1012083002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:659: if
(partition) {
Handling error after DCHECK is forbidden. See here
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

I suggest
DCHECK(partition) << "Storage partition not found for " << guest_url.spec();
partition->ClearData(...

https://codereview.chromium.org/1012083002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:660: LOG(ERROR) <<
"Start clearing cookies for " << guest_url.spec();
Why is it LOG(ERROR)?

https://codereview.chromium.org/1012083002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:669: } else {
nit: don't use else after return.

[Ivan Podogov, 2015-03-18 07:36:20]

https://codereview.chromium.org/1012083002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/1012083002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:697:
login_display_->ShowSigninUI("");
On 2015/03/17 17:00:41, Roman Sorokin wrote:
> Why is call here with an empty string? Could we just don't make call at all?

This call is made after the Sign-in UI has been made inactive, to show it once
again from the beginning. Empty string here ensures that we start from the first
screen (without e-mail already provided), rather thatn from the second one.

https://codereview.chromium.org/1012083002/diff/1/chrome/browser/resources/ga...
File chrome/browser/resources/gaia_auth_host/authenticator.js (right):

https://codereview.chromium.org/1012083002/diff/1/chrome/browser/resources/ga...
chrome/browser/resources/gaia_auth_host/authenticator.js:131:
Authenticator.prototype.clearCredentials_ = function() {
On 2015/03/17 17:00:41, Roman Sorokin wrote:
> Could we call this function in the constructor? We make the same init there.

That was my initial thought, but Dmitry asked me to keep it this way, so that
all the variables that are added in the constructor would be listed together.
Also, if we will add/remove anything from this function, we'll have to keep
track on what we change, and always update the constructor accordingly, which is
not a convenient way.

https://codereview.chromium.org/1012083002/diff/1/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right):

https://codereview.chromium.org/1012083002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:650: if
(StartupUtils::IsWebviewSigninEnabled()) {
On 2015/03/17 16:05:53, xiyuan wrote:
> Can we move the webview data clearing code into
> ProfileHelper::ClearSigninProfile?
> 
> Also, since we are doing this, can we also update ProfileAuthData::Transfer?
> That is, changing it to take a net::URLRequestContextGetter* instead of a
> BrowserContext* and pass in the webview's context when webview is used.

Can we make it a separate issue? Right now we are dealing with a release
blocker, so I don't want to mix in any significant refactoring here. Also, for
ProfileHelper::ClearSigninProfile, do we have a WebContents there, to determine
a WebView? Should we move any GAIA screen's logic there at all? Sounds really
strange to me.
Here we just clear one of two storages, depending on the GAIA flow that is being
used. This seems to be a better place to do it.

https://codereview.chromium.org/1012083002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:655:
contents->GetLastCommittedURL().GetContent());
On 2015/03/17 16:05:52, xiyuan wrote:
> This is not a reliable way. I happened to chat with Fady about this yesterday.
> He suggested to do the following the get the webview storage partition:
> 1. GuestViewManager::ForEachGuest with embedder WebContents [1], this gives us
> the guest WebContents of the webview;
> 2. Get SiteInstance* from the guest WebContents;
> 3. Get site URL using SiteInstance::GetSiteURL;
> 
> Step 3 should be the the |guest_url| we created manually here but this way
would
> be more proper and does not need to know the details of how the guest url is
> constructed.
> 
> [1]:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

Potentially it will give us a list of URLs (and/or partitions), should we clear
them all one-by-one?

https://codereview.chromium.org/1012083002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:659: if
(partition) {
On 2015/03/17 17:00:41, Roman Sorokin wrote:
> Handling error after DCHECK is forbidden. See here
>
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-
> 
> I suggest
> DCHECK(partition) << "Storage partition not found for " << guest_url.spec();
> partition->ClearData(...

In fact, Dmitry asked me to do it this way, for the sake of error logging in
production code. I think I'll just remove DCHECK for now.

https://codereview.chromium.org/1012083002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:660: LOG(ERROR) <<
"Start clearing cookies for " << guest_url.spec();
On 2015/03/17 17:00:41, Roman Sorokin wrote:
> Why is it LOG(ERROR)?

Forgotten debug logging, shouldn't be here.

https://codereview.chromium.org/1012083002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:669: } else {
On 2015/03/17 17:00:41, Roman Sorokin wrote:
> nit: don't use else after return.

Done.

[Ivan Podogov, 2015-03-18 07:53:05]

https://codereview.chromium.org/1012083002/diff/1/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right):

https://codereview.chromium.org/1012083002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:655:
contents->GetLastCommittedURL().GetContent());
On 2015/03/18 07:36:20, Ivan Podogov wrote:
> On 2015/03/17 16:05:52, xiyuan wrote:
> > This is not a reliable way. I happened to chat with Fady about this
yesterday.
> > He suggested to do the following the get the webview storage partition:
> > 1. GuestViewManager::ForEachGuest with embedder WebContents [1], this gives
us
> > the guest WebContents of the webview;
> > 2. Get SiteInstance* from the guest WebContents;
> > 3. Get site URL using SiteInstance::GetSiteURL;
> > 
> > Step 3 should be the the |guest_url| we created manually here but this way
> would
> > be more proper and does not need to know the details of how the guest url is
> > constructed.
> > 
> > [1]:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> 
> Potentially it will give us a list of URLs (and/or partitions), should we
clear
> them all one-by-one?

Oh, and one more thing to keep in mind here: in this place, we don't have a
webview yet, it will be loaded later, but we already need to clear the storage,
before the URL will be loaded. That is the reason why we construct it manually
here.

[xiyuan, 2015-03-18 16:52:27]

cc: fsamuel in case he is interested in how we deal with the webview's storage
partition and has inputs.

https://codereview.chromium.org/1012083002/diff/1/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right):

https://codereview.chromium.org/1012083002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:650: if
(StartupUtils::IsWebviewSigninEnabled()) {
On 2015/03/18 07:36:20, Ivan Podogov wrote:
> On 2015/03/17 16:05:53, xiyuan wrote:
> > Can we move the webview data clearing code into
> > ProfileHelper::ClearSigninProfile?
> > 
> > Also, since we are doing this, can we also update ProfileAuthData::Transfer?
> > That is, changing it to take a net::URLRequestContextGetter* instead of a
> > BrowserContext* and pass in the webview's context when webview is used.
> 
> Can we make it a separate issue? Right now we are dealing with a release
> blocker, so I don't want to mix in any significant refactoring here. Also, for
> ProfileHelper::ClearSigninProfile, do we have a WebContents there, to
determine
> a WebView? Should we move any GAIA screen's logic there at all? Sounds really
> strange to me.
> Here we just clear one of two storages, depending on the GAIA flow that is
being
> used. This seems to be a better place to do it.

I agree introducing gaia screen details such as WebContents/Webview into
ProfileHelper is bad. However, we need a central place to properly clear the
sign-in state. ClearSigninProfile is used as a way to clear sign-in state and
used in other places with that expectation [1]. And we don't want to duplicate
the code everywhere.

I don't see the bug is tagged as a release blocker. Since we are implementing
the feature, we should try to do it properly.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

https://codereview.chromium.org/1012083002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:655:
contents->GetLastCommittedURL().GetContent());
On 2015/03/18 07:53:05, Ivan Podogov wrote:
> On 2015/03/18 07:36:20, Ivan Podogov wrote:
> > On 2015/03/17 16:05:52, xiyuan wrote:
> > > This is not a reliable way. I happened to chat with Fady about this
> yesterday.
> > > He suggested to do the following the get the webview storage partition:
> > > 1. GuestViewManager::ForEachGuest with embedder WebContents [1], this
gives
> us
> > > the guest WebContents of the webview;
> > > 2. Get SiteInstance* from the guest WebContents;
> > > 3. Get site URL using SiteInstance::GetSiteURL;
> > > 
> > > Step 3 should be the the |guest_url| we created manually here but this way
> > would
> > > be more proper and does not need to know the details of how the guest url
is
> > > constructed.
> > > 
> > > [1]:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> > 
> > Potentially it will give us a list of URLs (and/or partitions), should we
> clear
> > them all one-by-one?
> 

We can DCHECK only one guest URL is returned.

> Oh, and one more thing to keep in mind here: in this place, we don't have a
> webview yet, it will be loaded later, but we already need to clear the
storage,
> before the URL will be loaded. That is the reason why we construct it manually
> here.

If webview is not loaded and storage does not exist, there is nothing to clear
and we can just bail out on this case.

[Ivan Podogov, 2015-03-19 09:29:45]

https://codereview.chromium.org/1012083002/diff/1/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right):

https://codereview.chromium.org/1012083002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:655:
contents->GetLastCommittedURL().GetContent());
On 2015/03/18 16:52:27, xiyuan wrote:
> On 2015/03/18 07:53:05, Ivan Podogov wrote:
> > On 2015/03/18 07:36:20, Ivan Podogov wrote:
> > > On 2015/03/17 16:05:52, xiyuan wrote:
> > > > This is not a reliable way. I happened to chat with Fady about this
> > yesterday.
> > > > He suggested to do the following the get the webview storage partition:
> > > > 1. GuestViewManager::ForEachGuest with embedder WebContents [1], this
> gives
> > us
> > > > the guest WebContents of the webview;
> > > > 2. Get SiteInstance* from the guest WebContents;
> > > > 3. Get site URL using SiteInstance::GetSiteURL;
> > > > 
> > > > Step 3 should be the the |guest_url| we created manually here but this
way
> > > would
> > > > be more proper and does not need to know the details of how the guest
url
> is
> > > > constructed.
> > > > 
> > > > [1]:
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> > > 
> > > Potentially it will give us a list of URLs (and/or partitions), should we
> > clear
> > > them all one-by-one?
> > 
> 
> We can DCHECK only one guest URL is returned.
> 
> > Oh, and one more thing to keep in mind here: in this place, we don't have a
> > webview yet, it will be loaded later, but we already need to clear the
> storage,
> > before the URL will be loaded. That is the reason why we construct it
manually
> > here.
> 
> If webview is not loaded and storage does not exist, there is nothing to clear
> and we can just bail out on this case.

Hm, somehow this line still returned me the same partition pointer as the one
used in WebViewGuest::ClearData... Looks like I was a little mistaken about
execution order of the whole thing, but the storage seems to be created some
time earlier. Anyway, on the first run we really don't have any webview at this
point, although we do have it on the re-authentication, so we should at least
clear it there.
Patchset 3 implements clearing the storage through WebViewGuest::ClearData thus
avoiding partition lookup details altogether, which seems an even better option.

[Fady Samuel, 2015-03-19 11:15:56]

fsamuel@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2015-03-19 11:15:56]

https://codereview.chromium.org/1012083002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/profiles/profile_helper.cc (right):

https://codereview.chromium.org/1012083002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_helper.cc:209: if
(guest_view->GetViewType() == extensions::WebViewGuest::Type) {
This is unnecessarily complex. All of this can be done as the following:

extensions::WebViewGuest* guest =
extensions::WebViewGuest::FromWebContents(web_contents);
if (!guest)
  return false;

...

[Ivan Podogov, 2015-03-19 11:56:32]

https://codereview.chromium.org/1012083002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/profiles/profile_helper.cc (right):

https://codereview.chromium.org/1012083002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_helper.cc:209: if
(guest_view->GetViewType() == extensions::WebViewGuest::Type) {
On 2015/03/19 11:15:56, Fady Samuel wrote:
> This is unnecessarily complex. All of this can be done as the following:
> 
> extensions::WebViewGuest* guest =
> extensions::WebViewGuest::FromWebContents(web_contents);
> if (!guest)
>   return false;
> 
> ...

This won't work: the WebContents that is passed here is the owner of the WebView
(not the contents of the WebView itself), so WebViewGuest::FromWebContents
returns nullptr.

[Fady Samuel, 2015-03-19 12:14:31]

https://codereview.chromium.org/1012083002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/profiles/profile_helper.cc (right):

https://codereview.chromium.org/1012083002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_helper.cc:209: if
(guest_view->GetViewType() == extensions::WebViewGuest::Type) {
On 2015/03/19 11:56:32, Ivan Podogov wrote:
> On 2015/03/19 11:15:56, Fady Samuel wrote:
> > This is unnecessarily complex. All of this can be done as the following:
> > 
> > extensions::WebViewGuest* guest =
> > extensions::WebViewGuest::FromWebContents(web_contents);
> > if (!guest)
> >   return false;
> > 
> > ...
> 
> This won't work: the WebContents that is passed here is the owner of the
WebView
> (not the contents of the WebView itself), so WebViewGuest::FromWebContents
> returns nullptr.

I'm confused. GuestViewBase::FromWebContents does the same thing.

[Ivan Podogov, 2015-03-19 12:18:43]

https://codereview.chromium.org/1012083002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/profiles/profile_helper.cc (right):

https://codereview.chromium.org/1012083002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_helper.cc:209: if
(guest_view->GetViewType() == extensions::WebViewGuest::Type) {
On 2015/03/19 12:14:30, Fady Samuel wrote:
> On 2015/03/19 11:56:32, Ivan Podogov wrote:
> > On 2015/03/19 11:15:56, Fady Samuel wrote:
> > > This is unnecessarily complex. All of this can be done as the following:
> > > 
> > > extensions::WebViewGuest* guest =
> > > extensions::WebViewGuest::FromWebContents(web_contents);
> > > if (!guest)
> > >   return false;
> > > 
> > > ...
> > 
> > This won't work: the WebContents that is passed here is the owner of the
> WebView
> > (not the contents of the WebView itself), so WebViewGuest::FromWebContents
> > returns nullptr.
> 
> I'm confused. GuestViewBase::FromWebContents does the same thing.

Ah, I thought you were talking about replacing the whole ForEachGuest construct!
My bad.

[Ivan Podogov, 2015-03-19 12:29:21]

https://codereview.chromium.org/1012083002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/profiles/profile_helper.cc (right):

https://codereview.chromium.org/1012083002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_helper.cc:209: if
(guest_view->GetViewType() == extensions::WebViewGuest::Type) {
On 2015/03/19 11:15:56, Fady Samuel wrote:
> This is unnecessarily complex. All of this can be done as the following:
> 
> extensions::WebViewGuest* guest =
> extensions::WebViewGuest::FromWebContents(web_contents);
> if (!guest)
>   return false;
> 
> ...

Done.

[Roman Sorokin (ftl), 2015-03-19 13:15:40]

https://codereview.chromium.org/1012083002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/profiles/profile_helper.cc (right):

https://codereview.chromium.org/1012083002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_helper.cc:204: static bool
ClearWebViewData(const base::Closure& on_clear_callback,
Why these functions are static? Please move to anonymous namespace.

https://codereview.chromium.org/1012083002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_helper.cc:212: return true;
Why we don't return value returned by ClearData?

[Ivan Podogov, 2015-03-19 13:25:37]

https://codereview.chromium.org/1012083002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/profiles/profile_helper.cc (right):

https://codereview.chromium.org/1012083002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_helper.cc:204: static bool
ClearWebViewData(const base::Closure& on_clear_callback,
On 2015/03/19 13:15:40, Roman Sorokin wrote:
> Why these functions are static? Please move to anonymous namespace.

Done.

https://codereview.chromium.org/1012083002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_helper.cc:212: return true;
On 2015/03/19 13:15:40, Roman Sorokin wrote:
> Why we don't return value returned by ClearData?

Done.

[Roman Sorokin (ftl), 2015-03-19 13:32:19]

lgtm

[xiyuan, 2015-03-19 16:33:32]

https://codereview.chromium.org/1012083002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/profiles/profile_helper.cc (right):

https://codereview.chromium.org/1012083002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_helper.cc:72: web_contents,
base::Bind(&ClearWebViewData, on_clear_callback))) {
This could cause the |on_clear_callback| be called multiple times if we have
more than one webview. We should use base::BarrierClosure to invoke it only once
after all webview is cleared.

void GetWebViewGuest(content::WebContents* guest_contents,
                     std::set<extensions::WebViewGuest*>* guest_set) {
  guest_set.insert(
      extensions::WebViewGuest::FromWebContents(web_contents));    
}

void ClearContentsData(content::WebContents* web_contents,
                       const base::Closure& on_clear_callback) {
...
  std::set<extensions::WebViewGuest*> guest_set;
  if (!manager->ForEachGuest(
      web_contents, base::Bind(&GetWebViewGuest, &guest_set)) {
    on_clear_callback.Run();
    return;
  }

  base::Closure barrier_closure = base::BarrierClosure(
      guest_set.size(), on_clear_callback);
  for (const auto& guest: guest_set) {
    guest->ClearData(..., barrier_closure);
  }
...

https://codereview.chromium.org/1012083002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_helper.cc:383:
ClearSigninProfile(base::Closure(), nullptr);
Could you double check whether passing nullptr is the right thing to do? If this
is only called for browser restart etc without WebUILoginView active, then this
is okay. Otherwise, we need to pass correct WebContents here.

[Ivan Podogov, 2015-03-19 16:59:06]

https://codereview.chromium.org/1012083002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/profiles/profile_helper.cc (right):

https://codereview.chromium.org/1012083002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_helper.cc:72: web_contents,
base::Bind(&ClearWebViewData, on_clear_callback))) {
On 2015/03/19 16:33:31, xiyuan wrote:
> This could cause the |on_clear_callback| be called multiple times if we have
> more than one webview. We should use base::BarrierClosure to invoke it only
once
> after all webview is cleared.
> 
> void GetWebViewGuest(content::WebContents* guest_contents,
>                      std::set<extensions::WebViewGuest*>* guest_set) {
>   guest_set.insert(
>       extensions::WebViewGuest::FromWebContents(web_contents));    
> }
> 
> void ClearContentsData(content::WebContents* web_contents,
>                        const base::Closure& on_clear_callback) {
> ...
>   std::set<extensions::WebViewGuest*> guest_set;
>   if (!manager->ForEachGuest(
>       web_contents, base::Bind(&GetWebViewGuest, &guest_set)) {
>     on_clear_callback.Run();
>     return;
>   }
> 
>   base::Closure barrier_closure = base::BarrierClosure(
>       guest_set.size(), on_clear_callback);
>   for (const auto& guest: guest_set) {
>     guest->ClearData(..., barrier_closure);
>   }
> ...

In fact, it cannot: ForEachGuest will stop iterating over guests once its
callback returns true, and that will be its return value. The same goes for
ClearData: it returns true only when callback is going to be called. So,
ForEachGuest returns true only when on_clear_callback was called exactly once,
and false if it was not called.
In this specific implementation we expect to clear cookies of a single "sing-in"
WebView, the only one in the new GAIA flow in Chrome OS, so this should be more
than enough.

https://codereview.chromium.org/1012083002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_helper.cc:383:
ClearSigninProfile(base::Closure(), nullptr);
On 2015/03/19 16:33:31, xiyuan wrote:
> Could you double check whether passing nullptr is the right thing to do? If
this
> is only called for browser restart etc without WebUILoginView active, then
this
> is okay. Otherwise, we need to pass correct WebContents here.

I'm afraid that I see no easy way to do it: it would require passing WebContents
through a dozen of different interfaces with numerous calls (that's why I was
against introducing WebContents to ClearSigninProfile in the first place).
Anyway, if you want to add some additional complex functionality here, please
file a separate issue for that. Right here I'm only fixing a specific P1-bug.
As for our login flow, ClearSigninProfile will be called every time the flow is
(re)started, with the correct WebContents associated.

[xiyuan, 2015-03-19 18:22:19]

https://codereview.chromium.org/1012083002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/profiles/profile_helper.cc (right):

https://codereview.chromium.org/1012083002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_helper.cc:72: web_contents,
base::Bind(&ClearWebViewData, on_clear_callback))) {
On 2015/03/19 16:59:06, Ivan Podogov wrote:
> On 2015/03/19 16:33:31, xiyuan wrote:
> > This could cause the |on_clear_callback| be called multiple times if we have
> > more than one webview. We should use base::BarrierClosure to invoke it only
> once
> > after all webview is cleared.
> > 
> > void GetWebViewGuest(content::WebContents* guest_contents,
> >                      std::set<extensions::WebViewGuest*>* guest_set) {
> >   guest_set.insert(
> >       extensions::WebViewGuest::FromWebContents(web_contents));    
> > }
> > 
> > void ClearContentsData(content::WebContents* web_contents,
> >                        const base::Closure& on_clear_callback) {
> > ...
> >   std::set<extensions::WebViewGuest*> guest_set;
> >   if (!manager->ForEachGuest(
> >       web_contents, base::Bind(&GetWebViewGuest, &guest_set)) {
> >     on_clear_callback.Run();
> >     return;
> >   }
> > 
> >   base::Closure barrier_closure = base::BarrierClosure(
> >       guest_set.size(), on_clear_callback);
> >   for (const auto& guest: guest_set) {
> >     guest->ClearData(..., barrier_closure);
> >   }
> > ...
> 
> In fact, it cannot: ForEachGuest will stop iterating over guests once its
> callback returns true, and that will be its return value. The same goes for
> ClearData: it returns true only when callback is going to be called. So,
> ForEachGuest returns true only when on_clear_callback was called exactly once,
> and false if it was not called.
> In this specific implementation we expect to clear cookies of a single
"sing-in"
> WebView, the only one in the new GAIA flow in Chrome OS, so this should be
more
> than enough.

You are right about the callback is invoked just once.

However, the fact that a piece of code works does not mean it is correct. There
will be more than one webview soon and this will break since we don't know which
webview would be visited first in ForEachGuest callback. So this might break in
the future. We should either clear all or clear the correct one (probably by
giving the webview a unique partition name).

https://codereview.chromium.org/1012083002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_helper.cc:383:
ClearSigninProfile(base::Closure(), nullptr);
On 2015/03/19 16:59:06, Ivan Podogov wrote:
> On 2015/03/19 16:33:31, xiyuan wrote:
> > Could you double check whether passing nullptr is the right thing to do? If
> this
> > is only called for browser restart etc without WebUILoginView active, then
> this
> > is okay. Otherwise, we need to pass correct WebContents here.
> 
> I'm afraid that I see no easy way to do it: it would require passing
WebContents
> through a dozen of different interfaces with numerous calls (that's why I was
> against introducing WebContents to ClearSigninProfile in the first place).
> Anyway, if you want to add some additional complex functionality here, please
> file a separate issue for that. Right here I'm only fixing a specific P1-bug.
> As for our login flow, ClearSigninProfile will be called every time the flow
is
> (re)started, with the correct WebContents associated.

I was asking to verify this is only called on browser restart (crash or apply
flags) and not in regular user login flow. If that is case, nullptr is the right
thing to do. From what I read from the code, this is probably the case and we
don't need to change. 

Otherwise, we need to do it properly. We could either make WebUILoginView
register with ProfileHelper (i.e. pass in its WebContents*) or use something
similar to AccessibilityManager::LoadChromeVoxToUserScreen [1].

The login code is complicated and we should be careful not to introduce new bugs
when we making changes.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

[Ivan Podogov, 2015-03-19 18:42:43]

https://codereview.chromium.org/1012083002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/profiles/profile_helper.cc (right):

https://codereview.chromium.org/1012083002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_helper.cc:72: web_contents,
base::Bind(&ClearWebViewData, on_clear_callback))) {
On 2015/03/19 18:22:19, xiyuan wrote:
> On 2015/03/19 16:59:06, Ivan Podogov wrote:
> > On 2015/03/19 16:33:31, xiyuan wrote:
> > > This could cause the |on_clear_callback| be called multiple times if we
have
> > > more than one webview. We should use base::BarrierClosure to invoke it
only
> > once
> > > after all webview is cleared.
> > > 
> > > void GetWebViewGuest(content::WebContents* guest_contents,
> > >                      std::set<extensions::WebViewGuest*>* guest_set) {
> > >   guest_set.insert(
> > >       extensions::WebViewGuest::FromWebContents(web_contents));    
> > > }
> > > 
> > > void ClearContentsData(content::WebContents* web_contents,
> > >                        const base::Closure& on_clear_callback) {
> > > ...
> > >   std::set<extensions::WebViewGuest*> guest_set;
> > >   if (!manager->ForEachGuest(
> > >       web_contents, base::Bind(&GetWebViewGuest, &guest_set)) {
> > >     on_clear_callback.Run();
> > >     return;
> > >   }
> > > 
> > >   base::Closure barrier_closure = base::BarrierClosure(
> > >       guest_set.size(), on_clear_callback);
> > >   for (const auto& guest: guest_set) {
> > >     guest->ClearData(..., barrier_closure);
> > >   }
> > > ...
> > 
> > In fact, it cannot: ForEachGuest will stop iterating over guests once its
> > callback returns true, and that will be its return value. The same goes for
> > ClearData: it returns true only when callback is going to be called. So,
> > ForEachGuest returns true only when on_clear_callback was called exactly
once,
> > and false if it was not called.
> > In this specific implementation we expect to clear cookies of a single
> "sing-in"
> > WebView, the only one in the new GAIA flow in Chrome OS, so this should be
> more
> > than enough.
> 
> You are right about the callback is invoked just once.
> 
> However, the fact that a piece of code works does not mean it is correct.
There
> will be more than one webview soon and this will break since we don't know
which
> webview would be visited first in ForEachGuest callback. So this might break
in
> the future. We should either clear all or clear the correct one (probably by
> giving the webview a unique partition name).

It is correct as long as ClearSigninProfile is called with correct arguments
(i.e. WebContents with one WebView). Yesterday you even proposed to add a DCHECK
that there's only one WebView.
As for the unique partition name, that was what I was trying to clear in the
very first patchset. This one is more general, but making it extremely
overengineered is not the way to fix the bug.

[xiyuan, 2015-03-19 19:49:28]

https://codereview.chromium.org/1012083002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/profiles/profile_helper.cc (right):

https://codereview.chromium.org/1012083002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_helper.cc:72: web_contents,
base::Bind(&ClearWebViewData, on_clear_callback))) {
On 2015/03/19 18:42:43, Ivan Podogov wrote:
> On 2015/03/19 18:22:19, xiyuan wrote:
> > On 2015/03/19 16:59:06, Ivan Podogov wrote:
> > > On 2015/03/19 16:33:31, xiyuan wrote:
> > > > This could cause the |on_clear_callback| be called multiple times if we
> have
> > > > more than one webview. We should use base::BarrierClosure to invoke it
> only
> > > once
> > > > after all webview is cleared.
> > > > 
> > > > void GetWebViewGuest(content::WebContents* guest_contents,
> > > >                      std::set<extensions::WebViewGuest*>* guest_set) {
> > > >   guest_set.insert(
> > > >       extensions::WebViewGuest::FromWebContents(web_contents));    
> > > > }
> > > > 
> > > > void ClearContentsData(content::WebContents* web_contents,
> > > >                        const base::Closure& on_clear_callback) {
> > > > ...
> > > >   std::set<extensions::WebViewGuest*> guest_set;
> > > >   if (!manager->ForEachGuest(
> > > >       web_contents, base::Bind(&GetWebViewGuest, &guest_set)) {
> > > >     on_clear_callback.Run();
> > > >     return;
> > > >   }
> > > > 
> > > >   base::Closure barrier_closure = base::BarrierClosure(
> > > >       guest_set.size(), on_clear_callback);
> > > >   for (const auto& guest: guest_set) {
> > > >     guest->ClearData(..., barrier_closure);
> > > >   }
> > > > ...
> > > 
> > > In fact, it cannot: ForEachGuest will stop iterating over guests once its
> > > callback returns true, and that will be its return value. The same goes
for
> > > ClearData: it returns true only when callback is going to be called. So,
> > > ForEachGuest returns true only when on_clear_callback was called exactly
> once,
> > > and false if it was not called.
> > > In this specific implementation we expect to clear cookies of a single
> > "sing-in"
> > > WebView, the only one in the new GAIA flow in Chrome OS, so this should be
> > more
> > > than enough.
> > 
> > You are right about the callback is invoked just once.
> > 
> > However, the fact that a piece of code works does not mean it is correct.
> There
> > will be more than one webview soon and this will break since we don't know
> which
> > webview would be visited first in ForEachGuest callback. So this might break
> in
> > the future. We should either clear all or clear the correct one (probably by
> > giving the webview a unique partition name).
> 
> It is correct as long as ClearSigninProfile is called with correct arguments
> (i.e. WebContents with one WebView). Yesterday you even proposed to add a
DCHECK
> that there's only one WebView.
> As for the unique partition name, that was what I was trying to clear in the
> very first patchset. This one is more general, but making it extremely
> overengineered is not the way to fix the bug.

It will not be correct when we add webviews for enrollment screen and hotrod
pairing screen because the WebContents (in WebUILoginView) will host more than
one webviews by then.

DCHECK is a safe beucase it makes sure we update the code when it breaks. But
this CL as it is now would just silently break. That is why DCHECK is fine to
assume one webview but this CL is not.

PS1 is simple but it is fragile. It could be broken when guest url format is
changed or when we add partition names to the webviews. That guest url does not
serve as a unique partition name. That guest url is the default in-memory
storage and would be shared by all webviews that do not have partition names. It
is a problem we need to fix later when more webview are added (i.e. sign-in
webview should not get cookies from enrollment webview).

[xiyuan, 2015-03-20 03:09:37]

On 2015/03/19 19:49:28, xiyuan wrote:
>
https://codereview.chromium.org/1012083002/diff/80001/chrome/browser/chromeos...
> File chrome/browser/chromeos/profiles/profile_helper.cc (right):
> 
>
https://codereview.chromium.org/1012083002/diff/80001/chrome/browser/chromeos...
> chrome/browser/chromeos/profiles/profile_helper.cc:72: web_contents,
> base::Bind(&ClearWebViewData, on_clear_callback))) {
> On 2015/03/19 18:42:43, Ivan Podogov wrote:
> > On 2015/03/19 18:22:19, xiyuan wrote:
> > > On 2015/03/19 16:59:06, Ivan Podogov wrote:
> > > > On 2015/03/19 16:33:31, xiyuan wrote:
> > > > > This could cause the |on_clear_callback| be called multiple times if
we
> > have
> > > > > more than one webview. We should use base::BarrierClosure to invoke it
> > only
> > > > once
> > > > > after all webview is cleared.
> > > > > 
> > > > > void GetWebViewGuest(content::WebContents* guest_contents,
> > > > >                      std::set<extensions::WebViewGuest*>* guest_set) {
> > > > >   guest_set.insert(
> > > > >       extensions::WebViewGuest::FromWebContents(web_contents));    
> > > > > }
> > > > > 
> > > > > void ClearContentsData(content::WebContents* web_contents,
> > > > >                        const base::Closure& on_clear_callback) {
> > > > > ...
> > > > >   std::set<extensions::WebViewGuest*> guest_set;
> > > > >   if (!manager->ForEachGuest(
> > > > >       web_contents, base::Bind(&GetWebViewGuest, &guest_set)) {
> > > > >     on_clear_callback.Run();
> > > > >     return;
> > > > >   }
> > > > > 
> > > > >   base::Closure barrier_closure = base::BarrierClosure(
> > > > >       guest_set.size(), on_clear_callback);
> > > > >   for (const auto& guest: guest_set) {
> > > > >     guest->ClearData(..., barrier_closure);
> > > > >   }
> > > > > ...
> > > > 
> > > > In fact, it cannot: ForEachGuest will stop iterating over guests once
its
> > > > callback returns true, and that will be its return value. The same goes
> for
> > > > ClearData: it returns true only when callback is going to be called. So,
> > > > ForEachGuest returns true only when on_clear_callback was called exactly
> > once,
> > > > and false if it was not called.
> > > > In this specific implementation we expect to clear cookies of a single
> > > "sing-in"
> > > > WebView, the only one in the new GAIA flow in Chrome OS, so this should
be
> > > more
> > > > than enough.
> > > 
> > > You are right about the callback is invoked just once.
> > > 
> > > However, the fact that a piece of code works does not mean it is correct.
> > There
> > > will be more than one webview soon and this will break since we don't know
> > which
> > > webview would be visited first in ForEachGuest callback. So this might
break
> > in
> > > the future. We should either clear all or clear the correct one (probably
by
> > > giving the webview a unique partition name).
> > 
> > It is correct as long as ClearSigninProfile is called with correct arguments
> > (i.e. WebContents with one WebView). Yesterday you even proposed to add a
> DCHECK
> > that there's only one WebView.
> > As for the unique partition name, that was what I was trying to clear in the
> > very first patchset. This one is more general, but making it extremely
> > overengineered is not the way to fix the bug.
> 
> It will not be correct when we add webviews for enrollment screen and hotrod
> pairing screen because the WebContents (in WebUILoginView) will host more than
> one webviews by then.
> 
> DCHECK is a safe beucase it makes sure we update the code when it breaks. But
> this CL as it is now would just silently break. That is why DCHECK is fine to
> assume one webview but this CL is not.
> 
> PS1 is simple but it is fragile. It could be broken when guest url format is
> changed or when we add partition names to the webviews. That guest url does
not
> serve as a unique partition name. That guest url is the default in-memory
> storage and would be shared by all webviews that do not have partition names.
It
> is a problem we need to fix later when more webview are added (i.e. sign-in
> webview should not get cookies from enrollment webview).

I think the difference is rooted on whether we would make the webviews use
different partitions. All my previous comments were based on yes, we need
isolated partitions for those webviews. And just now it struck me that this
might not be 100% true. We have old logic implemented using a shared sign-in
profile so we could actually make it work with a shared partition of all
webviews in the login screen. If we all agree on this, then I think PS5 is
actually good.

[Ivan Podogov, 2015-03-20 07:51:36]

On 2015/03/20 03:09:37, xiyuan wrote:
> On 2015/03/19 19:49:28, xiyuan wrote:
> >
>
https://codereview.chromium.org/1012083002/diff/80001/chrome/browser/chromeos...
> > File chrome/browser/chromeos/profiles/profile_helper.cc (right):
> > 
> >
>
https://codereview.chromium.org/1012083002/diff/80001/chrome/browser/chromeos...
> > chrome/browser/chromeos/profiles/profile_helper.cc:72: web_contents,
> > base::Bind(&ClearWebViewData, on_clear_callback))) {
> > On 2015/03/19 18:42:43, Ivan Podogov wrote:
> > > On 2015/03/19 18:22:19, xiyuan wrote:
> > > > On 2015/03/19 16:59:06, Ivan Podogov wrote:
> > > > > On 2015/03/19 16:33:31, xiyuan wrote:
> > > > > > This could cause the |on_clear_callback| be called multiple times if
> we
> > > have
> > > > > > more than one webview. We should use base::BarrierClosure to invoke
it
> > > only
> > > > > once
> > > > > > after all webview is cleared.
> > > > > > 
> > > > > > void GetWebViewGuest(content::WebContents* guest_contents,
> > > > > >                      std::set<extensions::WebViewGuest*>* guest_set)
{
> > > > > >   guest_set.insert(
> > > > > >       extensions::WebViewGuest::FromWebContents(web_contents));    
> > > > > > }
> > > > > > 
> > > > > > void ClearContentsData(content::WebContents* web_contents,
> > > > > >                        const base::Closure& on_clear_callback) {
> > > > > > ...
> > > > > >   std::set<extensions::WebViewGuest*> guest_set;
> > > > > >   if (!manager->ForEachGuest(
> > > > > >       web_contents, base::Bind(&GetWebViewGuest, &guest_set)) {
> > > > > >     on_clear_callback.Run();
> > > > > >     return;
> > > > > >   }
> > > > > > 
> > > > > >   base::Closure barrier_closure = base::BarrierClosure(
> > > > > >       guest_set.size(), on_clear_callback);
> > > > > >   for (const auto& guest: guest_set) {
> > > > > >     guest->ClearData(..., barrier_closure);
> > > > > >   }
> > > > > > ...
> > > > > 
> > > > > In fact, it cannot: ForEachGuest will stop iterating over guests once
> its
> > > > > callback returns true, and that will be its return value. The same
goes
> > for
> > > > > ClearData: it returns true only when callback is going to be called.
So,
> > > > > ForEachGuest returns true only when on_clear_callback was called
exactly
> > > once,
> > > > > and false if it was not called.
> > > > > In this specific implementation we expect to clear cookies of a single
> > > > "sing-in"
> > > > > WebView, the only one in the new GAIA flow in Chrome OS, so this
should
> be
> > > > more
> > > > > than enough.
> > > > 
> > > > You are right about the callback is invoked just once.
> > > > 
> > > > However, the fact that a piece of code works does not mean it is
correct.
> > > There
> > > > will be more than one webview soon and this will break since we don't
know
> > > which
> > > > webview would be visited first in ForEachGuest callback. So this might
> break
> > > in
> > > > the future. We should either clear all or clear the correct one
(probably
> by
> > > > giving the webview a unique partition name).
> > > 
> > > It is correct as long as ClearSigninProfile is called with correct
arguments
> > > (i.e. WebContents with one WebView). Yesterday you even proposed to add a
> > DCHECK
> > > that there's only one WebView.
> > > As for the unique partition name, that was what I was trying to clear in
the
> > > very first patchset. This one is more general, but making it extremely
> > > overengineered is not the way to fix the bug.
> > 
> > It will not be correct when we add webviews for enrollment screen and hotrod
> > pairing screen because the WebContents (in WebUILoginView) will host more
than
> > one webviews by then.
> > 
> > DCHECK is a safe beucase it makes sure we update the code when it breaks.
But
> > this CL as it is now would just silently break. That is why DCHECK is fine
to
> > assume one webview but this CL is not.
> > 
> > PS1 is simple but it is fragile. It could be broken when guest url format is
> > changed or when we add partition names to the webviews. That guest url does
> not
> > serve as a unique partition name. That guest url is the default in-memory
> > storage and would be shared by all webviews that do not have partition
names.
> It
> > is a problem we need to fix later when more webview are added (i.e. sign-in
> > webview should not get cookies from enrollment webview).
> 
> I think the difference is rooted on whether we would make the webviews use
> different partitions. All my previous comments were based on yes, we need
> isolated partitions for those webviews. And just now it struck me that this
> might not be 100% true. We have old logic implemented using a shared sign-in
> profile so we could actually make it work with a shared partition of all
> webviews in the login screen. If we all agree on this, then I think PS5 is
> actually good.

OK, I didn't know about plans for adding enrollment and hotrod pairing screens.
The partition that is currently used for the GAIA WebView is the default one
(without a name) for the login screen domain ("oobe", and we also already have
some other code that relies on that:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
- this one will probably also break once we'll start dealing with
material-design OOBE that is located in another domain, "oobe-md"). Relying on
that has its own advantages: for example, as in
UserSessionManager::RestoreAuthSessionImpl example, or like in PS1, we can
address that partition without having any active WebView (e.g. before it was
loaded or after it was closed).
Since we don't really know how everything will be implemented in the future, we
might either write large and complex perfectly universal implementations that
might not be used even once (and rely on some fragile things like still showing
the WebView and having it associated to some WebContents), or wait until that
future comes and go with the stuff we really need.
I don't know about partitions that will be used for enrollment and hotrod
pairing screens, I can only expect them to be using the same credentials
(unnamed, for current domain), and I'm not sure who to ask if that will or will
not be the case.

[Ivan Podogov, 2015-03-20 14:12:33]

On 2015/03/20 07:51:36, Ivan Podogov wrote:
> On 2015/03/20 03:09:37, xiyuan wrote:
> > On 2015/03/19 19:49:28, xiyuan wrote:
> > >
> >
>
https://codereview.chromium.org/1012083002/diff/80001/chrome/browser/chromeos...
> > > File chrome/browser/chromeos/profiles/profile_helper.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1012083002/diff/80001/chrome/browser/chromeos...
> > > chrome/browser/chromeos/profiles/profile_helper.cc:72: web_contents,
> > > base::Bind(&ClearWebViewData, on_clear_callback))) {
> > > On 2015/03/19 18:42:43, Ivan Podogov wrote:
> > > > On 2015/03/19 18:22:19, xiyuan wrote:
> > > > > On 2015/03/19 16:59:06, Ivan Podogov wrote:
> > > > > > On 2015/03/19 16:33:31, xiyuan wrote:
> > > > > > > This could cause the |on_clear_callback| be called multiple times
if
> > we
> > > > have
> > > > > > > more than one webview. We should use base::BarrierClosure to
invoke
> it
> > > > only
> > > > > > once
> > > > > > > after all webview is cleared.
> > > > > > > 
> > > > > > > void GetWebViewGuest(content::WebContents* guest_contents,
> > > > > > >                      std::set<extensions::WebViewGuest*>*
guest_set)
> {
> > > > > > >   guest_set.insert(
> > > > > > >       extensions::WebViewGuest::FromWebContents(web_contents));   

> > > > > > > }
> > > > > > > 
> > > > > > > void ClearContentsData(content::WebContents* web_contents,
> > > > > > >                        const base::Closure& on_clear_callback) {
> > > > > > > ...
> > > > > > >   std::set<extensions::WebViewGuest*> guest_set;
> > > > > > >   if (!manager->ForEachGuest(
> > > > > > >       web_contents, base::Bind(&GetWebViewGuest, &guest_set)) {
> > > > > > >     on_clear_callback.Run();
> > > > > > >     return;
> > > > > > >   }
> > > > > > > 
> > > > > > >   base::Closure barrier_closure = base::BarrierClosure(
> > > > > > >       guest_set.size(), on_clear_callback);
> > > > > > >   for (const auto& guest: guest_set) {
> > > > > > >     guest->ClearData(..., barrier_closure);
> > > > > > >   }
> > > > > > > ...
> > > > > > 
> > > > > > In fact, it cannot: ForEachGuest will stop iterating over guests
once
> > its
> > > > > > callback returns true, and that will be its return value. The same
> goes
> > > for
> > > > > > ClearData: it returns true only when callback is going to be called.
> So,
> > > > > > ForEachGuest returns true only when on_clear_callback was called
> exactly
> > > > once,
> > > > > > and false if it was not called.
> > > > > > In this specific implementation we expect to clear cookies of a
single
> > > > > "sing-in"
> > > > > > WebView, the only one in the new GAIA flow in Chrome OS, so this
> should
> > be
> > > > > more
> > > > > > than enough.
> > > > > 
> > > > > You are right about the callback is invoked just once.
> > > > > 
> > > > > However, the fact that a piece of code works does not mean it is
> correct.
> > > > There
> > > > > will be more than one webview soon and this will break since we don't
> know
> > > > which
> > > > > webview would be visited first in ForEachGuest callback. So this might
> > break
> > > > in
> > > > > the future. We should either clear all or clear the correct one
> (probably
> > by
> > > > > giving the webview a unique partition name).
> > > > 
> > > > It is correct as long as ClearSigninProfile is called with correct
> arguments
> > > > (i.e. WebContents with one WebView). Yesterday you even proposed to add
a
> > > DCHECK
> > > > that there's only one WebView.
> > > > As for the unique partition name, that was what I was trying to clear in
> the
> > > > very first patchset. This one is more general, but making it extremely
> > > > overengineered is not the way to fix the bug.
> > > 
> > > It will not be correct when we add webviews for enrollment screen and
hotrod
> > > pairing screen because the WebContents (in WebUILoginView) will host more
> than
> > > one webviews by then.
> > > 
> > > DCHECK is a safe beucase it makes sure we update the code when it breaks.
> But
> > > this CL as it is now would just silently break. That is why DCHECK is fine
> to
> > > assume one webview but this CL is not.
> > > 
> > > PS1 is simple but it is fragile. It could be broken when guest url format
is
> > > changed or when we add partition names to the webviews. That guest url
does
> > not
> > > serve as a unique partition name. That guest url is the default in-memory
> > > storage and would be shared by all webviews that do not have partition
> names.
> > It
> > > is a problem we need to fix later when more webview are added (i.e.
sign-in
> > > webview should not get cookies from enrollment webview).
> > 
> > I think the difference is rooted on whether we would make the webviews use
> > different partitions. All my previous comments were based on yes, we need
> > isolated partitions for those webviews. And just now it struck me that this
> > might not be 100% true. We have old logic implemented using a shared sign-in
> > profile so we could actually make it work with a shared partition of all
> > webviews in the login screen. If we all agree on this, then I think PS5 is
> > actually good.
> 
> OK, I didn't know about plans for adding enrollment and hotrod pairing
screens.
> The partition that is currently used for the GAIA WebView is the default one
> (without a name) for the login screen domain ("oobe", and we also already have
> some other code that relies on that:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
> - this one will probably also break once we'll start dealing with
> material-design OOBE that is located in another domain, "oobe-md"). Relying on
> that has its own advantages: for example, as in
> UserSessionManager::RestoreAuthSessionImpl example, or like in PS1, we can
> address that partition without having any active WebView (e.g. before it was
> loaded or after it was closed).
> Since we don't really know how everything will be implemented in the future,
we
> might either write large and complex perfectly universal implementations that
> might not be used even once (and rely on some fragile things like still
showing
> the WebView and having it associated to some WebContents), or wait until that
> future comes and go with the stuff we really need.
> I don't know about partitions that will be used for enrollment and hotrod
> pairing screens, I can only expect them to be using the same credentials
> (unnamed, for current domain), and I'm not sure who to ask if that will or
will
> not be the case.

Gave it a little more thought and simply changed the webcontents set to
partitions set: this way every partition will be cleared only once, so it should
be fine.

[xiyuan, 2015-03-20 15:59:53]

LGTM

Thank you for bearing with me and going the extra mile.

[xiyuan, 2015-03-20 17:36:21]

https://codereview.chromium.org/1012083002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/profiles/profile_helper.cc (right):

https://codereview.chromium.org/1012083002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/profiles/profile_helper.cc:75:
manager->ForEachGuest(web_contents,
Since we are interested in storage partitions, we probably can use
BrowserContext::ForEachStoragePartition. So we just clear everything in signin
profile. And there would be no need to pass in the owner webcontents.

[Ivan Podogov, 2015-03-21 14:47:43]

https://codereview.chromium.org/1012083002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/profiles/profile_helper.cc (right):

https://codereview.chromium.org/1012083002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/profiles/profile_helper.cc:75:
manager->ForEachGuest(web_contents,
On 2015/03/20 17:36:21, xiyuan wrote:
> Since we are interested in storage partitions, we probably can use
> BrowserContext::ForEachStoragePartition. So we just clear everything in signin
> profile. And there would be no need to pass in the owner webcontents.

Interesting thing to note here is that we have two partitions active at this
moment: profile's default one, and the webview storage. ForEachStoragePartition
enumerates both (which is not surprising), and this code here enumerates only
the second one (which is also quite logical). Thing to note here is that the
default partition will actually be cleared in ClearSigninProfile anyway by the
BrowsingDataRemover.
In the future we may want to use ForEachStoragePartition inside the
BrowsingDataRemover (probably under some additional flag), but for now I think
this code will do just fine.

[xiyuan, 2015-03-22 00:12:02]

https://codereview.chromium.org/1012083002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/profiles/profile_helper.cc (right):

https://codereview.chromium.org/1012083002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/profiles/profile_helper.cc:75:
manager->ForEachGuest(web_contents,
On 2015/03/21 14:47:43, Ivan Podogov wrote:
> On 2015/03/20 17:36:21, xiyuan wrote:
> > Since we are interested in storage partitions, we probably can use
> > BrowserContext::ForEachStoragePartition. So we just clear everything in
signin
> > profile. And there would be no need to pass in the owner webcontents.
> 
> Interesting thing to note here is that we have two partitions active at this
> moment: profile's default one, and the webview storage.
ForEachStoragePartition
> enumerates both (which is not surprising), and this code here enumerates only
> the second one (which is also quite logical). Thing to note here is that the
> default partition will actually be cleared in ClearSigninProfile anyway by the
> BrowsingDataRemover.
> In the future we may want to use ForEachStoragePartition inside the
> BrowsingDataRemover (probably under some additional flag), but for now I think
> this code will do just fine.

Okay.

I point out ForEachStoragePartition because it fits better with
CleanSigninProfile and we could avoid passing in a WebContents.

[Ivan Podogov, 2015-03-23 06:54:30]

The CQ bit was checked by ginkage@chromium.org

[Ivan Podogov, 2015-03-23 06:54:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
rsorokin@chromium.org
Link to the patchset: https://codereview.chromium.org/1012083002/#ps100001
(title: "Clear all WebView partitions.")

[commit-bot: I haz the power, 2015-03-23 06:54:38]

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

[commit-bot: I haz the power, 2015-03-23 06:57:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-23 06:57:27]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ivan Podogov, 2015-03-23 07:03:09]

The CQ bit was checked by ginkage@chromium.org

[Ivan Podogov, 2015-03-23 07:03:10]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
rsorokin@chromium.org
Link to the patchset: https://codereview.chromium.org/1012083002/#ps120001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-03-23 07:03:15]

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

[commit-bot: I haz the power, 2015-03-23 07:18:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-23 07:18:56]

Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ivan Podogov, 2015-03-23 07:36:58]

The CQ bit was checked by ginkage@chromium.org

[Ivan Podogov, 2015-03-23 07:36:59]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
rsorokin@chromium.org
Link to the patchset: https://codereview.chromium.org/1012083002/#ps130001
(title: "Fix build.")

[commit-bot: I haz the power, 2015-03-23 07:37:09]

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

[commit-bot: I haz the power, 2015-03-23 08:51:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-23 08:51:27]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ivan Podogov, 2015-03-23 09:26:14]

The CQ bit was checked by ginkage@chromium.org

[Ivan Podogov, 2015-03-23 09:26:14]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
rsorokin@chromium.org
Link to the patchset: https://codereview.chromium.org/1012083002/#ps150001
(title: "Fix test.")

[commit-bot: I haz the power, 2015-03-23 09:26:32]

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

[commit-bot: I haz the power, 2015-03-23 10:37:09]

Committed patchset #9 (id:150001)

[commit-bot: I haz the power, 2015-03-23 10:37:51]

Patchset 9 (id:??) landed as
https://crrev.com/74a79da40a413af2d4f470603d90eb02e15fc171
Cr-Commit-Position: refs/heads/master@{#321754}