Don't use request user data during one-click sign in

chrome/browser/ui/sync/one_click_signin_helper.cc

Index: chrome/browser/ui/sync/one_click_signin_helper.cc
diff --git a/chrome/browser/ui/sync/one_click_signin_helper.cc b/chrome/browser/ui/sync/one_click_signin_helper.cc
index 94b91165afb33be5e03ecd6b9d9a8ef18a73a1f7..37d1d570403a50f9de207bb6ec49348cc72a8260 100644
--- a/chrome/browser/ui/sync/one_click_signin_helper.cc
+++ b/chrome/browser/ui/sync/one_click_signin_helper.cc
@@ -181,54 +181,6 @@ bool IsValidGaiaSigninRedirectOrResponseURL(const GURL& url) {
   return false;
 }
 
-// This class is associated as user data with a given URLRequest object, in
-// order to pass information from one response to another during the process
-// of signing the user into their Gaia account.  This class is only meant
-// to be used from the IO thread.
-class OneClickSigninRequestUserData : public base::SupportsUserData::Data {
- public:
-  const std::string& email() const { return email_; }
-
-  // Associates signin information with the request.  Overwrites existing
-  // information if any.
-  static void AssociateWithRequest(base::SupportsUserData* request,
-                                   const std::string& email);
-
-  // Gets the one-click sign in information associated with the request.
-  static OneClickSigninRequestUserData* FromRequest(
-      base::SupportsUserData* request);
-
- private:
-  // Key used when setting this object on the request.
-  static const void* const kUserDataKey;
-
-  explicit OneClickSigninRequestUserData(const std::string& email)
-      : email_(email) {
-  }
-
-  std::string email_;
-
-  DISALLOW_COPY_AND_ASSIGN(OneClickSigninRequestUserData);
-};
-
-// static
-void OneClickSigninRequestUserData::AssociateWithRequest(
-    base::SupportsUserData* request,
-    const std::string& email) {
-  request->SetUserData(kUserDataKey, new OneClickSigninRequestUserData(email));
-}
-
-// static
-OneClickSigninRequestUserData* OneClickSigninRequestUserData::FromRequest(
-    base::SupportsUserData* request) {
-  return static_cast<OneClickSigninRequestUserData*>(
-      request->GetUserData(kUserDataKey));
-}
-
-const void* const OneClickSigninRequestUserData::kUserDataKey =
-    static_cast<const void* const>(
-        &OneClickSigninRequestUserData::kUserDataKey);
-
 }  // namespace
 
 // The infobar asking the user if they want to use one-click sign in.
@@ -409,13 +361,6 @@ OneClickSigninHelper::~OneClickSigninHelper() {
 }
 
 // static
