Move some Autofill processing logic on form submit to a background thread.

BUG=76992
TEST=Autofill continues to upload form data to the crowdsourcing server


Review URL: http://codereview.chromium.org/8387016

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@108162 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/autofill/autofill_manager.cc

@@ -11,6 +11,7 @@
 #include <set>
 #include <utility>
 
+#include "base/bind.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/string16.h"
@@ -44,6 +45,7 @@
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "content/browser/renderer_host/render_view_host.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
 #include "googleurl/src/gurl.h"
@@ -127,6 +129,40 @@ bool FormIsHTTPS(const FormStructure& form) {
   return form.source_url().SchemeIs(chrome::kHttpsScheme);
 }
 
+// Uses the existing personal data in |profiles| and |credit_cards| to determine
+// possible field types for the |submitted_form|.  This is potentially
+// expensive -- on the order of 50ms even for a small set of |stored_data|.
+// Hence, it should not run on the UI thread -- to avoid locking up the UI --
+// nor on the IO thread -- to avoid blocking IPC calls.
+void DeterminePossibleFieldTypesForUpload(
+    const std::vector<AutofillProfile>& profiles,
+    const std::vector<CreditCard>& credit_cards,
+    FormStructure* submitted_form) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
+  // For each field in the |submitted_form|, extract the value.  Then for each
+  // profile or credit card, identify any stored types that match the value.
+  for (size_t i = 0; i < submitted_form->field_count(); ++i) {
+    AutofillField* field = submitted_form->field(i);
+    string16 value = CollapseWhitespace(field->value, false);
+
+    FieldTypeSet matching_types;
+    for (std::vector<AutofillProfile>::const_iterator it = profiles.begin();
+         it != profiles.end(); ++it) {
+      it->GetMatchingTypes(value, &matching_types);
+    }
+    for (std::vector<CreditCard>::const_iterator it = credit_cards.begin();
+          it != credit_cards.end(); ++it) {
+      it->GetMatchingTypes(value, &matching_types);
+    }
+
+    if (matching_types.empty())
+      matching_types.insert(UNKNOWN_TYPE);
+
+    field->set_possible_types(matching_types);
+  }
+}
+
 // Check for unidentified forms among those with the most query or upload
 // requests.  If found, present an infobar prompting the user to send Google
 // Feedback identifying these forms.  Only executes if the appropriate flag is
