Resolve new GAIA flow's infinite loop.

chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
@@ skipping 20 matching lines @@
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/grit/generated_resources.h"
 #include "chromeos/chromeos_switches.h"
 #include "chromeos/login/auth/user_context.h"
 #include "chromeos/settings/cros_settings_names.h"
 #include "components/login/localized_values_builder.h"
 #include "components/user_manager/user_manager.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/storage_partition.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/common/url_constants.h"
 #include "google_apis/gaia/gaia_auth_util.h"
 #include "google_apis/gaia/gaia_switches.h"
 #include "google_apis/gaia/gaia_urls.h"
 #include "ui/base/ime/chromeos/input_method_manager.h"
 #include "ui/base/l10n/l10n_util.h"
 
 using content::BrowserThread;
 
 namespace chromeos {
 
@@ skipping 585 matching lines @@
 void GaiaScreenHandler::OnDnsCleared() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   dns_clear_task_running_ = false;
   dns_cleared_ = true;
   ShowGaiaScreenIfReady();
 }
 
 void GaiaScreenHandler::StartClearingCookies(
     const base::Closure& on_clear_callback) {
   cookies_cleared_ = false;
+
+  if (StartupUtils::IsWebviewSigninEnabled()) {

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

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.

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

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.

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

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...

+    content::WebContents* contents = web_ui()->GetWebContents();
+    content::BrowserContext* context = contents->GetBrowserContext();
+    GURL guest_url(std::string(content::kGuestScheme) +
+                   url::kStandardSchemeSeparator +
+                   contents->GetLastCommittedURL().GetContent());

[xiyuan, 2015/03/17 16:05:52]

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/...

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

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

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

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]

We can DCHECK only one guest URL is returned.
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]

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.

+    content::StoragePartition* partition =
+        content::BrowserContext::GetStoragePartitionForSite(context, guest_url);
+    DCHECK(partition);
+    if (partition) {

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

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(...

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

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.

+      LOG(ERROR) << "Start clearing cookies for " << guest_url.spec();

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

Why is it LOG(ERROR)?

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

Forgotten debug logging, shouldn't be here.

+      partition->ClearData(
+          content::StoragePartition::REMOVE_DATA_MASK_ALL,
+          content::StoragePartition::QUOTA_MANAGED_STORAGE_MASK_ALL, GURL(),
+          content::StoragePartition::OriginMatcherFunction(), base::Time(),
+          base::Time::Now(),
+          base::Bind(&GaiaScreenHandler::OnCookiesCleared,
+                     weak_factory_.GetWeakPtr(), on_clear_callback));
+      return;
+    } else {

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

nit: don't use else after return.

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

Done.

+      LOG(ERROR) << "Storage partition not found for " << guest_url.spec();
+    }
+  }
+
   ProfileHelper* profile_helper = ProfileHelper::Get();
   LOG_ASSERT(Profile::FromWebUI(web_ui()) ==
              profile_helper->GetSigninProfile());
   profile_helper->ClearSigninProfile(
       base::Bind(&GaiaScreenHandler::OnCookiesCleared,
                  weak_factory_.GetWeakPtr(),
                  on_clear_callback));
 }
 
 void GaiaScreenHandler::OnCookiesCleared(
@@ skipping 194 matching lines @@
 SigninScreenHandlerDelegate* GaiaScreenHandler::Delegate() {
   DCHECK(signin_screen_handler_);
   return signin_screen_handler_->delegate_;
 }
 
 void GaiaScreenHandler::SetSigninScreenHandler(SigninScreenHandler* handler) {
   signin_screen_handler_ = handler;
 }
 
 }  // namespace chromeos