[ios] Gracefully handle missing WebState's data

Remove the check of whether all the WebState's data file are present
when loading the metadata for a session. This performed poorly for
users with thousands of tabs, and is not enough to ensure that the
data could be loaded (e.g. loading can fail even if the file exists
if the data is corrupt, or it could be removed by the time the code
tries to load it).

Change the prototype of WebStateStorageLoader to propagate failing
to load the WebStateStorage (by returning std::nullopt) and create
a default storage from the metadata if possible. This ensure that
the tab will at least navigate to the last visible URL even if the
navigation history was lost.

This should improve the startup time by removing disk access from
the hot path and should reduce the risk of losing tabs because it
will only happen if the metadata themselves are invalid, not if a
few tabs data has disappeared.

Bug: none
Change-Id: I3c088368039472209be9eb75c28b49c0dc9d34bb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6377364
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Auto-Submit: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1435626}

ios/chrome/browser/sessions/model/session_loading.mm

@@ -235,18 +235,6 @@ ios::proto::WebStateListStorage LoadSessionStorage(
     const auto web_state_id =
         web::WebStateID::FromSerializedValue(item.identifier());
 
-    const base::FilePath web_state_dir =
-        WebStateDirectory(directory, web_state_id);
-
-    const base::FilePath web_state_storage_file =
-        web_state_dir.Append(kWebStateStorageFilename);
-
-    // If the item storage does not exist, then the session has been
-    // corrupted; return an empty session.
-    if (!FileExists(web_state_storage_file)) {
-      return EmptyWebStateListStorage();
-    }
-
     // If the item would be empty, drop it before restoration.
     if (!item.metadata().navigation_item_count()) {
       items_to_drop.push_back(index);

ios/chrome/browser/sessions/model/session_loading_unittest.mm

@@ -385,8 +385,8 @@ TEST_F(SessionLoadingTest, LoadSessionStorage_MissingSession) {
       "Tabs.DroppedDuplicatesCountOnSessionRestore", 0);
 }
 
-// Tests that LoadSessionStorage returns an empty session if some of the
-// items data is missing.
+// Tests that LoadSessionStorage returns a valid session even if some
+// of the item storage are missing.
 TEST_F(SessionLoadingTest, LoadSessionStorage_MissingItemStorage) {
   base::ScopedTempDir scoped_temp_dir;
   ASSERT_TRUE(scoped_temp_dir.CreateUniqueTempDir());
@@ -398,11 +398,11 @@ TEST_F(SessionLoadingTest, LoadSessionStorage_MissingItemStorage) {
 
   // Delete one item storage.
   ASSERT_GT(session.items_size(), 0);
-  const web::WebStateID item_id =
+  const web::WebStateID deleted_item_id =
       web::WebStateID::FromSerializedValue(session.items(0).identifier());
 
   const base::FilePath item_metadata_path =
-      ios::sessions::WebStateDirectory(root, item_id)
+      ios::sessions::WebStateDirectory(root, deleted_item_id)
           .Append(kWebStateStorageFilename);
 
   ASSERT_TRUE(ios::sessions::DeleteRecursively(item_metadata_path));
@@ -410,13 +410,21 @@ TEST_F(SessionLoadingTest, LoadSessionStorage_MissingItemStorage) {
   // Load the session and check it is empty.
   const ios::proto::WebStateListStorage loaded =
       ios::sessions::LoadSessionStorage(root);
-  EXPECT_EQ(loaded, EmptySessionStorage());
-  EXPECT_EQ(loaded.items_size(), 0);
+  EXPECT_EQ(loaded, session);
 
-  // Expect no log as there was a missing item storage. We never got to complete
-  // the filtering stage.
-  histogram_tester_.ExpectTotalCount(
-      "Tabs.DroppedDuplicatesCountOnSessionRestore", 0);
+  for (const auto& item : loaded.items()) {
+    const web::WebStateID item_id =
+        web::WebStateID::FromSerializedValue(item.identifier());
+
+    // Check that the item has been correctly loaded.
+    ASSERT_TRUE(item.has_metadata());
+    ASSERT_EQ(item.metadata().active_page().page_url(),
+              TestUrlForIdentifier(item_id));
+  }
+
+  // Expect a log of 0 duplicate.
+  histogram_tester_.ExpectUniqueSample(
+      "Tabs.DroppedDuplicatesCountOnSessionRestore", 0, 1);
 }
 
 // Tests that LoadSessionStorage returns an empty session if the identifiers

ios/chrome/browser/sessions/model/session_restoration_service_impl.mm

@@ -33,6 +33,18 @@ namespace {
 // Maximum size of session state NSData objects.
 const int kMaxSessionState = 5 * 1024 * 1024;
 
+// Status of loading the WebStateStorage.
+enum class WebStateStorageStatus {
+  kSuccess,
+  kFailure,
+};
+
+// Records whether loading a WebStateStorage was a success or not.
+void RecordLoadingWebStateStorageStatus(WebStateStorageStatus status) {
+  base::UmaHistogramBoolean("Tabs.MissingWebStateStorageFileOnSessionRestore",
+                            status != WebStateStorageStatus::kSuccess);
+}
+
 // Deletes all files and directory in `path` not present in `items_to_keep`.
 void DeleteUnknownContent(const base::FilePath& path,
                           const std::set<base::FilePath>& items_to_keep) {
@@ -49,14 +61,23 @@ void DeleteUnknownContent(const base::FilePath& path,
   }
 }
 
-// Loads WebState storage from `web_state_dir` into `storage`.
-web::proto::WebStateStorage LoadWebStateStorage(const base::FilePath& path) {
+// Loads storage for WebState at `path`.
+std::optional<web::proto::WebStateStorage> LoadWebStateStorage(
+    const base::FilePath& path) {
   web::proto::WebStateStorage storage;
-  std::ignore = ios::sessions::ParseProto(path, storage);
-  return storage;
+  if (ios::sessions::ParseProto(path, storage)) {
+    RecordLoadingWebStateStorageStatus(WebStateStorageStatus::kSuccess);
+    return storage;
+  }
+
+  // Loading the file failed, inform the caller by returning nullopt.
+  // They can decide what to do (e.g. ignore the WebState, try to
+  // recover from metadata, ...).
+  RecordLoadingWebStateStorageStatus(WebStateStorageStatus::kFailure);
+  return std::nullopt;
 }
 
-// Loads Webstate native session from `web_state_dir`. It is okay if the file
+// Loads Webstate native session from `path`. It is okay if the file
 // is missing, in that case the function return `nil`.
 NSData* LoadWebStateSession(const base::FilePath& path) {
   return ios::sessions::ReadFile(path);
@@ -86,6 +107,16 @@ std::unique_ptr<web::WebState> CreateWebState(
   return web_state;
 }
 
+// Helper that invoke `callback` with `optional` value if not null or
+// does nothing otherwise.
+void AdaptWebStateStorageCallback(
+    base::OnceCallback<void(web::proto::WebStateStorage)> callback,
+    std::optional<web::proto::WebStateStorage> optional) {
+  if (optional.has_value()) {
+    std::move(callback).Run(std::move(optional).value());
+  }
+}
+
 // Delete data for discarded sessions with `identifiers` in `storage_path`
 // on a background thread.
 void DeleteDataForSessions(const base::FilePath& storage_path,
@@ -193,7 +224,12 @@ void IterateDataForSessionAtPath(const base::FilePath& session_dir,
         ios::sessions::WebStateDirectory(session_dir, web_state_id)
             .Append(kWebStateStorageFilename);
 
-    iterator.Run(web_state_id, LoadWebStateStorage(web_state_storage_path));
+    // Client code can't know before hand how many tabs exists, so
+    // it is okay to drop tabs that cannot be loaded.
+    if (std::optional<web::proto::WebStateStorage> storage =
+            LoadWebStateStorage(web_state_storage_path)) {
+      iterator.Run(web_state_id, std::move(storage).value());
+    }
   }
 }
 
@@ -506,10 +542,11 @@ void SessionRestorationServiceImpl::LoadWebStateStorage(
 
   // Post the task to read the data from disk. It will execute after all the
   // pending requests, so if the WebState has recently been created by calling
-  // CreateUnrealizedWebState(...), the data should be available.
+  // CreateUnrealizedWebState(...), the data should be available. If it is not
+  // then the callback is not called.
   task_runner_->PostTaskAndReplyWithResult(
       FROM_HERE, base::BindOnce(&::LoadWebStateStorage, storage_path),
-      std::move(callback));
+      base::BindOnce(&AdaptWebStateStorageCallback, std::move(callback)));
 }
 
 void SessionRestorationServiceImpl::AttachBackup(Browser* browser,
@@ -602,7 +639,7 @@ SessionRestorationServiceImpl::CreateUnrealizedWebState(
   // main thread while it is being written to disk on a background thread.
   return web::WebState::CreateWithStorage(
       browser->GetProfile(), web_state_id, std::move(metadata),
-      base::ReturnValueOnce(std::move(storage)),
+      base::ReturnValueOnce(std::make_optional(std::move(storage))),
       base::ReturnValueOnce<NSData*>(nil));
 }
 

ios/chrome/browser/sessions/model/test_session_restoration_service.mm

@@ -90,7 +90,7 @@ TestSessionRestorationService::CreateUnrealizedWebState(
 
   return web::WebState::CreateWithStorage(
       browser->GetProfile(), web::WebStateID::NewUnique(), std::move(metadata),
-      base::ReturnValueOnce(std::move(storage)),
+      base::ReturnValueOnce(std::make_optional(std::move(storage))),
       base::ReturnValueOnce<NSData*>(nil));
 }
 

ios/web/public/test/web_state_test_util.mm

@@ -195,7 +195,7 @@ std::unique_ptr<WebState> CreateUnrealizedWebStateWithItems(
 
   std::unique_ptr<WebState> web_state = WebState::CreateWithStorage(
       browser_state, WebStateID::NewUnique(), std::move(metadata),
-      base::ReturnValueOnce(std::move(storage)),
+      base::ReturnValueOnce(std::make_optional(std::move(storage))),
       base::ReturnValueOnce<NSData*>(nil));
 
   DCHECK(!web_state->IsRealized());

ios/web/public/web_state.h

@@ -72,7 +72,8 @@ class WebState : public base::SupportsUserData {
  public:
   // Callback used to load the full information for the WebState when
   // it will become realized.
-  using WebStateStorageLoader = base::OnceCallback<proto::WebStateStorage()>;
+  using WebStateStorageLoader =
+      base::OnceCallback<std::optional<proto::WebStateStorage>()>;
 
   // Callback used to fetch the native session for the WebState.
   using NativeSessionFetcher = base::OnceCallback<NSData*()>;

ios/web/web_state/web_state_impl.mm

@@ -67,7 +67,7 @@ void CheckForOverRealization() {
 }
 
 // Serializes the `session_storage` to proto::WebStateStorage.
-web::proto::WebStateStorage SessionStorageToProto(
+std::optional<web::proto::WebStateStorage> SessionStorageToProto(
     CRWSessionStorage* session_storage) {
   web::proto::WebStateStorage storage;
   [session_storage serializeToProto:storage];
@@ -548,15 +548,12 @@ WebState* WebStateImpl::ForceRealized() {
     // pass it to initialize the RealizedWebState).
     std::unique_ptr<SerializedData> saved = std::move(saved_);
 
-    // Load the storage from disk.
-    proto::WebStateStorage storage = saved->TakeStorageLoader().Run();
-
     // Perform the initialisation of the RealizedWebState. No outside
     // code should be able to observe the WebStateImpl with both `saved_`
     // and `pimpl_` set.
     pimpl_->InitWithProto(saved->GetBrowserState(), saved->GetLastActiveTime(),
                           saved->GetTitle(), saved->GetVisibleURL(),
-                          saved->GetFaviconStatus(), std::move(storage),
+                          saved->GetFaviconStatus(), saved->LoadStorage(),
                           saved->TakeNativeSessionFetcher());
 
     // Delete the SerializedData without calling TearDown() as the WebState

ios/web/web_state/web_state_impl_serialized_data.h

@@ -11,6 +11,7 @@
 
 namespace web {
 namespace proto {
+class WebStateStorage;
 class WebStateMetadataStorage;
 }  // namespace proto
 
@@ -57,8 +58,8 @@ class WebStateImpl::SerializedData {
   // Serializes the metadata to `storage`.
   void SerializeMetadataToProto(proto::WebStateMetadataStorage& storage) const;
 
-  // Returns the callback used to load the complete data from disk.
-  WebStateStorageLoader TakeStorageLoader();
+  // Loads the data from disk, or create a default one using the metadata.
+  proto::WebStateStorage LoadStorage();
 
   // Returns the callback used to fetch the native session data blob.
   NativeSessionFetcher TakeNativeSessionFetcher();

ios/web/web_state/web_state_impl_serialized_data.mm

@@ -6,9 +6,11 @@
 
 #import "base/strings/string_util.h"
 #import "base/strings/utf_string_conversions.h"
+#import "ios/web/public/navigation/navigation_util.h"
 #import "ios/web/public/navigation/web_state_policy_decider.h"
 #import "ios/web/public/session/proto/metadata.pb.h"
 #import "ios/web/public/session/proto/proto_util.h"
+#import "ios/web/public/session/proto/storage.pb.h"
 #import "ios/web/public/web_state_observer.h"
 
 namespace web {
@@ -78,9 +80,28 @@ void WebStateImpl::SerializedData::SerializeMetadataToProto(
   }
 }
 
-WebState::WebStateStorageLoader
-WebStateImpl::SerializedData::TakeStorageLoader() {
-  return std::move(storage_loader_);
+proto::WebStateStorage WebStateImpl::SerializedData::LoadStorage() {
+  if (std::optional<proto::WebStateStorage> storage =
+          std::move(storage_loader_).Run()) {
+    return std::move(storage).value();
+  }
+
+  // If the visible URL is valid but the data cannot be loade from disk,
+  // create a WebStateStorage as if it contained a single navigation to
+  // that URL. The full navigation history will be lost, but at least
+  // the tab won't be fully lost.
+  if (page_visible_url_.is_valid()) {
+    const bool created_with_opener = false;
+    return CreateWebStateStorage(
+        NavigationManager::WebLoadParams(page_visible_url_), page_title_,
+        created_with_opener, UserAgentType::AUTOMATIC, creation_time_);
+  }
+
+  // Return an empty WebStateStorage. This will leave the tab blank,
+  // which will be weird, but there is not much that can be done if
+  // the data cannot be loaded and the visible URL from the metadata
+  // is invalid.
+  return proto::WebStateStorage();
 }
 
 WebState::NativeSessionFetcher

ios/web/web_state/web_state_impl_unittest.mm

@@ -861,10 +861,10 @@ TEST_F(WebStateImplTest, UncommittedRestoreSessionOptimisedStorage) {
   active_page->set_page_title("Title");
   active_page->set_page_url(url.spec());
 
-  WebStateImpl web_state =
-      WebStateImpl(GetBrowserState(), web::WebStateID::NewUnique(), metadata,
-                   base::ReturnValueOnce(std::move(storage)),
-                   base::ReturnValueOnce<NSData*>(nil));
+  WebStateImpl web_state = WebStateImpl(
+      GetBrowserState(), web::WebStateID::NewUnique(), metadata,
+      base::ReturnValueOnce(std::make_optional(std::move(storage))),
+      base::ReturnValueOnce<NSData*>(nil));
 
   // Check that the title and url are correct.
   ASSERT_FALSE(web_state.IsRealized());
@@ -1177,10 +1177,10 @@ TEST_F(WebStateImplTest, SerializeMetadataToProto) {
   original_metadata.Swap(storage.mutable_metadata());
 
   // Create an unrealized WebState.
-  web::WebStateImpl web_state =
-      WebStateImpl(GetBrowserState(), WebStateID::NewUnique(),
-                   original_metadata, base::ReturnValueOnce(std::move(storage)),
-                   base::ReturnValueOnce<NSData*>(nil));
+  web::WebStateImpl web_state = WebStateImpl(
+      GetBrowserState(), WebStateID::NewUnique(), original_metadata,
+      base::ReturnValueOnce(std::make_optional(std::move(storage))),
+      base::ReturnValueOnce<NSData*>(nil));
 
   // Check that the metadata can be fetched from the unrealized WebState.
   {

ios/web_view/internal/cwv_web_view.mm

@@ -239,7 +239,7 @@ WEB_STATE_USER_DATA_KEY_IMPL(WebViewHolder)
     (web::BrowserState*)browserState {
   return web::WebState::CreateWithStorage(
       browserState, self.webStateID, _storage.metadata(),
-      base::ReturnValueOnce(std::move(_storage)),
+      base::ReturnValueOnce(std::make_optional(std::move(_storage))),
       base::ReturnValueOnce<NSData*>(nil));
 }
 

tools/metrics/histograms/metadata/tab/histograms.xml

@@ -2083,6 +2083,16 @@ chromium-metrics-reviews@google.com.
   <token key="BatteryState" variants="BatteryState"/>
 </histogram>
 
+<histogram name="Tabs.MissingWebStateStorageFileOnSessionRestore"
+    enum="Boolean" expires_after="2026-03-20">
+  <owner>justincohen@google.com</owner>
+  <owner>bling-fundamentals@google.com</owner>
+  <summary>
+    Records whether a web state session storage file is missing on session
+    restore. Recorded when the web state is realized.
+  </summary>
+</histogram>
+
 <histogram name="Tabs.NewNavigationWithSameOriginTab" enum="Boolean"
     expires_after="2024-02-11">
   <owner>gjc@google.com</owner>