Fixes bug where first login attempt to hotmail after session restore

wouldn't work. The problem was we weren't storing post URLs at all
(because of bug 1361980). This resulted in session restore trying to
restore some redirects along the way that likely had bogus state in
them (perhaps referencing cookies that were nuked), resulting in the
first login failing. The fix is to persist POST URLs to disk for
session restore, but not the actual POST data. This way there
shouldn't be any problems as outlined in 1361980.

BUG=7727
TEST=see bug

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

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

chrome/browser/sessions/base_session_service.cc

@@ -12,6 +12,7 @@
 #include "chrome/browser/sessions/session_types.h"
 #include "chrome/browser/tab_contents/navigation_entry.h"
 #include "chrome/common/stl_util-inl.h"
+#include "webkit/glue/webkit_glue.h"
 
 // InternalGetCommandsRequest -------------------------------------------------
 
@@ -160,8 +161,14 @@ SessionCommand* BaseSessionService::CreateUpdateTabNavigationCommand(
   WriteWStringToPickle(pickle, &bytes_written, max_state_size,
                        UTF16ToWideHack(entry.title()));
 
-  WriteStringToPickle(pickle, &bytes_written, max_state_size,
-                      entry.content_state());
+  if (entry.has_post_data()) {
+    // Remove the form data, it may contain sensitive information.
+    WriteStringToPickle(pickle, &bytes_written, max_state_size,
+        webkit_glue::RemoveFormDataFromHistoryState(entry.content_state()));
+  } else {
+    WriteStringToPickle(pickle, &bytes_written, max_state_size,
+                        entry.content_state());
+  }
 
   pickle.WriteInt(entry.transition_type());
   int type_mask = entry.has_post_data() ? TabNavigation::HAS_POST_DATA : 0;
@@ -209,16 +216,11 @@ bool BaseSessionService::RestoreUpdateTabNavigationCommand(
 }
 
 bool BaseSessionService::ShouldTrackEntry(const NavigationEntry& entry) {
-  // Don't track entries that have post data. Post data may contain passwords
-  // and other sensitive data users don't want stored to disk.
-  return entry.display_url().is_valid() && !entry.has_post_data();
+  return entry.display_url().is_valid();
 }
 
 bool BaseSessionService::ShouldTrackEntry(const TabNavigation& navigation) {
-  // Don't track entries that have post data. Post data may contain passwords
-  // and other sensitive data users don't want stored to disk.
-  return navigation.url().is_valid() &&
-         (navigation.type_mask() & TabNavigation::HAS_POST_DATA) == 0;
+  return navigation.url().is_valid();
 }
 
 BaseSessionService::Handle BaseSessionService::ScheduleGetLastSessionCommands(

chrome/browser/sessions/session_service_unittest.cc

@@ -110,14 +110,13 @@ TEST_F(SessionServiceTest, Basic) {
   helper_.AssertNavigationEquals(nav1, tab->navigations[0]);
 }
 
-// Creates a navigation entry with post data, saves it, and makes sure it does
-// not get restored.
-TEST_F(SessionServiceTest, PrunePostData1) {
+// Make sure we persist post entries.
+TEST_F(SessionServiceTest, PersistPostData) {
   SessionID tab_id;
   ASSERT_NE(window_id.id(), tab_id.id());
 
   TabNavigation nav1(0, GURL("http://google.com"), GURL(),
-                     ASCIIToUTF16("abc"), "def",
+                     ASCIIToUTF16("abc"), std::string(),
                      PageTransition::QUALIFIER_MASK);
   nav1.set_type_mask(TabNavigation::HAS_POST_DATA);
 
@@ -127,38 +126,7 @@ TEST_F(SessionServiceTest, PrunePostData1) {
   ScopedVector<SessionWindow> windows;
   ReadWindows(&(windows.get()));
 
-  ASSERT_EQ(0U, windows->size());
-}
-
-// Creates two navigation entries, one with post data one without. Restores
-// and verifies we get back only the entry with no post data.
-TEST_F(SessionServiceTest, PrunePostData2) {
-  SessionID tab_id;
-  ASSERT_NE(window_id.id(), tab_id.id());
-
-  TabNavigation nav1(0, GURL("http://google.com"),
-                     GURL("http://www.referrer.com"),
-                     ASCIIToUTF16("abc"), "def",
-                     PageTransition::QUALIFIER_MASK);
-  nav1.set_type_mask(TabNavigation::HAS_POST_DATA);
-  TabNavigation nav2(0, GURL("http://google2.com"), GURL(),
-                     ASCIIToUTF16("abc"), "def",
-                     PageTransition::QUALIFIER_MASK);
-
-  helper_.PrepareTabInWindow(window_id, tab_id, 0, true);
-  UpdateNavigation(window_id, tab_id, nav1, 0, true);
-  UpdateNavigation(window_id, tab_id, nav2, 1, false);
-
-  ScopedVector<SessionWindow> windows;
-  ReadWindows(&(windows.get()));
-
-  ASSERT_EQ(1U, windows->size());
-  ASSERT_EQ(0, windows[0]->selected_tab_index);
-
-  SessionTab* tab = windows[0]->tabs[0];
-  helper_.AssertTabEquals(window_id, tab_id, 0, 0, 1, *tab);
-
-  helper_.AssertNavigationEquals(nav2, tab->navigations[0]);
+  helper_.AssertSingleWindowWithSingleTab(windows.get(), 1);
 }
 
 TEST_F(SessionServiceTest, ClosingTabStaysClosed) {

chrome/browser/sessions/tab_restore_service_unittest.cc

@@ -179,40 +179,12 @@ TEST_F(TabRestoreServiceTest, DontLoadRestoredTab) {
   ASSERT_EQ(0U, service_->entries().size());
 }
 
-// Make sure we don't persist entries to disk that have post data.
-TEST_F(TabRestoreServiceTest, DontPersistPostData1) {
+// Make sure we persist entries to disk that have post data.
+TEST_F(TabRestoreServiceTest, DontPersistPostData) {
   AddThreeNavigations();
-  controller_->GetEntryAtIndex(2)->set_has_post_data(true);
-
-  // Have the service record the tab.
-  service_->CreateHistoricalTab(controller_);
-  ASSERT_EQ(1U, service_->entries().size());
-
-  // Recreate the service and have it load the tabs.
-  RecreateService();
-
-  // One entry should be created.
-  ASSERT_EQ(1U, service_->entries().size());
-
-  // And verify the entry, the last navigation (url3_) should not have
-  // been written to disk as it contained post data.
-  TabRestoreService::Entry* entry = service_->entries().front();
-  ASSERT_EQ(TabRestoreService::TAB, entry->type);
-  TabRestoreService::Tab* tab = static_cast<TabRestoreService::Tab*>(entry);
-  ASSERT_EQ(2U, tab->navigations.size());
-  EXPECT_TRUE(url1_ == tab->navigations[0].url());
-  EXPECT_TRUE(url2_ == tab->navigations[1].url());
-  EXPECT_EQ(1, tab->current_navigation_index);
-}
-
-// Make sure we don't persist entries to disk that have post data. This
-// differs from DontPersistPostData1 in that all navigations before the
-// current index have post data.
-TEST_F(TabRestoreServiceTest, DontPersistPostData2) {
-  AddThreeNavigations();
-  NavigateToIndex(1);
   controller_->GetEntryAtIndex(0)->set_has_post_data(true);
   controller_->GetEntryAtIndex(1)->set_has_post_data(true);
+  controller_->GetEntryAtIndex(2)->set_has_post_data(true);
 
   // Have the service record the tab.
   service_->CreateHistoricalTab(controller_);
@@ -224,34 +196,13 @@ TEST_F(TabRestoreServiceTest, DontPersistPostData2) {
   // One entry should be created.
   ASSERT_EQ(1U, service_->entries().size());
 
-  // And verify the entry, the last navigation (url3_) should not have
-  // been written to disk as it contained post data.
-  TabRestoreService::Entry* entry = service_->entries().front();
-  ASSERT_EQ(TabRestoreService::TAB, entry->type);
-  TabRestoreService::Tab* tab = static_cast<TabRestoreService::Tab*>(entry);
-  ASSERT_EQ(1U, tab->navigations.size());
-  EXPECT_TRUE(url3_ == tab->navigations[0].url());
-  EXPECT_EQ(0, tab->current_navigation_index);
-}
-
-// Make sure we don't persist entries to disk that have post data. This
-// differs from DontPersistPostData1 in that all the navigations have post
-// data, so that nothing should be persisted.
-TEST_F(TabRestoreServiceTest, DontPersistPostData3) {
-  AddThreeNavigations();
-  controller_->GetEntryAtIndex(0)->set_has_post_data(true);
-  controller_->GetEntryAtIndex(1)->set_has_post_data(true);
-  controller_->GetEntryAtIndex(2)->set_has_post_data(true);
-
-  // Have the service record the tab.
-  service_->CreateHistoricalTab(controller_);
-  ASSERT_EQ(1U, service_->entries().size());
-
-  // Recreate the service and have it load the tabs.
-  RecreateService();
+  const TabRestoreService::Entry* restored_entry = service_->entries().front();
+  ASSERT_EQ(TabRestoreService::TAB, restored_entry->type);
 
-  // One entry should be created.
-  ASSERT_TRUE(service_->entries().empty());
+  const TabRestoreService::Tab* restored_tab =
+      static_cast<const TabRestoreService::Tab*>(restored_entry);
+  // There should be 3 navs.
+  ASSERT_EQ(3U, restored_tab->navigations.size());
 }
 
 // Make sure we don't persist entries to disk that have post data. This

webkit/glue/glue_serialize.cc

@@ -271,7 +271,8 @@ static void WriteHistoryItem(const HistoryItem* item, SerializeObject* obj) {
 
 // Creates a new HistoryItem tree based on the serialized string.
 // Assumes the data is in the format returned by WriteHistoryItem.
-static PassRefPtr<HistoryItem> ReadHistoryItem(const SerializeObject* obj) {
+static PassRefPtr<HistoryItem> ReadHistoryItem(const SerializeObject* obj,
+                                               bool include_form_data) {
   // See note in WriteHistoryItem. on this.
   obj->version = ReadInteger(obj);
 
@@ -299,20 +300,20 @@ static PassRefPtr<HistoryItem> ReadHistoryItem(const SerializeObject* obj) {
   item->setDocumentState(document_state);
 
   // Form data.  If there is any form data, we assume POST, otherwise GET.
-  // ResourceRequest takes ownership of the new FormData, then gives it to
-  // HistoryItem.
+  // FormData is ref counted.
   ResourceRequest dummy_request;  // only way to initialize HistoryItem
   dummy_request.setHTTPBody(ReadFormData(obj));
   dummy_request.setHTTPContentType(ReadString(obj));
   dummy_request.setHTTPReferrer(ReadString(obj));
   if (dummy_request.httpBody())
     dummy_request.setHTTPMethod("POST");
-  item->setFormInfoFromRequest(dummy_request);
+  if (include_form_data)
+    item->setFormInfoFromRequest(dummy_request);
 
   // Subitems
   int num_children = ReadInteger(obj);
   for (int i = 0; i < num_children; ++i)
-    item->addChildItem(ReadHistoryItem(obj));
+    item->addChildItem(ReadHistoryItem(obj, include_form_data));
 
   return item.release();
 }
@@ -332,15 +333,22 @@ void HistoryItemToString(PassRefPtr<HistoryItem> item,
 
 // Reconstruct a HistoryItem from a string, using our JSON Value deserializer.
 // This assumes that the given serialized string has all the required key,value
-// pairs, and does minimal error checking.
-PassRefPtr<HistoryItem> HistoryItemFromString(
-    const std::string& serialized_item) {
+// pairs, and does minimal error checking. If |include_form_data| is true,
+// the form data from a post is restored, otherwise the form data is empty.
+static PassRefPtr<HistoryItem> HistoryItemFromString(
+    const std::string& serialized_item,
+    bool include_form_data) {
   if (serialized_item.empty())
     return NULL;
 
   SerializeObject obj(serialized_item.data(),
                       static_cast<int>(serialized_item.length()));
-  return ReadHistoryItem(&obj);
+  return ReadHistoryItem(&obj, include_form_data);
+}
+
+PassRefPtr<HistoryItem> HistoryItemFromString(
+    const std::string& serialized_item) {
+  return HistoryItemFromString(serialized_item, true);
 }
 
 // For testing purposes only.
@@ -370,4 +378,15 @@ std::string CreateHistoryStateForURL(const GURL& url) {
   return data;
 }
 
+std::string RemoveFormDataFromHistoryState(const std::string& content_state) {
+  RefPtr<HistoryItem> history_item(HistoryItemFromString(content_state, false));
+  if (!history_item.get()) {
+    // Couldn't parse the string, return an empty string.
+    return std::string();
+  }
+  std::string new_state;
+  HistoryItemToString(history_item, &new_state);
+  return new_state;
+}
+
 }  // namespace webkit_glue

webkit/glue/webkit_glue.h

@@ -84,6 +84,9 @@ const std::string& GetUserAgent(const GURL& url);
 // if the saved state is empty.
 std::string CreateHistoryStateForURL(const GURL& url);
 
+// Removes any form data state from the history state string |content_state|.
+std::string RemoveFormDataFromHistoryState(const std::string& content_state);
+
 #ifndef NDEBUG
 // Checks various important objects to see if there are any in memory, and
 // calls AppendToLog with any leaked objects. Designed to be called on shutdown