@@ -219,8 +255,6 @@ AutofillManager::AutofillManager(TabContentsWrapper* tab_contents)
       user_did_autofill_(false),
       user_did_edit_autofilled_field_(false),
       external_delegate_(NULL) {
-  DCHECK(tab_contents);
-
   // |personal_data_| is NULL when using TestTabContents.
   personal_data_ = PersonalDataManagerFactory::GetForProfile(
       tab_contents->profile()->GetOriginalProfile());
@@ -282,53 +316,81 @@ bool AutofillManager::OnMessageReceived(const IPC::Message& message) {
   return handled;
 }
 
-void AutofillManager::OnFormSubmitted(const FormData& form,
+bool AutofillManager::OnFormSubmitted(const FormData& form,
                                       const TimeTicks& timestamp) {
   // Let AutoComplete know as well.
   tab_contents_wrapper_->autocomplete_history_manager()->OnFormSubmitted(form);
 
   if (!IsAutofillEnabled())
-    return;
+    return false;
 
   if (tab_contents()->browser_context()->IsOffTheRecord())
-    return;
+    return false;
 
   // Don't save data that was submitted through JavaScript.
   if (!form.user_submitted)
-    return;
+    return false;
 
   // Grab a copy of the form data.
-  FormStructure submitted_form(form);
+  scoped_ptr<FormStructure> submitted_form(new FormStructure(form));
 
   // Disregard forms that we wouldn't ever autofill in the first place.
-  if (!submitted_form.ShouldBeParsed(true))
-    return;
+  if (!submitted_form->ShouldBeParsed(true))
+    return false;
 
   // Ignore forms not present in our cache.  These are typically forms with
   // wonky JavaScript that also makes them not auto-fillable.
   FormStructure* cached_submitted_form;
   if (!FindCachedForm(form, &cached_submitted_form))
-    return;
-  submitted_form.UpdateFromCache(*cached_submitted_form);
+    return false;
+
+  submitted_form->UpdateFromCache(*cached_submitted_form);
+  if (submitted_form->IsAutofillable(true))
+    ImportFormData(*submitted_form);
 
   // Only upload server statistics and UMA metrics if at least some local data
   // is available to use as a baseline.
-  if (!personal_data_->profiles().empty() ||
-      !personal_data_->credit_cards().empty()) {
-    DeterminePossibleFieldTypesForUpload(&submitted_form);
-    submitted_form.LogQualityMetrics(*metric_logger_,
-                                     forms_loaded_timestamp_,
-                                     initial_interaction_timestamp_,
-                                     timestamp);
-
-    if (submitted_form.ShouldBeCrowdsourced())
-      UploadFormData(submitted_form);
-  }
+  const std::vector<AutofillProfile*>& profiles = personal_data_->profiles();
+  const std::vector<CreditCard*>& credit_cards = personal_data_->credit_cards();
+  if (!profiles.empty() || !credit_cards.empty()) {
+    // Copy the profile and credit card data, so that it can be accessed on a
+    // separate thread.
+    std::vector<AutofillProfile> copied_profiles;
+    copied_profiles.reserve(profiles.size());
+    for (std::vector<AutofillProfile*>::const_iterator it = profiles.begin();
+         it != profiles.end(); ++it) {
+      copied_profiles.push_back(**it);
+    }
 
-  if (!submitted_form.IsAutofillable(true))
-    return;
+    std::vector<CreditCard> copied_credit_cards;
+    copied_credit_cards.reserve(credit_cards.size());
+    for (std::vector<CreditCard*>::const_iterator it = credit_cards.begin();
+         it != credit_cards.end(); ++it) {
+      copied_credit_cards.push_back(**it);
+    }
 
-  ImportFormData(submitted_form);
+    // TODO(isherman): Ideally, we should not be using the FILE thread here.
+    // Per jar@, this is a good compromise for now, as we don't currently have a
+    // broad consensus on how to support such one-off background tasks.
+
+    // Note that ownership of |submitted_form| is passed to the second task,
+    // using |base::Owned|.
+    FormStructure* raw_submitted_form = submitted_form.get();
+    BrowserThread::PostTaskAndReply(
+        BrowserThread::FILE, FROM_HERE,
+        base::Bind(&DeterminePossibleFieldTypesForUpload,
+                   copied_profiles,
+                   copied_credit_cards,
+                   raw_submitted_form),
+        base::Bind(&AutofillManager::UploadFormDataAsyncCallback,
+                   this,
+                   base::Owned(submitted_form.release()),
+                   forms_loaded_timestamp_,
+                   initial_interaction_timestamp_,
+                   timestamp));
+  }
+
+  return true;
 }
 
 void AutofillManager::OnFormsSeen(const std::vector<FormData>& forms,
@@ -627,35 +689,6 @@ bool AutofillManager::IsAutofillEnabled() const {
   return profile->GetPrefs()->GetBoolean(prefs::kAutofillEnabled);
 }
 
-void AutofillManager::DeterminePossibleFieldTypesForUpload(
-    FormStructure* submitted_form) {
-  // Combine all the profiles and credit cards stored in |personal_data_| into
-  // one vector for ease of iteration.
-  const std::vector<AutofillProfile*>& profiles = personal_data_->profiles();
-  const std::vector<CreditCard*>& credit_cards = personal_data_->credit_cards();
-  std::vector<FormGroup*> stored_data;
-  stored_data.insert(stored_data.end(), profiles.begin(), profiles.end());
-  stored_data.insert(stored_data.end(), credit_cards.begin(),
-                     credit_cards.end());
-
-  // For each field in the |submitted_form|, extract the value.  Then for each
-  // profile or credit card, identify any stored types that match the value.
-  for (size_t i = 0; i < submitted_form->field_count(); i++) {
-    AutofillField* field = submitted_form->field(i);
-    string16 value = CollapseWhitespace(field->value, false);
-    FieldTypeSet matching_types;
-    for (std::vector<FormGroup*>::const_iterator it = stored_data.begin();
-         it != stored_data.end(); ++it) {
-      (*it)->GetMatchingTypes(value, &matching_types);
-    }
-
-    if (matching_types.empty())
-      matching_types.insert(UNKNOWN_TYPE);
-
-    field->set_possible_types(matching_types);
-  }
-}
-
 void AutofillManager::SendAutofillTypePredictions(
     const std::vector<FormStructure*>& forms) const {
   if (!CommandLine::ForCurrentProcess()->HasSwitch(
@@ -691,6 +724,24 @@ void AutofillManager::ImportFormData(const FormStructure& submitted_form) {
   }
 }
 
+// Note that |submitted_form| is passed as a pointer rather than as a reference
+// so that we can get memory management right across threads.  Note also that we
+// explicitly pass in all the time stamps of interest, as the cached ones might
+// get reset before this method executes.
+void AutofillManager::UploadFormDataAsyncCallback(
+    const FormStructure* submitted_form,
+    const TimeTicks& load_time,
+    const TimeTicks& interaction_time,
+    const TimeTicks& submission_time) {
+  submitted_form->LogQualityMetrics(*metric_logger_,
+                                    load_time,
+                                    interaction_time,
+                                    submission_time);
+
+  if (submitted_form->ShouldBeCrowdsourced())
+    UploadFormData(*submitted_form);
+}
+
 void AutofillManager::UploadFormData(const FormStructure& submitted_form) {
   if (disable_download_manager_requests_)
     return;

chrome/browser/autofill/autofill_manager.h

@@ -14,6 +14,7 @@
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
 #include "base/gtest_prod_util.h"
+#include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/scoped_vector.h"
 #include "base/string16.h"
@@ -47,10 +48,10 @@ struct FormField;
 // Manages saving and restoring the user's personal information entered into web
 // forms.
 class AutofillManager : public TabContentsObserver,
-                        public AutofillDownloadManager::Observer {
+                        public AutofillDownloadManager::Observer,
+                        public base::RefCounted<AutofillManager> {
  public:
   explicit AutofillManager(TabContentsWrapper* tab_contents);
-  virtual ~AutofillManager();
 
   // Registers our Enable/Disable Autofill pref.
   static void RegisterUserPrefs(PrefService* prefs);
@@ -65,6 +66,8 @@ class AutofillManager : public TabContentsObserver,
 
  protected:
   // Only test code should subclass AutofillManager.
+  friend class base::RefCounted<AutofillManager>;
+  virtual ~AutofillManager();
 
   // The string/int pair is composed of the guid string and variant index
   // respectively.  The variant index is an index into the multi-valued item
@@ -84,6 +87,14 @@ class AutofillManager : public TabContentsObserver,
   // Reset cache.
   void Reset();
 
+  // Logs quality metrics for the |submitted_form| and uploads the form data
+  // to the crowdsourcing server, if appropriate.
+  virtual void UploadFormDataAsyncCallback(
+      const FormStructure* submitted_form,
+      const base::TimeTicks& load_time,
+      const base::TimeTicks& interaction_time,
+      const base::TimeTicks& submission_time);
+
   // Maps GUIDs to and from IDs that are used to identify profiles and credit
   // cards sent to and from the renderer process.
   virtual int GUIDToID(const GUIDPair& guid) const;
@@ -110,6 +121,12 @@ class AutofillManager : public TabContentsObserver,
     return external_delegate_;
   }
 
+  // Processes the submitted |form|, saving any new Autofill data and uploading
+  // the possible field types for the submitted fields to the crowdsouring
+  // server.  Returns false if this form is not relevant for Autofill.
+  bool OnFormSubmitted(const webkit_glue::FormData& form,
+                       const base::TimeTicks& timestamp);
+
  private:
   // TabContentsObserver:
   virtual void DidNavigateMainFramePostCommit(
@@ -121,8 +138,6 @@ class AutofillManager : public TabContentsObserver,
   virtual void OnLoadedServerPredictions(
       const std::string& response_xml) OVERRIDE;
 
-  void OnFormSubmitted(const webkit_glue::FormData& form,
-                       const base::TimeTicks& timestamp);
   void OnFormsSeen(const std::vector<webkit_glue::FormData>& forms,
                    const base::TimeTicks& timestamp);
   void OnTextFieldDidChange(const webkit_glue::FormData& form,
@@ -223,10 +238,6 @@ class AutofillManager : public TabContentsObserver,
   // Parses the forms using heuristic matching and querying the Autofill server.
   void ParseForms(const std::vector<webkit_glue::FormData>& forms);
 
-  // Uses existing personal data to determine possible field types for the
-  // |submitted_form|.
-  void DeterminePossibleFieldTypesForUpload(FormStructure* submitted_form);
-
   // Imports the form data, submitted by the user, into |personal_data_|.
   void ImportFormData(const FormStructure& submitted_form);
 

chrome/browser/autofill/autofill_manager_unittest.cc

@@ -411,7 +411,9 @@ class TestAutofillManager : public AutofillManager {
                       TestPersonalDataManager* personal_data)
       : AutofillManager(tab_contents, personal_data),
         personal_data_(personal_data),
-        autofill_enabled_(true) {
+        autofill_enabled_(true),
+        did_finish_async_form_submit_(false),
+        message_loop_is_running_(false) {
   }
 
   virtual bool IsAutofillEnabled() const OVERRIDE { return autofill_enabled_; }
@@ -420,6 +422,62 @@ class TestAutofillManager : public AutofillManager {
     autofill_enabled_ = autofill_enabled;
   }
 
+  void set_expected_submitted_field_types(
+      const std::vector<FieldTypeSet>& expected_types) {
+    expected_submitted_field_types_ = expected_types;
+  }
+
+  virtual void UploadFormDataAsyncCallback(
+      const FormStructure* submitted_form,
+      const base::TimeTicks& load_time,
+      const base::TimeTicks& interaction_time,
+      const base::TimeTicks& submission_time) OVERRIDE {
+    if (message_loop_is_running_) {
+      MessageLoop::current()->Quit();
+      message_loop_is_running_ = false;
+    } else {
+      did_finish_async_form_submit_ = true;
+    }
+
+    // If we have expected field types set, make sure they match.
+    if (!expected_submitted_field_types_.empty()) {
+      ASSERT_EQ(expected_submitted_field_types_.size(),
+                submitted_form->field_count());
+      for (size_t i = 0; i < expected_submitted_field_types_.size(); ++i) {
+        SCOPED_TRACE(
+            StringPrintf("Field %d with value %s", static_cast<int>(i),
+                         UTF16ToUTF8(submitted_form->field(i)->value).c_str()));
+        const FieldTypeSet& possible_types =
+            submitted_form->field(i)->possible_types();
+        EXPECT_EQ(expected_submitted_field_types_[i].size(),
+                  possible_types.size());
+        for (FieldTypeSet::const_iterator it =
+                 expected_submitted_field_types_[i].begin();
+             it != expected_submitted_field_types_[i].end(); ++it) {
+          EXPECT_TRUE(possible_types.count(*it))
+              << "Expected type: " << AutofillType::FieldTypeToString(*it);
+        }
+      }
+    }
+
+    AutofillManager::UploadFormDataAsyncCallback(submitted_form,
+                                                 load_time,
+                                                 interaction_time,
+                                                 submission_time);
+  }
+
+  // Wait for the asynchronous OnFormSubmitted() call to complete.
+  void WaitForAsyncFormSubmit() {
+    if (!did_finish_async_form_submit_) {
+      // TODO(isherman): It seems silly to need this variable.  Is there some
+      // way I can just query the message loop's state?
+      message_loop_is_running_ = true;
+      MessageLoop::current()->Run();
+    } else {
+      did_finish_async_form_submit_ = false;
+    }
+  }
+
   virtual void UploadFormData(const FormStructure& submitted_form) OVERRIDE {
     submitted_form_signature_ = submitted_form.FormSignature();
   }
@@ -452,10 +510,19 @@ class TestAutofillManager : public AutofillManager {
   }
 
  private:
+  // AutofillManager is ref counted.
+  virtual ~TestAutofillManager() {}
+
   // Weak reference.
   TestPersonalDataManager* personal_data_;
+
   bool autofill_enabled_;
+
+  bool did_finish_async_form_submit_;
+  bool message_loop_is_running_;
+
   std::string submitted_form_signature_;
+  std::vector<FieldTypeSet> expected_submitted_field_types_;
 
   DISALLOW_COPY_AND_ASSIGN(TestAutofillManager);
 };
@@ -468,24 +535,32 @@ class AutofillManagerTest : public TabContentsWrapperTestHarness {
 
   AutofillManagerTest()
       : TabContentsWrapperTestHarness(),
-        browser_thread_(BrowserThread::UI, &message_loop_) {
+        ui_thread_(BrowserThread::UI, &message_loop_),
+        file_thread_(BrowserThread::FILE) {
   }
 
   virtual ~AutofillManagerTest() {
     // Order of destruction is important as AutofillManager relies on
     // PersonalDataManager to be around when it gets destroyed.
-    autofill_manager_.reset(NULL);
+    autofill_manager_ = NULL;
   }
 
-  virtual void SetUp() {
+  virtual void SetUp() OVERRIDE {
     Profile* profile = new TestingProfile();
     browser_context_.reset(profile);
     PersonalDataManagerFactory::GetInstance()->SetTestingFactory(
         profile, TestPersonalDataManager::Build);
 
     TabContentsWrapperTestHarness::SetUp();
-    autofill_manager_.reset(new TestAutofillManager(contents_wrapper(),
-                                                    &personal_data_));
+    autofill_manager_ = new TestAutofillManager(contents_wrapper(),
+                                                &personal_data_);
+
+    file_thread_.Start();
+  }
+
+  virtual void TearDown() OVERRIDE {
+    file_thread_.Stop();
+    TabContentsWrapperTestHarness::TearDown();
   }
 
   void GetAutofillSuggestions(int query_id,
@@ -509,7 +584,8 @@ class AutofillManagerTest : public TabContentsWrapperTestHarness {
   }
 
   void FormSubmitted(const FormData& form) {
-    autofill_manager_->OnFormSubmitted(form, base::TimeTicks::Now());
+    if (autofill_manager_->OnFormSubmitted(form, base::TimeTicks::Now()))
+      autofill_manager_->WaitForAsyncFormSubmit();
   }
 
   void FillAutofillFormData(int query_id,
@@ -570,9 +646,10 @@ class AutofillManagerTest : public TabContentsWrapperTestHarness {
   }
 
  protected:
-  content::TestBrowserThread browser_thread_;
+  content::TestBrowserThread ui_thread_;
+  content::TestBrowserThread file_thread_;
 
-  scoped_ptr<TestAutofillManager> autofill_manager_;
+  scoped_refptr<TestAutofillManager> autofill_manager_;
   TestPersonalDataManager personal_data_;
 
  private:
@@ -2773,70 +2850,8 @@ TEST_F(AutofillManagerTest, DeterminePossibleFieldTypesForUpload) {
   form.fields.push_back(field);
   expected_types.push_back(types);
 
-  FormStructure form_structure(form);
-  autofill_manager_->DeterminePossibleFieldTypesForUpload(&form_structure);
-
-  ASSERT_EQ(expected_types.size(), form_structure.field_count());
-  for (size_t i = 0; i < expected_types.size(); ++i) {
-    SCOPED_TRACE(
-        StringPrintf("Field %d with value %s", static_cast<int>(i),
-                     UTF16ToUTF8(form_structure.field(i)->value).c_str()));
-    const FieldTypeSet& possible_types =
-        form_structure.field(i)->possible_types();
-    EXPECT_EQ(expected_types[i].size(), possible_types.size());
-    for (FieldTypeSet::const_iterator it = expected_types[i].begin();
-         it != expected_types[i].end(); ++it) {
-      EXPECT_TRUE(possible_types.count(*it))
-          << "Expected type: " << AutofillType::FieldTypeToString(*it);
-    }
-  }
-}
-
-TEST_F(AutofillManagerTest, DeterminePossibleFieldTypesForUploadStressTest) {
-  personal_data_.ClearAutofillProfiles();
-  const int kNumProfiles = 5;
-  for (int i = 0; i < kNumProfiles; ++i) {
-    AutofillProfile* profile = new AutofillProfile;
-    autofill_test::SetProfileInfo(profile,
-                                  StringPrintf("John%d", i).c_str(),
-                                  "",
-                                  StringPrintf("Doe%d", i).c_str(),
-                                  StringPrintf("JohnDoe%d@somesite.com",
-                                               i).c_str(),
-                                  "",
-                                  StringPrintf("%d 1st st.", i).c_str(),
-                                  "",
-                                  "Memphis", "Tennessee", "38116", "USA",
-                                  StringPrintf("650234%04d", i).c_str());
-    profile->set_guid(
-        StringPrintf("00000000-0000-0000-0001-00000000%04d", i).c_str());
-    personal_data_.AddProfile(profile);
-  }
-  FormData form;
-  CreateTestAddressFormData(&form);
-  ASSERT_LT(3U, form.fields.size());
-  form.fields[0].value = ASCIIToUTF16("6502340001");
-  form.fields[1].value = ASCIIToUTF16("John1");
-  form.fields[2].value = ASCIIToUTF16("12345");
-  FormStructure form_structure(form);
-  autofill_manager_->DeterminePossibleFieldTypesForUpload(&form_structure);
-  ASSERT_LT(3U, form_structure.field_count());
-  const FieldTypeSet& possible_types0 =
-      form_structure.field(0)->possible_types();
-  EXPECT_EQ(2U, possible_types0.size());
-  EXPECT_TRUE(possible_types0.find(PHONE_HOME_WHOLE_NUMBER) !=
-              possible_types0.end());
-  EXPECT_TRUE(possible_types0.find(PHONE_HOME_CITY_AND_NUMBER) !=
-              possible_types0.end());
-  const FieldTypeSet& possible_types1 =
-      form_structure.field(1)->possible_types();
-  EXPECT_EQ(1U, possible_types1.size());
-  EXPECT_TRUE(possible_types1.find(NAME_FIRST) != possible_types1.end());
-  const FieldTypeSet& possible_types2 =
-      form_structure.field(2)->possible_types();
-  EXPECT_EQ(1U, possible_types2.size());
-  EXPECT_TRUE(possible_types2.find(UNKNOWN_TYPE) !=
-              possible_types2.end());
+  autofill_manager_->set_expected_submitted_field_types(expected_types);
+  FormSubmitted(form);
 }
 
 namespace {

chrome/browser/ui/tab_contents/tab_contents_wrapper.cc

@@ -247,7 +247,7 @@ TabContentsWrapper::TabContentsWrapper(TabContents* contents)
 
   // Create the tab helpers.
   autocomplete_history_manager_.reset(new AutocompleteHistoryManager(contents));
-  autofill_manager_.reset(new AutofillManager(this));
+  autofill_manager_ = new AutofillManager(this);
   if (CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kExternalAutofillPopup)) {
     autofill_external_delegate_.reset(

chrome/browser/ui/tab_contents/tab_contents_wrapper.h

@@ -11,6 +11,7 @@
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
+#include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "chrome/browser/prefs/pref_change_registrar.h"
 #include "chrome/browser/ui/tab_contents/tab_contents_wrapper_synced_tab_delegate.h"
@@ -274,7 +275,7 @@ class TabContentsWrapper : public TabContentsObserver,
   // "Tab Helpers" section in the member functions area, above.)
 
   scoped_ptr<AutocompleteHistoryManager> autocomplete_history_manager_;
-  scoped_ptr<AutofillManager> autofill_manager_;
+  scoped_refptr<AutofillManager> autofill_manager_;
   scoped_ptr<AutofillExternalDelegate> autofill_external_delegate_;
   scoped_ptr<AutomationTabHelper> automation_tab_helper_;
   scoped_ptr<BlockedContentTabHelper> blocked_content_tab_helper_;