-void OneClickSigninHelper::AssociateWithRequestForTesting(
-    base::SupportsUserData* request,
-    const std::string& email) {
-  OneClickSigninRequestUserData::AssociateWithRequest(request, email);
-}
-
-// static
 bool OneClickSigninHelper::CanOffer(content::WebContents* web_contents,
                                     CanOfferFor can_offer_for,
                                     const std::string& email,
@@ -564,10 +509,10 @@ OneClickSigninHelper::Offer OneClickSigninHelper::CanOfferOnIOThreadImpl(
   // that we want to connect the profile, chrome needs to tell Gaia that
   // it should offer the interstitial.  Therefore missing one click data on
   // the request means can offer is true.
-  OneClickSigninRequestUserData* one_click_data =
-      OneClickSigninRequestUserData::FromRequest(request);
-  if (one_click_data) {
-    if (!SigninManager::IsAllowedUsername(one_click_data->email(),
+  std::string pending_email =
+      io_data->reverse_autologin_pending_email()->GetValue();
+  if (!pending_email.empty()) {
+    if (!SigninManager::IsAllowedUsername(pending_email,
             io_data->google_services_username_pattern()->GetValue())) {
       return DONT_OFFER;
     }
@@ -576,12 +521,12 @@ OneClickSigninHelper::Offer OneClickSigninHelper::CanOfferOnIOThreadImpl(
         io_data->one_click_signin_rejected_email_list()->GetValue();
     if (std::count_if(rejected_emails.begin(), rejected_emails.end(),
                       std::bind2nd(std::equal_to<std::string>(),
-                                   one_click_data->email())) > 0) {
+                                   pending_email)) > 0) {
       return DONT_OFFER;
     }
 
     if (io_data->signin_names()->GetEmails().count(
-            UTF8ToUTF16(one_click_data->email())) > 0) {
+            UTF8ToUTF16(pending_email)) > 0) {
       return DONT_OFFER;
     }
   }
@@ -647,12 +592,6 @@ void OneClickSigninHelper::ShowInfoBarIfPossible(net::URLRequest* request,
     }
   }
 
-  // Later in the chain of this request, we'll need to check the email address
-  // in the IO thread (see CanOfferOnIOThread).  So save the email address as
-  // user data on the request (only for web-based flow).
-  if (SyncPromoUI::UseWebBasedSigninFlow() && !email.empty())
-    OneClickSigninRequestUserData::AssociateWithRequest(request, email);
-
   if (!email.empty() || !session_index.empty()) {
     VLOG(1) << "OneClickSigninHelper::ShowInfoBarIfPossible:"
             << " email=" << email
@@ -730,6 +669,24 @@ void OneClickSigninHelper::ShowInfoBarUIThread(
 
   int error_message_id = 0;
 
+  // Save the email in the one-click signin manager.  The manager may
+  // not exist if the contents is incognito or if the profile is already
+  // connected to a Google account.
+  if (!session_index.empty())
+    helper->session_index_ = session_index;
+
+  if (!email.empty()) {
+    helper->email_ = email;
+    Profile* profile =
+        Profile::FromBrowserContext(web_contents->GetBrowserContext());

[erikwright (departed), 2013/01/18 03:06:54]

This is a bit of an abuse (perhaps harmless) of Preferences.

And in any case, it's not really necessary - presumably you can store this data
in ShowInfoBarIfPossible, in which case you save yourself all the trouble of
moving this data back and forth across threads (and cleaning it up afterwards).

[Roger Tawa OOO till Jul 10th, 2013/01/18 15:21:51]

Are you suggesting I use an std::string member directly in ProfileIOData, which
can be set in ShowInfoBarIfPossible() and read in CanOfferOnIOThreadImpl(),
since both occur on the IO thread?

However, I still need to be able to clear the value when the process is done,
see line 744 below, and this happens on the UI thread.  Maybe I can post to the
IO thread at line 744 to clear the value?

[erikwright (departed), 2013/01/18 15:36:28]

That's basically what I'm suggesting. Here are a few reasons:

1) There are other side effects of using preferences (i.e., this value will be
persisted if the browser quits/crashes during the process, no?) that it would be
best to avoid.
2) The cross-thread marshalling will be explicit, clear, and correlated with the
movement of the logic across threads.
3) I'm concerned about possible surprises - what if the tasks that are updating
the pref member and the tasks that are reading it don't get interleaved exactly
as you expect? You might end up reading a stale value.

Mostly, it just feels like it is unnecessary and a little abusive. #3 is
probably not a problem in practice.

So I'd suggest giving it a try and seeing what the impact on the code is.

[Roger Tawa OOO till Jul 10th, 2013/01/21 15:26:45]

Done.

+    profile->GetPrefs()->SetString(prefs::kReverseAutologinPendingEmail, email);
+  }
+
+  if (auto_accept != AUTO_ACCEPT_NONE) {
+    helper->auto_accept_ = auto_accept;
+    helper->source_ = source;
+  }
+
   CanOfferFor can_offer_for =
       (auto_accept != AUTO_ACCEPT_EXPLICIT &&
           helper->auto_accept_ != AUTO_ACCEPT_EXPLICIT) ?
@@ -744,20 +701,6 @@ void OneClickSigninHelper::ShowInfoBarUIThread(
     return;
   }
 
-  // Save the email in the one-click signin manager.  The manager may
-  // not exist if the contents is incognito or if the profile is already
-  // connected to a Google account.
-  if (!session_index.empty())
-    helper->session_index_ = session_index;
-
-  if (!email.empty())
-    helper->email_ = email;
-
-  if (auto_accept != AUTO_ACCEPT_NONE) {
-    helper->auto_accept_ = auto_accept;
-    helper->source_ = source;
-  }
-
   if (continue_url.is_valid()) {
     // When Gaia finally redirects to the continue URL, Gaia will add some
     // extra query parameters.  So ignore the parameters when checking to see
@@ -795,6 +738,10 @@ void OneClickSigninHelper::CleanTransientState() {
   auto_accept_ = AUTO_ACCEPT_NONE;
   source_ = SyncPromoUI::SOURCE_UNKNOWN;
   continue_url_ = GURL();
+
+  Profile* profile =
+      Profile::FromBrowserContext(web_contents()->GetBrowserContext());
+  profile->GetPrefs()->SetString(prefs::kReverseAutologinPendingEmail, "");
 }
 
 void OneClickSigninHelper::DidNavigateAnyFrame(