Resolve new GAIA flow's infinite loop.

chrome/browser/chromeos/profiles/profile_helper.cc

 // Copyright (c) 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/chromeos/profiles/profile_helper.h"
 
 #include "base/callback.h"
 #include "base/command_line.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browsing_data/browsing_data_helper.h"
 #include "chrome/browser/chromeos/login/signin/oauth2_login_manager_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/profiles/profiles_state.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_switches.h"
 #include "chromeos/chromeos_switches.h"
 #include "components/user_manager/user.h"
 #include "components/user_manager/user_manager.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/storage_partition.h"
+#include "content/public/browser/web_contents.h"
+#include "extensions/browser/guest_view/guest_view_manager.h"
+#include "extensions/browser/guest_view/web_view/web_view_guest.h"
 
 namespace chromeos {
 
 namespace {
 
 // As defined in /chromeos/dbus/cryptohome_client.cc.
 static const char kUserIdHashSuffix[] = "-hash";
 
 bool ShouldAddProfileDirPrefix(const std::string& user_id_hash) {
   // Do not add profile dir prefix for legacy profile dir and test
@@ skipping 10 matching lines @@
  public:
   explicit UsernameHashMatcher(const std::string& h) : username_hash(h) {}
   bool operator()(const user_manager::User* user) const {
     return user->username_hash() == username_hash;
   }
 
  private:
   const std::string& username_hash;
 };
 
+bool ClearWebViewData(const base::Closure& on_clear_callback,
+                      content::WebContents* web_contents) {
+  extensions::WebViewGuest* view_guest =
+      extensions::WebViewGuest::FromWebContents(web_contents);
+  if (!view_guest)
+    return false;
+  return view_guest->ClearData(base::Time(),
+                               content::StoragePartition::REMOVE_DATA_MASK_ALL,
+                               on_clear_callback);
+}
+
+void ClearContentsData(content::WebContents* web_contents,
+                       const base::Closure& on_clear_callback) {
+  content::BrowserContext* context = web_contents->GetBrowserContext();
+  extensions::GuestViewManager* manager =
+      extensions::GuestViewManager::FromBrowserContext(context);
+  if (!manager->ForEachGuest(
+          web_contents, base::Bind(&ClearWebViewData, on_clear_callback))) {

[xiyuan, 2015/03/19 16:33:31]

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);
  }
...

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

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.

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

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

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

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]

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

+    on_clear_callback.Run();
+  }
+}
+
 }  // anonymous namespace
 
 // static
 bool ProfileHelper::enable_profile_to_user_testing = false;
 bool ProfileHelper::always_return_primary_user_for_testing = false;
 
 ////////////////////////////////////////////////////////////////////////////////
 // ProfileHelper, public
 
 ProfileHelper::ProfileHelper()
@@ skipping 129 matching lines @@
 }
 
 base::FilePath ProfileHelper::GetActiveUserProfileDir() {
   return ProfileHelper::GetUserProfileDir(active_user_id_hash_);
 }
 
 void ProfileHelper::Initialize() {
   user_manager::UserManager::Get()->AddSessionStateObserver(this);
 }
 
-void ProfileHelper::ClearSigninProfile(const base::Closure& on_clear_callback) {
-  on_clear_callbacks_.push_back(on_clear_callback);
+void ProfileHelper::ClearSigninProfile(const base::Closure& on_clear_callback,
+                                       content::WebContents* webview_contents) {
+  on_clear_callbacks_.push_back(
+      webview_contents
+          ? base::Bind(&ClearContentsData, webview_contents, on_clear_callback)
+          : on_clear_callback);
   if (signin_profile_clear_requested_)
     return;
   ProfileManager* profile_manager = g_browser_process->profile_manager();
   // Check if signin profile was loaded.
   if (!profile_manager->GetProfileByPath(GetSigninProfileDir())) {
     OnBrowsingDataRemoverDone();
     return;
   }
   signin_profile_clear_requested_ = true;
   BrowsingDataRemover* remover =
@@ skipping 131 matching lines @@
 void ProfileHelper::OnSessionRestoreStateChanged(
     Profile* user_profile,
     OAuth2LoginManager::SessionRestoreState state) {
   if (state == OAuth2LoginManager::SESSION_RESTORE_DONE ||
       state == OAuth2LoginManager::SESSION_RESTORE_FAILED ||
       state == OAuth2LoginManager::SESSION_RESTORE_CONNECTION_FAILED) {
     chromeos::OAuth2LoginManager* login_manager =
         chromeos::OAuth2LoginManagerFactory::GetInstance()->
             GetForProfile(user_profile);
     login_manager->RemoveObserver(this);
-    ClearSigninProfile(base::Closure());
+    ClearSigninProfile(base::Closure(), nullptr);

[xiyuan, 2015/03/19 16:33:31]

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]

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]

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

   }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // ProfileHelper, UserManager::UserSessionStateObserver implementation:
 
 void ProfileHelper::ActiveUserHashChanged(const std::string& hash) {
   active_user_id_hash_ = hash;
 }
 
@@ skipping 19 matching lines @@
   user_to_profile_for_testing_[user] = profile;
 }
 
 // static
 std::string ProfileHelper::GetUserIdHashByUserIdForTesting(
     const std::string& user_id) {
   return user_id + kUserIdHashSuffix;
 }
 
 }  // namespace chromeos