commit 91cc4417bdc45c3ffb8cda691cd5e5540fbc865c
author Sylvain Defresne <sdefresne@chromium.org> Tue Aug 27 11:08:30 2024
committer Chromium LUCI CQ <chromium-scoped@luci-project-accounts.iam.gserviceaccount.com> Tue Aug 27 11:08:30 2024
tree 30f456dec17ca815ac5be7244d20da083ef4997a
parent b5bbf210660045532c811e3df275871de6ad8683

[ios] Avoid crashing if session data is corrupt

A few users have a corrupt session data (where itemStorages stores
NSString instead of CRWNavigationItemStorage) causing crash loop
when the data is loaded.

Instead of crashing, ignore the data if it is invalid (this will
result in a tab with no navigation, which should be pruned later
by the session loading code). This is better than crashing even
though it will lead to losing the corresponding tabs.

Fixed: 358616893
Change-Id: Ieb7fed5928866f75dcf3c8acfbb7d8c95f6625ec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5816590
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Federica Germinario <fedegermi@google.com>
Commit-Queue: Federica Germinario <fedegermi@google.com>
Auto-Submit: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1347278}

ios/web/navigation/crw_session_storage_unittest.mm

diff --git a/ios/web/navigation/crw_session_storage_unittest.mm b/ios/web/navigation/crw_session_storage_unittest.mm
index f1b4e216..46c31f5b 100644
--- a/ios/web/navigation/crw_session_storage_unittest.mm
+++ b/ios/web/navigation/crw_session_storage_unittest.mm
@@ -230,3 +230,14 @@
 
   EXPECT_FALSE([decoded.userData objectForKey:@"TabId"]);
 }
+
+// Tests that unarchiving CRWSessionStorage drops invalid itemStorages.
+// This is a test for the fix for https://crbug.com/358616893 (where a
+// couple of user have corrupt data on disk).
+TEST_F(CRWSessionStorageTest, TestWorkaroundForIssue_358616893) {
+  session_storage_.itemStorages = @[ @"Not a CRWNavigationItemStorage" ];
+
+  CRWSessionStorage* decoded =
+      DecodeSessionStorage(EncodeSessionStorage(session_storage_));
+  EXPECT_EQ(decoded.itemStorages.count, 0u);
+}

ios/web/session/crw_session_storage.mm

diff --git a/ios/web/session/crw_session_storage.mm b/ios/web/session/crw_session_storage.mm
index b60a77d..8ca219ff 100644
--- a/ios/web/session/crw_session_storage.mm
+++ b/ios/web/session/crw_session_storage.mm
@@ -152,8 +152,28 @@
           [decoder decodeIntForKey:kLastCommittedItemIndexDeprecatedKey];
     }
 
-    _itemStorages = [[NSMutableArray alloc]
-        initWithArray:[decoder decodeObjectForKey:kItemStoragesKey]];
+    // A few users are crashing because they have a corrupted session (where
+    // _itemStorages contains objects that are not CRWNavigationItemStorage).
+    // If this happens, consider that the session has no navigations instead.
+    // It will result in a tab with no navigation, which will be dropped. It
+    // is better than crashing when trying to convert the session to proto.
+    // See https://crbug.com/358616893 for details.
+    NSObject* itemStoragesObj = [decoder decodeObjectForKey:kItemStoragesKey];
+    if ([itemStoragesObj isKindOfClass:[NSArray class]]) {
+      NSArray* itemStorages =
+          base::apple::ObjCCastStrict<NSArray>(itemStoragesObj);
+      for (NSObject* item in itemStorages) {
+        if (![item isKindOfClass:[CRWNavigationItemStorage class]]) {
+          itemStorages = nil;
+          break;
+        }
+      }
+      _itemStorages =
+          [[NSMutableArray alloc] initWithArray:(itemStorages ?: @[])];
+    } else {
+      _itemStorages = [[NSMutableArray alloc] init];
+    }
+
     // Prior to M34, 0 was used as "no index" instead of -1; adjust for that.
     if (!_itemStorages.count)
       _lastCommittedItemIndex = -1;