How well did we test that the recent changes to session serialization are backwards compatible?

What do we do if we try to decode session.plist and it contains unexpected data?  We throw the whole thing away and start with an empty TabModel, right?

I remember testing that with my changes I was able to restore tabs (but my tests were light, I just stopped Chrome with a bunch of tabs opens, made my changes and then launched it and checked that the tabs were still opens). I'll check whether this could be caused by the changes to TabModel.

This doesn't seem to be that widespread, so it could be related to keys that are only sometimes serialized, and I happened to get unlucky.

I don't know for sure that this is related to serialization, but I'm pretty confident that a decoding failure would look exactly like this.

I agree, incorrect de-serialization would likely lead to dropping the serialized session. Are you able to reproduce it? If yes, could you try to copy the saved session so that I can inspect them and see what could have broken them (I just remember that I did not really test in depth "tabID", "openerID", "openerNavigationIndex" or "lastVisitedTimestamp" changes).

I am able to reproduce this issue.
Upgrading from 3005 to any dogfoody build resulting in lost tabs.
Upgrading from 3012 to any above version is working fine.

Please let me know if you want me to provide any additional information.

I'm going to take a look. It's most likely caused by my recent changes or kkhorimoto.

Let's hold any pushes of 58 to Beta/TestFlight until this is resolved, hopefully this is resolved before that anyway.

Here is a small video to reproduce.
https://drive.google.com/file/d/0B-xmXLQhjeKuYkVWb1Zxd0Y1RFk/view

From the Video: First Installed 58.0.3005.0 canary
Performed a omnibox search (no scrolling here)
Background the app
Upgrade to 58.0.3012.0 canary

NTP is displayed. 

Also Noticed:
Fresh Install: 58.0.2991.0 canary and Upgrade directly to 58.0.3012.canary DOES NOT reproduce the issue.
Install 2991, then sequential upgrade to 2998, 3005, 3012 reproduces the issue.

Ok. I found what is the problem. Version 3005 uses a class name when serialising (CRWNavigationManagerStorage) that does not exists in previous version nor in more recent version (due to https://codereview.chromium.org/2687353003).

We have multiple options:
1. add an extra mapping for that case in session_service.mm,
2. do nothing.

I listed option 2. because if no publicly released version included that code (i.e. the code using the temporary class name), then there is no need to maintain compatibility with those saved session (as no user could have created them).

If we want to support them, then I have https://codereview.chromium.org/2731403002/ to implement option 1.

3005 is M58, so we need to do option 1. (good that I already have a CL ready for review).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8fb2aaae1f672153aac52e747c67a7c181dcc0a9

commit 8fb2aaae1f672153aac52e747c67a7c181dcc0a9
Author: sdefresne <sdefresne@chromium.org>
Date: Mon Mar 06 23:41:02 2017

Add a class alias in SessionWindowUnarchiver.

CL https://codereview.chromium.org/2687353003 renamed the class
CRWNavigationManagerStorage to CRWSessionStorage but didn't add
the mapping to SessionWindowUnarchiver causing the restoration
of saved session in branch 3005 to fail in later branches.

BUG= 698750 

Review-Url: https://codereview.chromium.org/2731403002
Cr-Commit-Position: refs/heads/master@{#455000}

[modify] https://crrev.com/8fb2aaae1f672153aac52e747c67a7c181dcc0a9/ios/chrome/browser/sessions/session_service.mm

Sorry for introducing this bug; I had changed the serialization classes within a pretty short duration and wasn't aware that the intermediary serialization class name was ever shipped in a non-dev version.

Please can we write a test to prevent this from happening in the future?

Branch 3005 will never be shipped to users, and moving from M57 to M58 then HEAD works, so removing the RBB tag and the M58 milestone.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/88d556703b1fbb2b53fa3d90c4938ee93ba3a8c1

commit 88d556703b1fbb2b53fa3d90c4938ee93ba3a8c1
Author: sdefresne <sdefresne@chromium.org>
Date: Tue Mar 07 21:02:09 2017

Add unittests checking that old saved session can be restored.

The session serialisation code has support for converting a
serialised session from an old format to the new one when a
class is renamed or a property is moved so add unittests to
check the conversion works properly by loading old sessions.

Clean up the code to use proper idioms (use leading underscore
for Objective-C ivar, use macro to mark designated initialiser,
remove redundant ivar backing synthesised property, ...).

BUG= 698750 

Review-Url: https://codereview.chromium.org/2732063003
Cr-Commit-Position: refs/heads/master@{#455217}

[modify] https://crrev.com/88d556703b1fbb2b53fa3d90c4938ee93ba3a8c1/ios/chrome/browser/sessions/BUILD.gn
[modify] https://crrev.com/88d556703b1fbb2b53fa3d90c4938ee93ba3a8c1/ios/chrome/browser/sessions/session_service.h
[modify] https://crrev.com/88d556703b1fbb2b53fa3d90c4938ee93ba3a8c1/ios/chrome/browser/sessions/session_service.mm
[modify] https://crrev.com/88d556703b1fbb2b53fa3d90c4938ee93ba3a8c1/ios/chrome/browser/sessions/session_service_unittest.mm
[modify] https://crrev.com/88d556703b1fbb2b53fa3d90c4938ee93ba3a8c1/ios/chrome/browser/tabs/tab_model_unittest.mm
[add] https://crrev.com/88d556703b1fbb2b53fa3d90c4938ee93ba3a8c1/ios/chrome/test/data/sessions/session_m57.plist
[add] https://crrev.com/88d556703b1fbb2b53fa3d90c4938ee93ba3a8c1/ios/chrome/test/data/sessions/session_m58.plist
[modify] https://crrev.com/88d556703b1fbb2b53fa3d90c4938ee93ba3a8c1/ios/web/navigation/serializable_user_data_manager_impl.mm

Upgraded from 58.0.3005.0 canary to 59.0.3047.0 canary with some open tabs.  All the tabs opened are displayed after upgrade.  Looks good.

Device: iPhone 6s plus iOS 10.2.1, iPad 4 iOS 10.3 beta 6