Did some testing to try and get a better sense for what is going on. I think I may have found the disconnect between how Custom Tabs interact with Sync (which Ted had pointed out earlier actually, but I didn't quite grasp the implications of).

For reference, within Sync, I think we want the following behavior:
- When the tabbed window is open, track all navigations within tabs (and the set of all open tabs)
- When a custom tab is open, treat is as a separate window, and track changes within it. The tabbed window should be preserved.

This somewhat works. I installed the CustomTabTabClientExample and did some experiments, with the following results:

Experiment A
1. Launch Chrome, navigate to a page.
2. Launch the CustomTabTabClientExample. Navigate to a custom tab (without warmup/may load)

This appears to work as expected. The custom tab is in its own window (due to having its own TabModel), and the old tab from the main window is preserved. If you then open that custom tab in Chrome, Sync detects that as the second window closing, and a new tab opening within the main tabbed window. SGTM

Experiment B
1. Launch Chrome, navigate to a page, kill Chrome
2. Launch the CustomTabTabClientExample. Navigate to a custom tab (without warmup/may load)

Fail. Results in the custom tab window being the only open window. 

Experiment C.
1. Launch Chrome, navigate to a page, kill Chrome
2. Launch the CustomTabTabClientExample. Trigger warmup + may load, but don't actually open the custom tab

Success. No change in synced state. My patch appears to have helped with this case (testing with an older version resulted in all tabs disappearing).

So I think the thing that my previous patch missed (and what the previous fix for CustomTabs integration missed) is that you can open Chrome with just the CustomTab's TabModel. This will then completely confuse the Sync logic tracking tabs for this device (and is I think why sometimes old previously closed tabs stick around). We do in fact want to sync this custom tab, we just don't want to let it clobber or confuse our information for the main window.

The challenge here is in identifying which tabs from this session belong to that of previous sessions. SessionId's are static ints, and therefore can't be relied upon across restarts. The way we typically handle this is by relying on each tab to persist Syncs id for that tab. But this assumes that the TabState for all tabs within the tabbed window are restored. It appears this doesn't happen when Chrome is launched to open the custom tab.

Question for lizeb/tedchoc: is there any reason we don't always restore the TabState for the main window when we launch Chrome in a backgrounded mode? My understanding is that we don't actually have to load that tab into memory (i.e. webcontents can still be null), so hopefully this isn't too expensive memory-wise. If we could ensure that that tab state (namely the sync id of all previously open tabs) is always present, I think Sync would be able to both track changes to this new custom tab, while realizing that it's separate from all the old tabs and preserving them.

Nice testing zea!  I have some comments, though your question for lizeb & tedchoc remain.

Your testing reveals why sometimes another platform may not see any of the main window tabs on Android.  Does your understanding here also explain the other part of this bug report, that sometimes some tabs from the main window appear and others are missing (reported here by me most recently in comment #24)?

Also, you wrote:
> The challenge here is in identifying which tabs from this session belong to that
> of previous sessions. SessionId's are static ints, and therefore can't be relied
> upon across restarts. 
While this may be true for these SessionIds
https://cs.chromium.org/chromium/src/components/sessions/core/session_id.h
  // Uniquely identifies a tab or window for the duration of a session.
it's not true for these other SessionIds
https://cs.chromium.org/chromium/src/components/metrics/proto/chrome_user_metrics_extension.proto?&l=62
  // The session id is simply an integer that is incremented each time the user
  // relaunches Chrome.
I wonder if something more like the latter would be more useful here.

> Your testing reveals why sometimes another platform may not see any of the main window tabs on Android.  Does your understanding here also explain the other part of this bug report, that sometimes some tabs from the main window appear and others are missing (reported here by me most recently in comment #24)?

Good question. I believe it does, but in part due to an optimization we're doing within the SessionsSyncManager. Basically, the tab sync association process works like this
1. On startup:
- go through all windows
- for each window go through all tabs
- if the tab is a placeholder tab (no webcontents in memory), load its sync id and use that to mark the previously synced tab as still open
- if the tab is not a placeholder tab, go through and load its navigation state and write it into sync
- Update the header node with the set of open window ids and tab ids. If any tab that was previously open was not seen, remove it from the header node (we don't delete the tab entity itself though, as removing its reference from the header node is enough for UI purposes, and it's extra traffic).

2. Anytime a tab changes:
- Make sure the header node is up to date with the current set of tabs (rewriting window ids and tab ids)
- Go through the modified tab, load its navigation state, and write it into sync

Note that in case 2, we only load the modified tab. It's assumed that the window id and tab ids of all other tabs are accurate. As such, when you load Chrome after having opened a custom tab (assuming Chrome wasn't in memory before), you'll only be doing a full sync of any _new_ tabs. All old tabs that you haven't reloaded are assumed by sync to have already been synced properly. And in the meantime, the tab ids and window ids of all the previously open tabs will have changed, and we won't notice, and hence the other platforms will be looking for these new window/tab ids and will not be able to find them.

This also means that it's possible by chance a tab id will match an entity that already exists. This explains why it's possible previously closed tabs might suddenly appear as open.

In short, Sync is very reliant on doing a proper association at startup time, and once that's broken tabs can be either missing or stale. We could theoretically attempt to do a full association of all windows and all tabs every time _any_ tab is modified, but that could theoretically be a fair amount of processing if you have lots of tabs, and this is all happening synchronously on the UI thread, so we attempted to avoid that. If possible, I'd much prefer to just have the full tab state available reliably on startup.

Upping priority now that it's clear this can break tab sync fairly easily, and setting for M56.

@#26, Paraphrase: Why don't we deserialize all tabs on startup?

1.) Easiest answer is that we've never needed to, so we never did.

2.) You are correct that we do not need to create the WebContents for when deserializing, but we do end up pulling any blobs of web state data that was previously persisted.  Also, URLs can "theoretically" be large.  We also have to decrypt incognito files and other non-free things.  It is also a non-trivial amount of I/O on a constrained system.

3.) Right now, TabPersistentStore relies on a TabModelSelector, which has a tight coupling to the Activity.  In reality, we only recently added support for having tabs not associated with an Activity.  So even if we wanted to support this, it isn't currently possible without some refactoring.

--------

In general, Chrome on Android is really not set up to have all tabs available at any point.  We have the potential for multiple ChromeTabbedActivities due to multiwindow (whether on N or prior on Samsung).  CustomTab's lifetime is not something that we can easily control.  For example if a CustomTab is sitting on top of another app and you background it and it gets killed, if you start up normal Chrome we have no way to determine if the task is still there since we don't own the root task.  In this failure mode, more custom tabs will appear than not those.

But again, we don't deserialize all other custom tabs data when opening a new one because it is very much not going to be needed right now (has the potential to slow other things down, etc..., etc...).

I think relying on all tabs being available on startup is currently not feasible and would be a rather substantial undertaking to make it so.

Got it, thanks for clarifying Ted. Knowing that, and now knowing what is confusing Sync, I think I might have some ideas about how we might update the tab association logic to not rely on assumptions that are false in the face of custom tabs.

Basically, I think we can introduce the notion of window types into sync: persistent and transient. ChromeTabbedActivity will be a persistent type, and Custom Tabs (or anything else that doesn't save/restore state) will be a transient type. We can then store this value within Sync's session state, and ensure that persistent windows and their tabs are never discarded if only transient windows are available.

As long as we don't have to distinguish between two persistent windows (in other words as long as there's only one ChromeTabbedActivity) I think this should work out.

Disregard that last statement about needing there to be a single ChromeTabbedActivity, I see you call out that's not necessarily the case. I think that can still work, but will need to be care of how session restore works in that scenario.

And another point of clarification is that Custom Tabs do support saving and restoring state, but each Custom Tab only handles their own tab.

They are transient in the fact that if the user swipes them away we do not attempt to restore them (where ChromeTabbedActivity we will restore tabs).

Also for multiwindow, we merge the tabs back into a single ChromeTabbedActivity if you make one of them full screen (it is to prevent users from losing there tabs).  Just another thing to consider in the crazy world of tabs.

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

commit fb4ce10866412b0ecddb9e6b207fe643314f491f
Author: zea <zea@chromium.org>
Date: Thu Nov 10 03:20:41 2016

[Sync] Add support for identifying tabbed activites

Tabbed activites are different from non-tabbed activities in that they get
preserved on cold start start if no other tabbed activities are present,
as it's assumed they just haven't been loaded. Other activities, like
custom tabs, are not restored by sync.

This patch adds the plumbing for detecting if the activity is tabbed, and
persists that information within the sync window type. Windows are now
either tabbed, popup, or custom tabs.

BUG= 639009 

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

[modify] https://crrev.com/fb4ce10866412b0ecddb9e6b207fe643314f491f/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
[modify] https://crrev.com/fb4ce10866412b0ecddb9e6b207fe643314f491f/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/IncognitoTabModelImplCreator.java
[modify] https://crrev.com/fb4ce10866412b0ecddb9e6b207fe643314f491f/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java
[modify] https://crrev.com/fb4ce10866412b0ecddb9e6b207fe643314f491f/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelJniBridge.java
[modify] https://crrev.com/fb4ce10866412b0ecddb9e6b207fe643314f491f/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java
[modify] https://crrev.com/fb4ce10866412b0ecddb9e6b207fe643314f491f/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java
[modify] https://crrev.com/fb4ce10866412b0ecddb9e6b207fe643314f491f/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/DocumentTabModelImpl.java
[modify] https://crrev.com/fb4ce10866412b0ecddb9e6b207fe643314f491f/chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicyTest.java
[modify] https://crrev.com/fb4ce10866412b0ecddb9e6b207fe643314f491f/chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/ContextMenuLoadUrlParamsTest.java
[modify] https://crrev.com/fb4ce10866412b0ecddb9e6b207fe643314f491f/chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorObserverTestBase.java
[modify] https://crrev.com/fb4ce10866412b0ecddb9e6b207fe643314f491f/chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabPersistentStoreTest.java
[modify] https://crrev.com/fb4ce10866412b0ecddb9e6b207fe643314f491f/chrome/browser/sync/glue/synced_window_delegate_android.cc
[modify] https://crrev.com/fb4ce10866412b0ecddb9e6b207fe643314f491f/chrome/browser/sync/glue/synced_window_delegate_android.h
[modify] https://crrev.com/fb4ce10866412b0ecddb9e6b207fe643314f491f/chrome/browser/ui/android/tab_model/tab_model.cc
[modify] https://crrev.com/fb4ce10866412b0ecddb9e6b207fe643314f491f/chrome/browser/ui/android/tab_model/tab_model.h
[modify] https://crrev.com/fb4ce10866412b0ecddb9e6b207fe643314f491f/chrome/browser/ui/android/tab_model/tab_model_jni_bridge.cc
[modify] https://crrev.com/fb4ce10866412b0ecddb9e6b207fe643314f491f/chrome/browser/ui/android/tab_model/tab_model_jni_bridge.h
[modify] https://crrev.com/fb4ce10866412b0ecddb9e6b207fe643314f491f/chrome/browser/ui/android/tab_model/tab_model_list_unittest.cc
[modify] https://crrev.com/fb4ce10866412b0ecddb9e6b207fe643314f491f/chrome/browser/ui/android/tab_model/tab_model_unittest.cc
[modify] https://crrev.com/fb4ce10866412b0ecddb9e6b207fe643314f491f/components/sync/protocol/proto_enum_conversions.cc
[modify] https://crrev.com/fb4ce10866412b0ecddb9e6b207fe643314f491f/components/sync/protocol/session_specifics.proto
[modify] https://crrev.com/fb4ce10866412b0ecddb9e6b207fe643314f491f/components/sync_sessions/sessions_sync_manager.cc

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

commit 7453d79b8ab314577881145ed341e8603bfdbe4f
Author: zea <zea@chromium.org>
Date: Tue Nov 15 00:13:42 2016

[Sync] Put session tracker in charge of maintaining local state.

Previously, the session tracker was only updated when local tabs were
available in the TabModel. It was not updated with sync data from a previous
session (and as a result, we had to pass around the restored_data list
at association time in order to handle restoring placeholder tabs).

The session tracker now gets updated at startup of previous local state,
which is then used when reassociating restored tabs. This is a purely
refactoring change that should not introduce external behavioral changes
(although internally the data is managed in a different way).

To make this happen, InitFromSyncModel now treats local tabs and remote
tabs the same. In addition, the SyncedSessionTracker now has a
ReassociateTab method that can be called to update the tab id of a
SessionTab.

This is all necessary in order to enable preserving tabs from a previous
session when they're not present in the TabModel (which can happen on
Android when a custom tab is opened, but the main tabbed activity is not
loaded).

BUG= 639009 

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

[modify] https://crrev.com/7453d79b8ab314577881145ed341e8603bfdbe4f/chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc
[modify] https://crrev.com/7453d79b8ab314577881145ed341e8603bfdbe4f/components/sync_sessions/sessions_sync_manager.cc
[modify] https://crrev.com/7453d79b8ab314577881145ed341e8603bfdbe4f/components/sync_sessions/sessions_sync_manager.h
[modify] https://crrev.com/7453d79b8ab314577881145ed341e8603bfdbe4f/components/sync_sessions/synced_session_tracker.cc
[modify] https://crrev.com/7453d79b8ab314577881145ed341e8603bfdbe4f/components/sync_sessions/synced_session_tracker.h
[modify] https://crrev.com/7453d79b8ab314577881145ed341e8603bfdbe4f/components/sync_sessions/synced_session_tracker_unittest.cc

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

commit 1cd87c1fd8b685edcd9b053def59a5d3f7b26056
Author: zea <zea@chromium.org>
Date: Tue Nov 15 19:06:40 2016

Revert of [Sync] Put session tracker in charge of maintaining local state. (patchset #5 id:120001 of https://codereview.chromium.org/2494533002/ )

Reason for revert:
crbug.com/665314
crbug.com/665524

Original issue's description:
> [Sync] Put session tracker in charge of maintaining local state.
>
> Previously, the session tracker was only updated when local tabs were
> available in the TabModel. It was not updated with sync data from a previous
> session (and as a result, we had to pass around the restored_data list
> at association time in order to handle restoring placeholder tabs).
>
> The session tracker now gets updated at startup of previous local state,
> which is then used when reassociating restored tabs. This is a purely
> refactoring change that should not introduce external behavioral changes
> (although internally the data is managed in a different way).
>
> To make this happen, InitFromSyncModel now treats local tabs and remote
> tabs the same. In addition, the SyncedSessionTracker now has a
> ReassociateTab method that can be called to update the tab id of a
> SessionTab.
>
> This is all necessary in order to enable preserving tabs from a previous
> session when they're not present in the TabModel (which can happen on
> Android when a custom tab is opened, but the main tabbed activity is not
> loaded).
>
> BUG= 639009 
>
> Committed: https://crrev.com/7453d79b8ab314577881145ed341e8603bfdbe4f
> Cr-Commit-Position: refs/heads/master@{#432006}

TBR=maxbogue@chromium.org,skym@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 639009 

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

[modify] https://crrev.com/1cd87c1fd8b685edcd9b053def59a5d3f7b26056/chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc
[modify] https://crrev.com/1cd87c1fd8b685edcd9b053def59a5d3f7b26056/components/sync_sessions/sessions_sync_manager.cc
[modify] https://crrev.com/1cd87c1fd8b685edcd9b053def59a5d3f7b26056/components/sync_sessions/sessions_sync_manager.h
[modify] https://crrev.com/1cd87c1fd8b685edcd9b053def59a5d3f7b26056/components/sync_sessions/synced_session_tracker.cc
[modify] https://crrev.com/1cd87c1fd8b685edcd9b053def59a5d3f7b26056/components/sync_sessions/synced_session_tracker.h
[modify] https://crrev.com/1cd87c1fd8b685edcd9b053def59a5d3f7b26056/components/sync_sessions/synced_session_tracker_unittest.cc

Status update: I'm in the process of doing a significant refactor to how SessionSyncManager tracks tabs. It's going a bit slow, but I'm making progress.

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

commit 159246f269b561bf931fa56a35b2704fab7dcc96
Author: zea <zea@chromium.org>
Date: Wed Dec 07 00:40:34 2016

Reland of [Sync] Put session tracker in charge of maintaining local state.

Previously, the session tracker was only updated when local tabs were
available in the TabModel. It was not updated with sync data from a previous
session (and as a result, we had to pass around the restored_data list
at association time in order to handle restoring placeholder tabs).

The session tracker now gets updated at startup of previous local state,
which is then used when reassociating restored tabs. This is a purely
refactoring change that should not introduce user visible changes
(although internally the data is managed in a different way).

To make this happen, InitFromSyncModel now treats local tabs and remote
tabs the same. In addition, the SyncedSessionTracker now has a
ReassociateTab method that can be called to update the tab id of a
SessionTab. To make this work, the TabNodePool has been moved into
the SyncedSessionTracker, and in general data ownership has been
simplified (with quite a few new tests added).

This is all necessary in order to enable preserving tabs from a previous
session when they're not present in the TabModel (which can happen on
Android when a custom tab is opened, but the main tabbed activity is not
loaded).

BUG= 639009 

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

[modify] https://crrev.com/159246f269b561bf931fa56a35b2704fab7dcc96/chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc
[modify] https://crrev.com/159246f269b561bf931fa56a35b2704fab7dcc96/components/sync_sessions/sessions_sync_manager.cc
[modify] https://crrev.com/159246f269b561bf931fa56a35b2704fab7dcc96/components/sync_sessions/sessions_sync_manager.h
[modify] https://crrev.com/159246f269b561bf931fa56a35b2704fab7dcc96/components/sync_sessions/synced_session_tracker.cc
[modify] https://crrev.com/159246f269b561bf931fa56a35b2704fab7dcc96/components/sync_sessions/synced_session_tracker.h
[modify] https://crrev.com/159246f269b561bf931fa56a35b2704fab7dcc96/components/sync_sessions/synced_session_tracker_unittest.cc
[modify] https://crrev.com/159246f269b561bf931fa56a35b2704fab7dcc96/components/sync_sessions/tab_node_pool.cc
[modify] https://crrev.com/159246f269b561bf931fa56a35b2704fab7dcc96/components/sync_sessions/tab_node_pool.h
[modify] https://crrev.com/159246f269b561bf931fa56a35b2704fab7dcc96/components/sync_sessions/tab_node_pool_unittest.cc

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

commit 159246f269b561bf931fa56a35b2704fab7dcc96
Author: zea <zea@chromium.org>
Date: Wed Dec 07 00:40:34 2016

Reland of [Sync] Put session tracker in charge of maintaining local state.

Previously, the session tracker was only updated when local tabs were
available in the TabModel. It was not updated with sync data from a previous
session (and as a result, we had to pass around the restored_data list
at association time in order to handle restoring placeholder tabs).

The session tracker now gets updated at startup of previous local state,
which is then used when reassociating restored tabs. This is a purely
refactoring change that should not introduce user visible changes
(although internally the data is managed in a different way).

To make this happen, InitFromSyncModel now treats local tabs and remote
tabs the same. In addition, the SyncedSessionTracker now has a
ReassociateTab method that can be called to update the tab id of a
SessionTab. To make this work, the TabNodePool has been moved into
the SyncedSessionTracker, and in general data ownership has been
simplified (with quite a few new tests added).

This is all necessary in order to enable preserving tabs from a previous
session when they're not present in the TabModel (which can happen on
Android when a custom tab is opened, but the main tabbed activity is not
loaded).

BUG= 639009 

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

[modify] https://crrev.com/159246f269b561bf931fa56a35b2704fab7dcc96/chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc
[modify] https://crrev.com/159246f269b561bf931fa56a35b2704fab7dcc96/components/sync_sessions/sessions_sync_manager.cc
[modify] https://crrev.com/159246f269b561bf931fa56a35b2704fab7dcc96/components/sync_sessions/sessions_sync_manager.h
[modify] https://crrev.com/159246f269b561bf931fa56a35b2704fab7dcc96/components/sync_sessions/synced_session_tracker.cc
[modify] https://crrev.com/159246f269b561bf931fa56a35b2704fab7dcc96/components/sync_sessions/synced_session_tracker.h
[modify] https://crrev.com/159246f269b561bf931fa56a35b2704fab7dcc96/components/sync_sessions/synced_session_tracker_unittest.cc
[modify] https://crrev.com/159246f269b561bf931fa56a35b2704fab7dcc96/components/sync_sessions/tab_node_pool.cc
[modify] https://crrev.com/159246f269b561bf931fa56a35b2704fab7dcc96/components/sync_sessions/tab_node_pool.h
[modify] https://crrev.com/159246f269b561bf931fa56a35b2704fab7dcc96/components/sync_sessions/tab_node_pool_unittest.cc

 Issue 672558  has been merged into this issue.

 Issue 674208  has been merged into this issue.

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

commit 0b91d7171d412f61c13d6e4aeb44ebbb26e9544b
Author: zea <zea@chromium.org>
Date: Wed Dec 14 19:59:57 2016

Revert "Reland of [Sync] Put session tracker in charge of maintaining local state."

Reason for revert: crashing on android, spike in datatype errors across
platforms. See crbug.com/673618

Reland of [Sync] Put session tracker in charge of maintaining local state.

Previously, the session tracker was only updated when local tabs were
available in the TabModel. It was not updated with sync data from a previous
session (and as a result, we had to pass around the restored_data list
at association time in order to handle restoring placeholder tabs).

The session tracker now gets updated at startup of previous local state,
which is then used when reassociating restored tabs. This is a purely
refactoring change that should not introduce user visible changes
(although internally the data is managed in a different way).

To make this happen, InitFromSyncModel now treats local tabs and remote
tabs the same. In addition, the SyncedSessionTracker now has a
ReassociateTab method that can be called to update the tab id of a
SessionTab. To make this work, the TabNodePool has been moved into
the SyncedSessionTracker, and in general data ownership has been
simplified (with quite a few new tests added).

This is all necessary in order to enable preserving tabs from a previous
session when they're not present in the TabModel (which can happen on
Android when a custom tab is opened, but the main tabbed activity is not
loaded).

BUG= 639009 , 673618
Committed: https://crrev.com/159246f269b561bf931fa56a35b2704fab7dcc96
Cr-Commit-Position: refs/heads/master@{#436786}

TBR=skym@chromium.org

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

[modify] https://crrev.com/0b91d7171d412f61c13d6e4aeb44ebbb26e9544b/chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc
[modify] https://crrev.com/0b91d7171d412f61c13d6e4aeb44ebbb26e9544b/components/sync_sessions/sessions_sync_manager.cc
[modify] https://crrev.com/0b91d7171d412f61c13d6e4aeb44ebbb26e9544b/components/sync_sessions/sessions_sync_manager.h
[modify] https://crrev.com/0b91d7171d412f61c13d6e4aeb44ebbb26e9544b/components/sync_sessions/synced_session_tracker.cc
[modify] https://crrev.com/0b91d7171d412f61c13d6e4aeb44ebbb26e9544b/components/sync_sessions/synced_session_tracker.h
[modify] https://crrev.com/0b91d7171d412f61c13d6e4aeb44ebbb26e9544b/components/sync_sessions/synced_session_tracker_unittest.cc
[modify] https://crrev.com/0b91d7171d412f61c13d6e4aeb44ebbb26e9544b/components/sync_sessions/tab_node_pool.cc
[modify] https://crrev.com/0b91d7171d412f61c13d6e4aeb44ebbb26e9544b/components/sync_sessions/tab_node_pool.h
[modify] https://crrev.com/0b91d7171d412f61c13d6e4aeb44ebbb26e9544b/components/sync_sessions/tab_node_pool_unittest.cc

Status update: will be getting back to this now that I'm back from vacation and am mostly caught up on other items.

Is this still going to be in M-56?

No, punting to 58.

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

commit 0b0a0a13587273010f58db836c86c0d4748e9e5b
Author: zea <zea@chromium.org>
Date: Thu Jan 26 19:49:40 2017

Reland v3 of Session refactor

Original CL review: https://codereview.chromium.org/2494533002

This relands that CL, with a couple key fixes:
- Detect if a previously synced tab no longer exists, and ignore it if so
- Simplifies the logic to add tab ids to a window to avoid possibly out-of-bounds
- Adds checking for invalid tab node ids
- Simplifies the logic that either calls AssociateTab or AssociateRestoredPlaceholderTab
- Adds some CHECKs to help detect remaining issues.

BUG= 639009 , 673618

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

[modify] https://crrev.com/0b0a0a13587273010f58db836c86c0d4748e9e5b/chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc
[modify] https://crrev.com/0b0a0a13587273010f58db836c86c0d4748e9e5b/components/sync_sessions/sessions_sync_manager.cc
[modify] https://crrev.com/0b0a0a13587273010f58db836c86c0d4748e9e5b/components/sync_sessions/sessions_sync_manager.h
[modify] https://crrev.com/0b0a0a13587273010f58db836c86c0d4748e9e5b/components/sync_sessions/synced_session_tracker.cc
[modify] https://crrev.com/0b0a0a13587273010f58db836c86c0d4748e9e5b/components/sync_sessions/synced_session_tracker.h
[modify] https://crrev.com/0b0a0a13587273010f58db836c86c0d4748e9e5b/components/sync_sessions/synced_session_tracker_unittest.cc
[modify] https://crrev.com/0b0a0a13587273010f58db836c86c0d4748e9e5b/components/sync_sessions/tab_node_pool.cc
[modify] https://crrev.com/0b0a0a13587273010f58db836c86c0d4748e9e5b/components/sync_sessions/tab_node_pool.h
[modify] https://crrev.com/0b0a0a13587273010f58db836c86c0d4748e9e5b/components/sync_sessions/tab_node_pool_unittest.cc

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

commit ddd430086336c26c832f8a4f80c6f595d5f61a10
Author: zea <zea@chromium.org>
Date: Tue Jan 31 01:00:37 2017

Revert of Reland v3 of Session refactor (patchset #5 id:70001 of https://codereview.chromium.org/2651583006/ )

Reason for revert:
crbug.com/686620

Original issue's description:
> Reland v3 of Session refactor
>
> Original CL review: https://codereview.chromium.org/2494533002
>
> This relands that CL, with a couple key fixes:
> - Detect if a previously synced tab no longer exists, and ignore it if so
> - Simplifies the logic to add tab ids to a window to avoid possibly out-of-bounds
> - Adds checking for invalid tab node ids
> - Simplifies the logic that either calls AssociateTab or AssociateRestoredPlaceholderTab
> - Adds some CHECKs to help detect remaining issues.
>
> BUG= 639009 , 673618
>
> Review-Url: https://codereview.chromium.org/2651583006
> Cr-Commit-Position: refs/heads/master@{#446405}
> Committed: https://chromium.googlesource.com/chromium/src/+/0b0a0a13587273010f58db836c86c0d4748e9e5b

TBR=skym@chromium.org,pnoland@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 639009 , 673618

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

[modify] https://crrev.com/ddd430086336c26c832f8a4f80c6f595d5f61a10/chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc
[modify] https://crrev.com/ddd430086336c26c832f8a4f80c6f595d5f61a10/components/sync_sessions/sessions_sync_manager.cc
[modify] https://crrev.com/ddd430086336c26c832f8a4f80c6f595d5f61a10/components/sync_sessions/sessions_sync_manager.h
[modify] https://crrev.com/ddd430086336c26c832f8a4f80c6f595d5f61a10/components/sync_sessions/synced_session_tracker.cc
[modify] https://crrev.com/ddd430086336c26c832f8a4f80c6f595d5f61a10/components/sync_sessions/synced_session_tracker.h
[modify] https://crrev.com/ddd430086336c26c832f8a4f80c6f595d5f61a10/components/sync_sessions/synced_session_tracker_unittest.cc
[modify] https://crrev.com/ddd430086336c26c832f8a4f80c6f595d5f61a10/components/sync_sessions/tab_node_pool.cc
[modify] https://crrev.com/ddd430086336c26c832f8a4f80c6f595d5f61a10/components/sync_sessions/tab_node_pool.h
[modify] https://crrev.com/ddd430086336c26c832f8a4f80c6f595d5f61a10/components/sync_sessions/tab_node_pool_unittest.cc

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

commit 9e83363531256aad94ed2bec676bbca8d715fbe1
Author: zea <zea@chromium.org>
Date: Fri Feb 10 18:59:11 2017

Reland v4 of Session refactor

Previous review: https://codereview.chromium.org/2651583006/

Changes from previous review:
- Deletes sync tab nodes with invalid tab ids
- Adds support for gracefully ignoring tabs with the same tab id. It's
unclear how this might happen, but we now log an error and silently
ignore the tab, instead of crashing.
- Adds some more checks to tests

TEST=SessionsSyncManagerTest.DuplicateTabIdFromNative, SessionsSyncManagerTest.MergeDeletesCorruptTabNodeId
BUG= 639009 

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

[modify] https://crrev.com/9e83363531256aad94ed2bec676bbca8d715fbe1/chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc
[modify] https://crrev.com/9e83363531256aad94ed2bec676bbca8d715fbe1/components/sync_sessions/sessions_sync_manager.cc
[modify] https://crrev.com/9e83363531256aad94ed2bec676bbca8d715fbe1/components/sync_sessions/sessions_sync_manager.h
[modify] https://crrev.com/9e83363531256aad94ed2bec676bbca8d715fbe1/components/sync_sessions/synced_session_tracker.cc
[modify] https://crrev.com/9e83363531256aad94ed2bec676bbca8d715fbe1/components/sync_sessions/synced_session_tracker.h
[modify] https://crrev.com/9e83363531256aad94ed2bec676bbca8d715fbe1/components/sync_sessions/synced_session_tracker_unittest.cc
[modify] https://crrev.com/9e83363531256aad94ed2bec676bbca8d715fbe1/components/sync_sessions/tab_node_pool.cc
[modify] https://crrev.com/9e83363531256aad94ed2bec676bbca8d715fbe1/components/sync_sessions/tab_node_pool.h
[modify] https://crrev.com/9e83363531256aad94ed2bec676bbca8d715fbe1/components/sync_sessions/tab_node_pool_unittest.cc

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

commit 471807b6e3af01f221c48a2b96b8156763ca1db1
Author: zea <zea@chromium.org>
Date: Tue Feb 14 03:59:46 2017

Revert of Reland v4 of Session refactor (patchset #3 id:40001 of https://codereview.chromium.org/2683263002/ )

Reason for revert:
Still crashing. Likely due to sync nodes being reused across windows not being handled gracefully.

Original issue's description:
> Reland v4 of Session refactor
>
> Previous review: https://codereview.chromium.org/2651583006/
>
> Changes from previous review:
> - Deletes sync tab nodes with invalid tab ids
> - Adds support for gracefully ignoring tabs with the same tab id. It's
> unclear how this might happen, but we now log an error and silently
> ignore the tab, instead of crashing.
> - Adds some more checks to tests
>
> TEST=SessionsSyncManagerTest.DuplicateTabIdFromNative, SessionsSyncManagerTest.MergeDeletesCorruptTabNodeId
> BUG= 639009 
>
> Review-Url: https://codereview.chromium.org/2683263002
> Cr-Commit-Position: refs/heads/master@{#449677}
> Committed: https://chromium.googlesource.com/chromium/src/+/9e83363531256aad94ed2bec676bbca8d715fbe1

TBR=skym@chromium.org,pnoland@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 639009 

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

[modify] https://crrev.com/471807b6e3af01f221c48a2b96b8156763ca1db1/chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc
[modify] https://crrev.com/471807b6e3af01f221c48a2b96b8156763ca1db1/components/sync_sessions/sessions_sync_manager.cc
[modify] https://crrev.com/471807b6e3af01f221c48a2b96b8156763ca1db1/components/sync_sessions/sessions_sync_manager.h
[modify] https://crrev.com/471807b6e3af01f221c48a2b96b8156763ca1db1/components/sync_sessions/synced_session_tracker.cc
[modify] https://crrev.com/471807b6e3af01f221c48a2b96b8156763ca1db1/components/sync_sessions/synced_session_tracker.h
[modify] https://crrev.com/471807b6e3af01f221c48a2b96b8156763ca1db1/components/sync_sessions/synced_session_tracker_unittest.cc
[modify] https://crrev.com/471807b6e3af01f221c48a2b96b8156763ca1db1/components/sync_sessions/tab_node_pool.cc
[modify] https://crrev.com/471807b6e3af01f221c48a2b96b8156763ca1db1/components/sync_sessions/tab_node_pool.h
[modify] https://crrev.com/471807b6e3af01f221c48a2b96b8156763ca1db1/components/sync_sessions/tab_node_pool_unittest.cc

Thanks zea@ for reverting this.

Just to update, 'sync_sessions::SyncedSessionTracker::PutTabInWindow' is #1 browser crash on the latest canary(58.0.3011.0- 12 crashes from 12 clients) which has been live for 8 hours.

Link to the list of the builds with this crashes:
==================================================
https://crash.corp.google.com/browse?q=custom_data.ChromeCrashProto.ptype%3D%27browser%27%20AND%20custom_data.ChromeCrashProto.magic_signature_1.name%3D%27sync_sessions%3A%3ASyncedSessionTracker%3A%3APutTabInWindow%27%20AND%20product.name%3D%27Chrome_Android%27&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D

'sync_sessions::SyncedSessionTracker::PutTabInWindow' crashes obserevd with below steps 

Application Version (from "Chrome Settings > About Chrome"): 58.0.3012.0
Android Build Number (from "Android Settings > About Phone/Tablet"): N2F90
Device: Pixel

Steps to reproduce: 
1.Launch chrome
2.Put in Multiwindow
3.Long press on Most visited section or Downloads area or Articles for you and "open in other Window"
4.Drag the the multi window to merge the tabs 

Observed behavior: 
chrome crashes

Expected behavior: 
Chrome shoudn't crash 

Frequency: 5/5


Crash Links: 
https://crash.corp.google.com/browse?stbtiq=e9084a3580000000
https://crash.corp.google.com/browse?stbtiq=e11bba5280000000

Adding Release Block Dev and RVG Labels as its direct crash and Frequency of reproducibility is more.  

Exception:

02-14 13:23:41.252  1064 17523 W ActivityManager:   Force finishing activity com.chrome.dev/org.chromium.chrome.browser.ChromeTabbedActivity
02-14 13:23:41.266  1064  1094 W ActivityManager: Failed setting process group of 16965 to 1
02-14 13:23:41.266  1064  1094 W System.err: java.lang.IllegalArgumentException: Given thread 17216 does not exist
02-14 13:23:41.267  1064  1094 W System.err: 	at android.os.Process.setThreadPriority(Native Method)
02-14 13:23:41.267  1064  1094 W System.err: 	at com.android.server.am.ActivityManagerService.applyOomAdjLocked(ActivityManagerService.java:20534)
02-14 13:23:41.267  1064  1094 W System.err: 	at com.android.server.am.ActivityManagerService.updateOomAdjLocked(ActivityManagerService.java:21033)
02-14 13:23:41.267  1064  1094 W System.err: 	at com.android.server.am.BroadcastQueue.processCurBroadcastLocked(BroadcastQueue.java:274)
02-14 13:23:41.267  1064  1094 W System.err: 	at com.android.server.am.BroadcastQueue.processNextBroadcast(BroadcastQueue.java:1225)
02-14 13:23:41.267  1064  1094 W System.err: 	at com.android.server.am.BroadcastQueue$BroadcastHandler.handleMessage(BroadcastQueue.java:172)
02-14 13:23:41.267  1064  1094 W System.err: 	at android.os.Handler.dispatchMessage(Handler.java:102)
02-14 13:23:41.267  1064  1094 W System.err: 	at android.os.Looper.loop(Looper.java:154)
02-14 13:23:41.267  1064  1094 W System.err: 	at android.os.HandlerThread.run(HandlerThread.java:61)
02-14 13:23:41.267  1064  1094 W System.err: 	at com.android.server.ServiceThread.run(ServiceThread.java:46)
02-14 13:23:41.314  1064  2193 D ConnectivityService: ConnectivityService NetworkRequestInfo binderDied(NetworkRequest [ LISTEN id=211, [ Capabilities: INTERNET&NOT_RESTRICTED&TRUSTED&FOREGROUND] ], android.os.BinderProxy@67bbfbf)
02-14 13:23:41.315  1064  2146 D ConnectivityService: ConnectivityService NetworkRequestInfo binderDied(NetworkRequest [ LISTEN id=203, [ Capabilities: INTERNET&NOT_RESTRICTED&TRUSTED&FOREGROUND] ], android.os.BinderProxy@3d7068c)
02-14 13:23:41.315  1064  2194 D GraphicsStats: Buffer count: 5
02-14 13:23:41.315  1064  1376 W InputDispatcher: channel '23c9e49 com.chrome.dev/org.chromium.chrome.browser.ChromeTabbedActivity (server)' ~ Consumer closed input channel or an error occurred.  events=0xd
02-14 13:23:41.315  1064  1376 E InputDispatcher: channel '23c9e49 com.chrome.dev/org.chromium.chrome.browser.ChromeTabbedActivity (server)' ~ Channel is unrecoverably broken and will be disposed!
02-14 13:23:41.316  1064  1390 E ConnectivityService: RemoteException caught trying to send a callback msg for NetworkRequest [ LISTEN id=211, [ Capabilities: INTERNET&NOT_RESTRICTED&TRUSTED&FOREGROUND] ]
02-14 13:23:41.317  1064  1390 E ConnectivityService: RemoteException caught trying to send a callback msg for NetworkRequest [ LISTEN id=203, [ Capabilities: INTERNET&NOT_RESTRICTED&TRUSTED&FOREGROUND] ]
02-14 13:23:41.318  1064  2146 I ActivityManager: Process com.chrome.dev (pid 16965) has died
02-14 13:23:41.318  1064  2146 D ActivityManager: cleanUpApplicationRecord -- 16965
02-14 13:23:41.326  1064  6183 I OpenGLRenderer: Initialized EGL, version 1.4
02-14 13:23:41.326  1064  6183 D OpenGLRenderer: Swap behavior 1
02-14 13:23:41.329  1064  2192 I WindowManager: WIN DEATH: Window{23c9e49 u0 com.chrome.dev/org.chromium.chrome.browser.ChromeTabbedActivity}
02-14 13:23:41.329  1064  2192 W InputDispatcher: Attempted to unregister already unregistered input channel '23c9e49 com.chrome.dev/org.chromium.chrome.browser.ChromeTabbedActivity (server)'
02-14 13:23:41.329  1064  2192 W WindowManager: Force-removing child win Window{2d4a1d5 u0 SurfaceView - com.chrome.dev/org.chromium.chrome.browser.ChromeTabbedActivity} from container Window{23c9e49 u0 com.chrome.dev/org.chromium.chrome.browser.ChromeTabbedActivity}
02-14 13:23:41.334  1064  2146 W ActivityManager: Scheduling restart of crashed service com.chrome.dev/org.chromium.chrome.browser.crash.MinidumpUploadService in 2192ms
02-14 13:23:41.334  1064  1872 W WindowManager: Failed looking up window
02-14 13:23:41.334  1064  1872 W WindowManager: java.lang.IllegalArgumentException: Requested window android.os.BinderProxy@3fefe26 does not exist
02-14 13:23:41.334  1064  1872 W WindowManager: 	at com.android.server.wm.WindowManagerService.windowForClientLocked(WindowManagerService.java:9415)
02-14 13:23:41.334  1064  1872 W WindowManager: 	at com.android.server.wm.WindowManagerService.windowForClientLocked(WindowManagerService.java:9406)
02-14 13:23:41.334  1064  1872 W WindowManager: 	at com.android.server.wm.WindowState$DeathRecipient.binderDied(WindowState.java:1800)
02-14 13:23:41.334  1064  1872 W WindowManager: 	at android.os.BinderProxy.sendDeathNotice(Binder.java:688)
02-14 13:23:41.334  1064  1872 I WindowManager: WIN DEATH: null
02-14 13:23:41.334  1064  1931 I WindowManager: WIN DEATH: Window{f8e90c9 u0 SurfaceView - com.chrome.dev/org.chromium.chrome.browser.ChromeTabbedActivity2}
02-14 13:23:41.337 17342 17342 I cr_ChildProcessService: Destroying ChildProcessService pid=17342
02-14 13:23:41.338  1064  2147 I WindowManager: WIN DEATH: Window{48fea31 u0 com.chrome.dev/org.chromium.chrome.browser.ChromeTabbedActivity2}
02-14 13:23:41.342 17217 17217 I cr_ChildProcessService: Destroying ChildProcessService pid=17217
02-14 13:23:41.345 17494 17494 I cr_ChildProcessService: Destroying ChildProcessService pid=17494
02-14 13:23:41.353   636   636 I Zygote  : Process 16965 exited due to signal (4)
02-14 13:23:41.353  1064  2146 I ActivityManager: Killing 17494:com.chrome.dev:sandboxed_process4/u0a123i150 (adj 0): isolated not needed
02-14 13:23:41.354  1064  2146 I ActivityManager: Killing 17342:com.chrome.dev:sandboxed_process1/u0a123i147 (adj 0): isolated not needed
02-14 13:23:41.354  1064  2146 I ActivityManager: Killing 17429:com.chrome.dev:sandboxed_process3/u0a123i149 (adj 0): isolated not needed
02-14 13:23:41.354  1064  2146 I ActivityManager: Killing 17217:com.chrome.dev:sandboxed_process0/u0a123i146 (adj 0): isolated not needed
02-14 13:23:41.420  1064  1931 I ActivityManager: Process com.chrome.dev:privileged_process0 (pid 17258) has died
02-14 13:23:41.420  1064  1931 D ActivityManager: cleanUpApplicationRecord -- 17258
02-14 13:23:41.475  1064  1930 D ActivityManager: cleanUpApplicationRecord -- 17494

Adding Further information for Comment#53 

Good Build: 58.0.3011.0
Bad Build: 58.0.3012.0

Bisect Range : https://chromium.googlesource.com/chromium/src/+log/58.0.3011.0..58.0.3012.0?pretty=fuller&n=10000

Can not bisect because it doesn't happen on Public Builds.

Please find the log & video @ http://go/chrome-androidlogs1/6/639009

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

commit df2f9f173ff3d7c80d20a3877d6625a105ceb0a0
Author: Alex Mineer <amineer@chromium.org>
Date: Tue Feb 14 19:26:44 2017

Revert of Reland v4 of Session refactor (patchset #3 id:40001 of https://codereview.chromium.org/2683263002/ )

Reason for revert:
Still crashing. Likely due to sync nodes being reused across windows not being handled gracefully.

Original issue's description:
> Reland v4 of Session refactor
>
> Previous review: https://codereview.chromium.org/2651583006/
>
> Changes from previous review:
> - Deletes sync tab nodes with invalid tab ids
> - Adds support for gracefully ignoring tabs with the same tab id. It's
> unclear how this might happen, but we now log an error and silently
> ignore the tab, instead of crashing.
> - Adds some more checks to tests
>
> TEST=SessionsSyncManagerTest.DuplicateTabIdFromNative, SessionsSyncManagerTest.MergeDeletesCorruptTabNodeId
> BUG= 639009 
>
> Review-Url: https://codereview.chromium.org/2683263002
> Cr-Commit-Position: refs/heads/master@{#449677}
> Committed: https://chromium.googlesource.com/chromium/src/+/9e83363531256aad94ed2bec676bbca8d715fbe1

TBR=skym@chromium.org,pnoland@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 639009 

(cherry picked from commit 471807b6e3af01f221c48a2b96b8156763ca1db1)

Review-Url: https://codereview.chromium.org/2694963002
Cr-Original-Commit-Position: refs/heads/master@{#450231}
Cr-Commit-Position: refs/branch-heads/3012@{#4}
Cr-Branched-From: 66847e854d3acd13965764bcbc72d38f455c463c-refs/heads/master@{#450199}

[modify] https://crrev.com/df2f9f173ff3d7c80d20a3877d6625a105ceb0a0/chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc
[modify] https://crrev.com/df2f9f173ff3d7c80d20a3877d6625a105ceb0a0/components/sync_sessions/sessions_sync_manager.cc
[modify] https://crrev.com/df2f9f173ff3d7c80d20a3877d6625a105ceb0a0/components/sync_sessions/sessions_sync_manager.h
[modify] https://crrev.com/df2f9f173ff3d7c80d20a3877d6625a105ceb0a0/components/sync_sessions/synced_session_tracker.cc
[modify] https://crrev.com/df2f9f173ff3d7c80d20a3877d6625a105ceb0a0/components/sync_sessions/synced_session_tracker.h
[modify] https://crrev.com/df2f9f173ff3d7c80d20a3877d6625a105ceb0a0/components/sync_sessions/synced_session_tracker_unittest.cc
[modify] https://crrev.com/df2f9f173ff3d7c80d20a3877d6625a105ceb0a0/components/sync_sessions/tab_node_pool.cc
[modify] https://crrev.com/df2f9f173ff3d7c80d20a3877d6625a105ceb0a0/components/sync_sessions/tab_node_pool.h
[modify] https://crrev.com/df2f9f173ff3d7c80d20a3877d6625a105ceb0a0/components/sync_sessions/tab_node_pool_unittest.cc

Thank you for those repro steps! I had a feeling multi-window issues were the blind spot in all the testing (which have coverage for multiple windows on Android), and your repro steps confirm it.

Correction: the tests don't have coverage for multi-window. I'm in the process of refactoring the tests to enable multi-window coverage, which will let me verify a possible crash fix when I reland.

Checked for Comment# 53 with steps , Issue is not reproducible on 58.0.3012.4

We haven't seen this crash in builds 3013++, which I imagine is because that's the first place the revert in c#51 landed.  I'm going to remove the RB-Dev as this is no longer crashing from what I can tell.

FWIW we should also have filed a separate bug for the crash, since what's talked about in c#0 seems totally unrelated to recent discussion here...

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

commit 5bdcd6fff79307e080a87ee417960f2e2141dc24
Author: zea <zea@chromium.org>
Date: Thu Feb 23 00:58:49 2017

[Sync] Refactor SessionsSyncManager unit tests

Tests no longer rely on BrowserWithTestTestWindowTest, and instead inject
all the relevant navigation events directly. This allows for much more control
over what gets tested, and in particular allows for testing situations with
multiple windows.

As part of this cleanup, various helper methods were added to make the tests
simpler and more readable. A couple outdated tests were also removed.

Lastly, because there are no longer any content dependencies, the test was
moved to components/sync_sessions, to live next to the code it's actually
testing (SessionsSyncManager).

BUG= 639009 ,  671283 

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

[delete] https://crrev.com/169beb0367bc0ca03c4cb0baa18d183740f157d0/chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc
[modify] https://crrev.com/5bdcd6fff79307e080a87ee417960f2e2141dc24/chrome/test/BUILD.gn
[modify] https://crrev.com/5bdcd6fff79307e080a87ee417960f2e2141dc24/components/sync_sessions/BUILD.gn
[add] https://crrev.com/5bdcd6fff79307e080a87ee417960f2e2141dc24/components/sync_sessions/sessions_sync_manager_unittest.cc

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

commit 9b42f431d47b0e3a00b9435c719ce42f7c53c2c9
Author: zea <zea@chromium.org>
Date: Wed Mar 01 19:15:44 2017

Reland v5 of Sessions Refactor

Changes from last review:
Added support for mapping a tab that had already been mapped to a different
window. This can happen in Android when a placeholder tab is created from
a previous window that hasn't yet closed. In addition, for testing/repro
purposes this patch makes the iteration order of windows deterministic (
by going through them in SessionID order).

Previous review: https://codereview.chromium.org/2683263002/

TEST=SessionsSyncMagerTest.PlaceholderConflictAcrossWindows
BUG= 639009 

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

[modify] https://crrev.com/9b42f431d47b0e3a00b9435c719ce42f7c53c2c9/chrome/browser/sync/glue/synced_window_delegates_getter_android.cc
[modify] https://crrev.com/9b42f431d47b0e3a00b9435c719ce42f7c53c2c9/chrome/browser/sync/glue/synced_window_delegates_getter_android.h
[modify] https://crrev.com/9b42f431d47b0e3a00b9435c719ce42f7c53c2c9/chrome/browser/ui/sync/browser_synced_window_delegates_getter.cc
[modify] https://crrev.com/9b42f431d47b0e3a00b9435c719ce42f7c53c2c9/chrome/browser/ui/sync/browser_synced_window_delegates_getter.h
[modify] https://crrev.com/9b42f431d47b0e3a00b9435c719ce42f7c53c2c9/components/sync/protocol/session_specifics.proto
[modify] https://crrev.com/9b42f431d47b0e3a00b9435c719ce42f7c53c2c9/components/sync_sessions/session_data_type_controller.cc
[modify] https://crrev.com/9b42f431d47b0e3a00b9435c719ce42f7c53c2c9/components/sync_sessions/session_data_type_controller_unittest.cc
[modify] https://crrev.com/9b42f431d47b0e3a00b9435c719ce42f7c53c2c9/components/sync_sessions/sessions_sync_manager.cc
[modify] https://crrev.com/9b42f431d47b0e3a00b9435c719ce42f7c53c2c9/components/sync_sessions/sessions_sync_manager.h
[modify] https://crrev.com/9b42f431d47b0e3a00b9435c719ce42f7c53c2c9/components/sync_sessions/sessions_sync_manager_unittest.cc
[modify] https://crrev.com/9b42f431d47b0e3a00b9435c719ce42f7c53c2c9/components/sync_sessions/synced_session_tracker.cc
[modify] https://crrev.com/9b42f431d47b0e3a00b9435c719ce42f7c53c2c9/components/sync_sessions/synced_session_tracker.h
[modify] https://crrev.com/9b42f431d47b0e3a00b9435c719ce42f7c53c2c9/components/sync_sessions/synced_session_tracker_unittest.cc
[modify] https://crrev.com/9b42f431d47b0e3a00b9435c719ce42f7c53c2c9/components/sync_sessions/synced_window_delegates_getter.h
[modify] https://crrev.com/9b42f431d47b0e3a00b9435c719ce42f7c53c2c9/components/sync_sessions/tab_node_pool.cc
[modify] https://crrev.com/9b42f431d47b0e3a00b9435c719ce42f7c53c2c9/components/sync_sessions/tab_node_pool.h
[modify] https://crrev.com/9b42f431d47b0e3a00b9435c719ce42f7c53c2c9/components/sync_sessions/tab_node_pool_unittest.cc
[modify] https://crrev.com/9b42f431d47b0e3a00b9435c719ce42f7c53c2c9/ios/chrome/browser/tabs/tab_model_synced_window_delegate_getter.h
[modify] https://crrev.com/9b42f431d47b0e3a00b9435c719ce42f7c53c2c9/ios/chrome/browser/tabs/tab_model_synced_window_delegate_getter.mm

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

commit 325508e90dc01afb4d6332510fc83d39c74c96f0
Author: zea <zea@chromium.org>
Date: Wed Mar 29 20:11:22 2017

[Sync] Introduce SyncedSessionWindow type.

Because we now need to maintain state outside of the normal SessionWindow
struct, this patch introduces a SyncedSessionWindow type. It wraps a
normal SessionWindow, while allowing tab sync to track additional window
information.

As part of this patch, I've also cleaned up the unused variation ids.
Moving forward, nothing sync specific should need to be in
SessionWindow. We'll probably need to add a wrapper
for SessionTab in the future as well.

The patch itself does not have any behavioral impact, but paves the way for
consuming sync window type information.

BUG= 639009 
TBR=bauerb, skuhne, thestig

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

[modify] https://crrev.com/325508e90dc01afb4d6332510fc83d39c74c96f0/chrome/browser/android/foreign_session_helper.cc
[modify] https://crrev.com/325508e90dc01afb4d6332510fc83d39c74c96f0/chrome/browser/extensions/api/sessions/sessions_api.cc
[modify] https://crrev.com/325508e90dc01afb4d6332510fc83d39c74c96f0/chrome/browser/sync/test/integration/performance/sessions_sync_perf_test.cc
[modify] https://crrev.com/325508e90dc01afb4d6332510fc83d39c74c96f0/chrome/browser/sync/test/integration/sessions_helper.cc
[modify] https://crrev.com/325508e90dc01afb4d6332510fc83d39c74c96f0/chrome/browser/sync/test/integration/sessions_helper.h
[modify] https://crrev.com/325508e90dc01afb4d6332510fc83d39c74c96f0/chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc
[modify] https://crrev.com/325508e90dc01afb4d6332510fc83d39c74c96f0/chrome/browser/ui/webui/foreign_session_handler.cc
[modify] https://crrev.com/325508e90dc01afb4d6332510fc83d39c74c96f0/components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc
[modify] https://crrev.com/325508e90dc01afb4d6332510fc83d39c74c96f0/components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc
[modify] https://crrev.com/325508e90dc01afb4d6332510fc83d39c74c96f0/components/sessions/core/session_types.cc
[modify] https://crrev.com/325508e90dc01afb4d6332510fc83d39c74c96f0/components/sessions/core/session_types.h
[modify] https://crrev.com/325508e90dc01afb4d6332510fc83d39c74c96f0/components/sessions/core/session_types_unittest.cc
[modify] https://crrev.com/325508e90dc01afb4d6332510fc83d39c74c96f0/components/sync/protocol/session_specifics.proto
[modify] https://crrev.com/325508e90dc01afb4d6332510fc83d39c74c96f0/components/sync_sessions/revisit/sessions_page_revisit_observer.cc
[modify] https://crrev.com/325508e90dc01afb4d6332510fc83d39c74c96f0/components/sync_sessions/revisit/sessions_page_revisit_observer_unittest.cc
[modify] https://crrev.com/325508e90dc01afb4d6332510fc83d39c74c96f0/components/sync_sessions/session_sync_test_helper.cc
[modify] https://crrev.com/325508e90dc01afb4d6332510fc83d39c74c96f0/components/sync_sessions/sessions_sync_manager.cc
[modify] https://crrev.com/325508e90dc01afb4d6332510fc83d39c74c96f0/components/sync_sessions/sessions_sync_manager.h
[modify] https://crrev.com/325508e90dc01afb4d6332510fc83d39c74c96f0/components/sync_sessions/sessions_sync_manager_unittest.cc
[modify] https://crrev.com/325508e90dc01afb4d6332510fc83d39c74c96f0/components/sync_sessions/sync_sessions_metrics.cc
[modify] https://crrev.com/325508e90dc01afb4d6332510fc83d39c74c96f0/components/sync_sessions/sync_sessions_metrics_unittest.cc
[modify] https://crrev.com/325508e90dc01afb4d6332510fc83d39c74c96f0/components/sync_sessions/synced_session.cc
[modify] https://crrev.com/325508e90dc01afb4d6332510fc83d39c74c96f0/components/sync_sessions/synced_session.h
[modify] https://crrev.com/325508e90dc01afb4d6332510fc83d39c74c96f0/components/sync_sessions/synced_session_tracker.cc
[modify] https://crrev.com/325508e90dc01afb4d6332510fc83d39c74c96f0/components/sync_sessions/synced_session_tracker.h
[modify] https://crrev.com/325508e90dc01afb4d6332510fc83d39c74c96f0/components/sync_sessions/synced_session_tracker_unittest.cc
[modify] https://crrev.com/325508e90dc01afb4d6332510fc83d39c74c96f0/ios/chrome/browser/ui/ntp/recent_tabs/synced_sessions.mm

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

commit 92d4a108882a01897d144d810bc22bcce20e9b46
Author: zea <zea@chromium.org>
Date: Tue Apr 18 06:26:07 2017

[Sync] Restore previous session if no tabbed windows present

First, adds logic to support restoring the in-memory representation of the local
session based on the previous synced version of that session. This involves
updating rewriting SessionIds and updating the SyncedSessionTracker on each
startup.

Second, adds logic to include the previously synced session as part of
reassociation if the current session have at least one tabbed window. This
addresses the issue in Android where a custom tab can be opened without the
original Chrome for Android session being restored. We still want to sync the
custom tab's data, but don't want to lose the old session.

Lastly, this fixes an issue where the default value for Sync ids that are
persisted outside of sync (e.g. in the Android Tab state) were being initialized
to 0. 0 Is a valid sync id, so initialize them to -1.

BUG= 639009 

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

[modify] https://crrev.com/92d4a108882a01897d144d810bc22bcce20e9b46/chrome/android/java/src/org/chromium/chrome/browser/TabState.java
[modify] https://crrev.com/92d4a108882a01897d144d810bc22bcce20e9b46/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java
[modify] https://crrev.com/92d4a108882a01897d144d810bc22bcce20e9b46/chrome/browser/ui/sync/tab_contents_synced_tab_delegate.cc
[modify] https://crrev.com/92d4a108882a01897d144d810bc22bcce20e9b46/components/sync/android/java/src/org/chromium/components/sync/SyncConstants.java
[modify] https://crrev.com/92d4a108882a01897d144d810bc22bcce20e9b46/components/sync_sessions/sessions_sync_manager.cc
[modify] https://crrev.com/92d4a108882a01897d144d810bc22bcce20e9b46/components/sync_sessions/sessions_sync_manager.h
[modify] https://crrev.com/92d4a108882a01897d144d810bc22bcce20e9b46/components/sync_sessions/sessions_sync_manager_unittest.cc
[modify] https://crrev.com/92d4a108882a01897d144d810bc22bcce20e9b46/components/sync_sessions/synced_session_tracker.cc
[modify] https://crrev.com/92d4a108882a01897d144d810bc22bcce20e9b46/components/sync_sessions/synced_session_tracker.h
[modify] https://crrev.com/92d4a108882a01897d144d810bc22bcce20e9b46/components/sync_sessions/synced_session_tracker_unittest.cc
[modify] https://crrev.com/92d4a108882a01897d144d810bc22bcce20e9b46/ios/chrome/browser/sync/ios_chrome_synced_tab_delegate.mm

Ok! I think this last patch might (finally) be it.

Mark, if you get the chance feel free to play around with the next canary/dev build that has this patch. In my own tests this fixed the behavior when dealing with custom tabs.

>>>
Mark, if you get the chance feel free to play around with the next canary/dev build that has this patch. In my own tests this fixed the behavior when dealing with custom tabs.
>>>

I'm afraid all the oddities I notice are on my intensely-used low-memory personal device, which I'd rather not switch to canary/dev.  I think you can assume that you fixed this; if I still see trouble in about, what, four months, I'll let you know. :-)

thanks for all the hard work!

I just hit this CHECK (crash/391f510b90000000) on Windows canary with 60.0.3082.3.

    // TODO(zea): remove this once PutTabInWindow isn't crashing anymore.
    CHECK(tab) << " Unable to find tab " << tab_id
               << " within unmapped tabs or previously mapped windows."
               << "  crbug.com/639009 ";

Re-opening.

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

commit 783900c29c6242e65191a8951e12ce45c739571e
Author: zea <zea@chromium.org>
Date: Fri May 05 00:46:47 2017

[Sync] Handle reassociation of tabs where the old tab is already mapped.

If ReassociateLocalTab is called with a tab node that is already mapped to a
tab, it's possible to wind up with the synced session tracker holding two
tab objects referring to the same tab id. This can lead to memory corruption.

This CL makes the tracker defensive against that scenario, and adds some more
data verification in the unit tests

BUG=714524,  639009 

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

[modify] https://crrev.com/783900c29c6242e65191a8951e12ce45c739571e/components/sync_sessions/sessions_sync_manager.cc
[modify] https://crrev.com/783900c29c6242e65191a8951e12ce45c739571e/components/sync_sessions/sessions_sync_manager_unittest.cc
[modify] https://crrev.com/783900c29c6242e65191a8951e12ce45c739571e/components/sync_sessions/synced_session_tracker.cc
[modify] https://crrev.com/783900c29c6242e65191a8951e12ce45c739571e/components/sync_sessions/synced_session_tracker.h
[modify] https://crrev.com/783900c29c6242e65191a8951e12ce45c739571e/components/sync_sessions/synced_session_tracker_unittest.cc

Not seeing any more crashes in latest canary. Closing out, but please let me know if you hit this again.

This is planned for M-60 now, correct?

Correct.

I'm really looking forward to M-60.  Check out the absurdity of (this fragment of) my M-58 view from my Mac of the tabs open on my phone.  (Trust me, I don't have duplicate pages open.)

Deleted: screenshot.png 164 KB

	screenshot.png 164 KB View Download

Some initial testing on M-60 stable on Android and Windows seems to imply that in order to get my many old Android tabs to sync, I have to actually view them as the "current tab" on Android, otherwise they still don't sync. So if I have N tabs that have been open for many months, I have to go to each one in turn.  Was hoping they would just sync without any action on my part.  But glad this seems to be fixed after so many years.  Thanks for that. In fact I don't really think it was ever working in full since the introduction of tab syncing on Android.

Sorry actually it is not working in full.  It seems like I can only get a fixed number of tabs to sync and it is like 70% of the tabs I have open.  If I force one of the missing tabs to sync, then one of the ones that had previously synced gets dropped so the total number synced is maxed out.  Strange behavior.  Do you need more details?

zea: Should we re-open this bug, or should a new bug be filed to investigate the issues reported in #74? The issue in #75 is new to me though.

Update on my comment #75: what I described seems to apply only to tabs that were already open before upgrading Chrome (stable) on Android.  So if on Chrome M-59, I had X tabs open, then after upgrading to M-60, I can only get Y (about 70%) of those X to sync and not more.  But if I open a brand new tab that hadn't been opened until after the M-60 update, then my total synced tab count is now Y+1.  But Y seems to remain a constant number.

Interesting. Yeah, I could see the previously open synced tabs (the ones from before you upgraded) having issues. The code changes included fixing some bugs around how those tabs are identified, that would have necessarily propagated to pre-existing tabs.

I wonder, if you close all your open tabs on the device, will things work properly from then on?

As promised in comments 65/66, it's now late August and my device has been updated to Chrome 60 which has these changes.

I can report that things are a lot better now, though not perfect.  In particular, I'm still seeing an oddity, exactly like that described by clopes@ in comments 74-78 (especially comment 77).  After updating to Chrome 60, I explicitly swiped through all my open tabs on my Android device.  Then for the next several days, I went around browsing as normal.  When I used my laptop to look at the list of open tabs on my phone, it has all the new tabs I created since the update but only about 70% of the existing tabs.  Funnily enough, when I view some of those existing tabs on my phone in various orders, maybe browse some more, then look again at the view my laptop has, it still appears to always be update-to-date on newly created tabs, yet has a different random subset of 70% of existing tabs.

Do I have to explicitly close all those existing tabs and open new versions of the same web pages to get everything working correctly?  I had hoped that simply bringing them to the foreground and causing a reload would make them behave well.

You likely have to explicitly close them. They have an incorrect sync id persisted, and we don't have a good way of detecting whether the id is correct or not unfortunately. As tabs are naturally closed though this issue should fix itself up.

That said, if you do close all tabs and then start seeing this again, I would love to hear about it in case there's something else that's been missed.

Things are still troubling.

Several days ago, I did the following: I closed all but two of my previously-existing tabs by copying the URL, creating a new tab, pasting the URL in there (well, not actually pasting, as it appeared as a clipboard omnibox suggestion, heh), and swiping away from the old tabs.

Currently I have 13 tabs open on my Android device including two old ones.  The rest have been created after the Chrome 60 update.

When viewing the Recent Tabs from my Mac, I see only 5 tabs (none of two old ones, 5 of the 11 newer ones).

Is this expected?  Do I need to delete the last two old tabs for everything to work correctly?  Or did I not dispose of tabs correctly in the first place?

@zea, Re: comment #78,

As an end user, closing my pre-M60 open tabs is not something that I would really like to do, since:
1) It results in a permanent loss of information, because although I can certainly copy/note the URLs, it is fairly impractical to reproduce other synced data such as the backward and forward history of that tab
2) It is cumbersome to re-open many tabs manually on a mobile device
3) Users expect this to work when updating the app without having to reopen old tabs

As someone who wants to contribute to the betterment of Chromium, I would love to try this test if there was a way to revert back after trying it, or an easy way for advanced/developer-level users to clone/backup a set of tabs from one chrome instance to another (across devices or OS's or release channels) without relying on synced data, since that's what's broken in this case.  I mean I can look in chrome://sync-internals/ locally on Android but there is no way to export from there and then import back to there later.

I've closed all my pre-M60 tabs, and am still having trouble with the sync of tabs.  Right now I have 14 tabs open on my Android device and only 2 are shown on the list of open tabs as seen from my laptop.  (My laptop is also on M-60.)

Should I file a new bug, or should we re-open this one because it hasn't been entirely fixed?

By the way, I can use the History feature on my laptop (using the cloud-powered history data) and easily spot that I viewed several of the missing tabs within the last two hours.  I haven't closed them on my phone in the meantime.  I wonder what makes Sync think those tabs have been closed / are no longer worth showing?

(I wonder if this can be related to  bug 169083 .  Is closing and restarting/restoring Chrome on my phone, and not visiting those old tabs during the new session, making sync think those old tabs are actually closed?)

Reopening to maintain the overall context and assigning to Sky (I don't have cycles to dig into this myself anymore).

Updating to M63 milestone (hopefully we can track down what's busted in time).

Thanks for reopening.  Also wanted to request that the fix work seamlessly (without any user action) for tabs that were already open prior to updating to whatever milestone it gets into, assuming this is practical.

I just had this yesterday on my Pixel XL, with latest dogfood Chrome (not sure exact version, I've since wiped the device as I was moving to different device).

Only 4 (and exactly 4) tabs would sync. I had 15 tabs open.
I attempted to reload all tabs, but still only 4 in 'recent tabs'.
I restarted Chrome, and still only 4 tabs.

Since I was migrating phone, I got around it by using the history of when I had reloaded all the tabs.

I investigated a bit more.  The most obvious problem is the following.  Maybe these repro steps will help you get a handle on how to fix this bug:
1. navigate within a tab on the Chrome on Android.  Call the destination A.
2. wait for all the syncs to happen and the desktop view of the Android device shows the most recent destination (A).
3. create a new tab on the Chrome on Android
4. open the tab view on Chrome on Android and swipe away the old tab (with destination A).
5. in the new tab you created in step 3, navigate somewhere.  Call the destination B.
6. wait for all the syncs to happen and the desktop view of the Android device shows the most recent destination (B).

Expected output:
1. the desktop view of the Android device does not show A, because it was swiped away (and we know the result of that swipe should've propagated, as the navigation to B--which happened after the swipe--propagated.

Actual output:
1. both A and B are listed on the desktop view of the Android device as destinations on the "Recent Tabs" list.


In general, the current flaky behavior of "Recent Tabs" is annoying, making trying to view "Recent Tabs" to see tabs on an Android device frustrating.  Usually I have to resort to searching my Chrome history.  I hope you can look into it soon.  thanks!

Removing R-V-G label as the original crash that triggered it is now more than 6 months old.

How reliably does this repro for you Mark? I just tried going through the steps you outlined, and it worked fine for me.

It repros every time I've tried it, something like 4 for 4.

I wonder if it's to do with some corrupted Sync data under the hood. Do you have a second account you can test with (ideally after you do a clear data via google.com/dashboard to ensure it's in a clean state)?

zea@,
I created a test account, installed chrome canary on the device I've been using, and spent ~1.5 days actively using it in the various ways I used Chrome stable before.  When viewing open tabs as seen from my desktop, I didn't at any point witness a mismatch between tabs shown and tabs I actually had open.  In other words, I couldn't get this bug to appear.

Thus, it must have to do with my pre-existing state.  I'm pretty sure the problem is bad data on the state, as per comment #83 I think we've eliminated the possibility of bad data on my device:
>>>
I've closed all my pre-M60 tabs, and am still having trouble with the sync of tabs.  Right now I have 14 tabs open on my Android device and only 2 are shown on the list of open tabs as seen from my laptop.  (My laptop is also on M-60.)
>>>

You have permission to access my personal account data on the sync servers to investigate.  Mail me separately for the account name.

Note that I'm not the only one reproducing this bug (see other comments on this bug).

Tried to repro this on my own, but so far have been unable to get a fresh profile/account into this state where any inconsistency was shown.

Looked at the sync data authorized in c#93 (note the client was using M-62). Picked a tab sync id (1474) that was currently being referenced by the header node, and scanned through the history of this entry. Found a url that was shown on the 11th, lots of other various values in between, and then that same url on the 9th. The chance of this news related page being independently loaded on those two days, and happening to be randomly assigned the same tab sync id seems very unlikely. Instead, it seems much more likely this tab has been alive the whole time, but was being shadowed by other tabs that used the same tab sync id.

This brings us to my working theory of what's causing this problem, we're incorrectly using the same tab sync id for multiple tabs simultaneously. SessionsSyncManager attaches tab sync ids to the native model, and then uses these ids to match up native data to sync data. Sync effectively uses the tab sync id as the id of the entity that it synced, and updates to the same tab sync id will overwrite the previous data. So if we have n native tabs and m tab sync ids in use, where n > m, then we'd expect to only show m foreign tabs on other devices. This is inline with the current descriptions of symptoms in c#77 and c#81.

In addition to not showing all of the tabs, I would expect the ordering of tabs to be somewhat incorrect as well. People still experiencing this, can you check and let me know if you see incorrect ordering?

It does not explain c#88 or c#73. I think c#73 may be a result of old logic before some of zea@'s changes, so I'm less worried. c#88 on the other hand, can be explained. We don't delete tab data when a tab is closed. Instead we simply un-reference it from the window node. However, if we have two native tabs sharing a tab node id, and one is closed, we'll still find that this _tab_node_id_ is alive, because the other tab is using it. So that keeps it in the window node. The tab didn't change though, so we don't re-write the tab data, and it still holds the data from the deleted tab.

Although I was not able to get a tip of tree build to use duplicate tab sync ids naturally, I did hack at the code and force duplicates. And I did see some tabs that were clearly open on my Android device not show up on a foreign device. I also saw the ordering of tabs was incorrect. Also, no [D]CHECKs while in this state.

But this duplicate tab node id is kind of a symptom, not the bug itself. Why are we in this state? I haven't reproed exactly, so I'm not sure. Three theories

1. Old data from before zea@'s CLs. In my testing, if you have two tabs with the same tab node id, and then closed one, I wasn't able to open a new tab and pick up that tab node id. It seemingly wasn't put into the pool of available ids. Not sure why this was exactly, but this makes it seem like we should self-recover, which is not happening for these users.

2. There's a race condition between sync storage and native sessions storage. This seems clearly possible, and likely the that code should account for this. While I could see a sync record and the native record easily diverging and not agreeing on ids, it's really tricky to imagine a sequence of events that would result in duplicate tab node ids. On startup we read out tab node ids and then keep track of them. Merge on every startup.

3. Some actions cause new duplicate tab node ids. This is the scary scenario, because there could be a major flaw somewhere I just don't see. Unfortunately, I think this is most likely cause. And ideally we find this and fix it.

However, given I haven't found #3, there's another approach that should be possible. We should be able to scan and detect duplicate tab node ids trivially, and, I think, update them to not conflict, even for placeholder tabs. Very easy to slap this  kind of defensive check into merge, I'm a bit hesitant to do it more often than that. I don't want this act as a band-aid that covers up #3, so I plan to add a histogram for how often we have to fix things to mitigate this.

Thanks for the thorough writeup Sky! And thanks for confirming that this is an issue with the pre-existing Sync data Mark!

I agree that going through and detecting duplicate tab ids and fixing them up seems like a good approach. It might be interesting to track how often we're detecting that via UMA so we have a sense of its prevalence too. In theory it should steadily decrease in frequency over time, and if it doesn't that implies that something else is up.

Thanks for the detailed examination of the problem.

>>>
People still experiencing this, can you check and let me know if you see incorrect ordering?
>>>
Yes, the ordering of tabs has no rhyme or reason.


As for
>>>
2. There's a race condition between sync storage and native sessions storage.
and
3. Some actions cause new duplicate tab node ids.
>>>
I'll mention that:
(i) I have a low-ish memory device (1.5 gb) compared to the number of tabs I have.  This means Chrome is killed pretty regularly when it's not on the foreground, sometimes I'm pretty sure practically instantly.  (As in, I go back to Chrome thirty seconds later and witness a cold startup.)  This makes it more likely that's a race condition, or even a race condition that's terminated before both sides finish.
and
(ii) I often use custom tabs -> Open in Chrome.  This speaks to possibility 3.  As a concrete example, I often do: GSA -> click link -> enter into custom tabs -> open in Chrome -> Recents menu -> select GSA -> press back to leave the view of the page that's displayed in custom tabs -> click something else -> custom tabs -> open in Chrome -> and so on.  I can easily imagine this sequence messing things up, especially when done fast.  Sometimes in between these two "open in Chrome" events Chrome is killed.  And I'm not sure if Chrome fully completes startup before I use Recents to go back to GSA.  I often don't wait for the page to fully render, just long enough to see that the handoff to Chrome is complete and the progress bar is moving.


Regardless of whether you get to the root cause, I'm happy for a band-aid solution. :-)

skym@, by the way, I'm willing to run a special / debug chromium build on this device and pull off logs (adb logcat) for you, if you can think that would help.  (If you can add debug logging about tab ids and think it would reproduce with my same account on the same device, just not the same previous session store.)

Here's an anecdote that may help confirm/deny something. I checked to see if I had a mismatch today. My phone had a few tabs open, 1 of them was not showing on desktop "other devices". I closed a few. So now I had 2 tabs open on the phone, we'll call them old-tab and recent-tab.


Phone:
- old-tab
- recent-tab

https://photos.app.goo.gl/BT3zBeXpNPKdnx312

Desktop: 
- closed-yesterday-tab
- recent-tab
- closed-this-morning-tab1
- closed-this-morning-tab2

https://photos.app.goo.gl/hFbr95PiVBgHMNgZ2

So I swiped away chrome and started it again.

Desktop updated to
- closed-this-morning-tab1
- closed-this-morning-tab2
- recent-tab


https://photos.google.com/photo/AF1QipPQHhq5-zRhQ_vi2R_I7wwjqhN3Dn6zD7HCARQ

Then I opened a new tab from GSA and desktop updated to


https://photos.app.goo.gl/7W7LDsWsNQ5cx2mI2

- closed-this-morning-tab1
- closed-this-morning-tab2
- old-tab
- recent-tab
- new-tab


So eventually I got all the open tabs onto desktop.

Happy to provide more info or access to the account involved.

I am unable to view the third photo from c#99, could you re-post it? I'm surprised to hear that you are seeing more tabs in your remote device than you have open on your Android device. That's the opposite of the previous symptoms.

To give an update on my proposed changes from c#95, I'm currently struggling to implement them. Turns out I cannot "fix" things even after detecting duplicate tab node ids, since often one or both of the tabs are in a placeholder state (no web contents anymore). Still working on it.

Sorry, that wasn't the sharing link.

https://photos.app.goo.gl/9Xmf3CWWGHHuypE03

Those actually look like custom tabs (that horizontal bar denotes a different window, which only happens if you have custom tabs open from other apps, e.g. AGSA).

You're right, zea@, they are custom tabs. If they are still opened, then it explains why they're still synced.

I've found by adding a many second sleep to Directory::SaveChanges() I can repro the duplicate tab node issue very consistently.

Unfourtunately, I keep finding assumptions that are not safe to make. If you open and close Chrome really fast, it might run OnLocalTabModified with a tab representation that returns nullptr for every GetTabAt() call from the main window, which ends up removing all your tabs. This is a real problem, because when you re-load Chrome, now everything is a placeholder, so we cannot recover the lost tab data.

Trying to deal with both the tab node id and the tab id being inconsistent, the more I try, seems like an impossibly diffifult task, we need at least one of them to be stable. Part of me wonders if it'd be easier to stop syncing (or at least stop vending tab node ids) when we cannot find a tabbed window. Yes, we'll be worse at syncing custom tabs, but one part of this bug should stop.

If we find two placeholder tabs that use the same tab node id, there's not much we can do to be certain which tab corresponds to the sync data we have. I think we have to drop it. But if we do the above, this should stop happening going forward. But anyone with current symptoms will have their symptoms get slightly worse before they get better.

But, worst of all, with the current tab/sync abstraction, we cannot really handle the case where:

1. Load page A into a new tab.
2. TabState for A is saved to disk.
3. Close/crash Chrome before A can be saved to sync's DB.
4. Open a link into Chrome so that we create another new tab, and A is only loaded into a placeholder.

This is still going to be possible, and still going to result in tabs that don't get synced unless you make them your active tab on your Android device to load the web contents into memory.

Possible ways to get around this would be to allow sync to load the web contents on demand, or maybe read more data out of the native Tab object. Grabbing a timestmap/url/title would be enough to populate the tabs from other devices UI, but wouldn't really be enough to make real SESSIONS data. I'm not sure.

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

commit 88982246725295b628b7c6b10281f0c205985665
Author: Sky Malice <skym@chromium.org>
Date: Wed Jan 03 18:45:52 2018

[Sync] Stop vending sync ids without tabbed window.

This results in a regression, in the sense that we're going to be worse
at syncing custom tabs because of this change. The problem was that we
could assign the same sync id to two seperate tabs on Android when
the main windowed of Chrome was not fully loaded. To fix this, this CL
stops giving out sync ids if we don't have full native data.

While refactoring the checking for a tabbed window, realized that it's
possible to get all null tab objects, and this should be treated as
not having native data as well.

Bug:  639009 ,  797452 
Change-Id: I0f477326b9d2c3fa526399623f089a791f1b3046
Reviewed-on: https://chromium-review.googlesource.com/843574
Reviewed-by: Nicolas Zea <zea@chromium.org>
Commit-Queue: Sky Malice <skym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526753}
[modify] https://crrev.com/88982246725295b628b7c6b10281f0c205985665/components/sync_sessions/sessions_sync_manager.cc
[modify] https://crrev.com/88982246725295b628b7c6b10281f0c205985665/components/sync_sessions/sessions_sync_manager.h
[modify] https://crrev.com/88982246725295b628b7c6b10281f0c205985665/components/sync_sessions/sessions_sync_manager_unittest.cc
[modify] https://crrev.com/88982246725295b628b7c6b10281f0c205985665/components/sync_sessions/synced_session_tracker.cc
[modify] https://crrev.com/88982246725295b628b7c6b10281f0c205985665/components/sync_sessions/synced_session_tracker.h
[modify] https://crrev.com/88982246725295b628b7c6b10281f0c205985665/components/sync_sessions/tab_node_pool.cc

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

commit 5ecd6633bdf1a8f7177b94dce908475ece93f3f6
Author: Sky Malice <skym@chromium.org>
Date: Fri Jan 19 18:03:07 2018

[Sync] Strip out duplicate sync ids when encountered.

Check for duplicate sync ids within sessions native data. If any is
encountered, then clear those sync ids, and let them be re-assigned.
This effectively destroys the link a placeholder has with sync data,
if duplicates are present. This may cause a partial regression for
clients in existing broken states, affected synced tabs will
subsequently disappear from other devices view. But it will also clean
up any broken state before a race condition was removed.

Bug:  639009 
Change-Id: I5f1372107167ae5c5d7b598c982830bd51bb3d26
Reviewed-on: https://chromium-review.googlesource.com/862394
Commit-Queue: Sky Malice <skym@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Nicolas Zea <zea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530559}
[modify] https://crrev.com/5ecd6633bdf1a8f7177b94dce908475ece93f3f6/components/sync_sessions/sessions_sync_manager.cc
[modify] https://crrev.com/5ecd6633bdf1a8f7177b94dce908475ece93f3f6/components/sync_sessions/sessions_sync_manager_unittest.cc
[modify] https://crrev.com/5ecd6633bdf1a8f7177b94dce908475ece93f3f6/components/sync_sessions/synced_session_tracker.cc
[modify] https://crrev.com/5ecd6633bdf1a8f7177b94dce908475ece93f3f6/tools/metrics/histograms/histograms.xml

This should be fixed, at least the major bugs. Thank you everyone for your patience and help investigating this issue. To see more or less correct tabs, your Android device will need to update Chrome to have a version that contains fixes from #105 and #106. The version of Chrome that views these remote tabs will not need to change to see things being healthy.

However, there are a few caveats. You may be missing tabs, tabs that are not fully loaded by Android Chrome. Sync cannot do anything if it doesn't have the right data. By opening a missing tab on Android, it will become fully loaded, and then sync will pick it up. This will continue to happen going forward, although hopefully much less frequently now that  issue 797451  which could cause lots of sync data to be lost.

Also, custom tabs will not sync if Chrome itself is not currently loaded as well. It seems users in this bug were bringing their custom tabs into Chrome app, so this shouldn't affect you folks.

If I'm reading the commit messages right, this should be better in Chrome *66*, which will be in Stable probably in late April.

Thanks for your work fixing this skym@!  I'm hoping this fix sticks, so this bug doesn't have to be reopened again as it was after comment #70. :-)

This is still problematic for me.  In fact, it's worse than my original report.  In the earlier reports, before the long series of code changes, I'd see a good fraction of the tabs.  I'd also see many tabs listed multiple times, which was annoying.  But now I usually don't see many of my tabs at all.

skym@ - can you transfer this to a current sync owner (or take it on yourself)?


I can do the following:
1. Open Chrome on Android.
2. Go to tab switcher, switch to a tab.  Switch to another, and another, and so on.
3. Wait a little.
4. Open "Open Tabs" pane on desktop.  See all the tabs I just viewed.
  [Great so far!]

This is great.  Any tab I touch in a session will show up.

However, if I leave Chrome on Android, do some other things on my phone, and then relaunch Chrome, all these Recent Tabs are no longer shown on my "Open Tabs" list.  Only tabs viewed since the most recent Chrome open are shown.  Usually this might be the tab that appeared in the foreground when I opened Chrome, and nothing else.  Something this will also include a few other tabs, if I actually brought them to the foreground during this particular session.  It also can include a Custom Tab.

I will continue to grant permission to access my person gmail sync history account for the purpose of investigating this issue.  E-mail me directly to ask for the account name.

Chrome version 66.0.3359.158

Repro steps:
1. In Chrome on Android, open a series set of tabs.  Wait a bit.
2. Leave the browser by pressing the home button.
3. Kill the browser using "Recents" and the "X" button.
4. On Chrome on Desktop, display "Open Tabs".  Verify that all the pages you viewed are there.
5. On Android, in another app (I used gmail), click on a link to open a Chrome Custom Tab.
6. On the three-dot menu, click "Open in Chrome".

Expected results:
7. On Chrome on Desktop, on "Open Tabs", you should soon refresh to show all the previously open tabs, plus the new tab you just opened.

Actual results:
7. On Chrome on Desktop, "Open Tabs" refreshes and lists only two tabs, the newly opened one and the one that was previously in the foreground at the end of step 1.

Notes:
* Simply viewing a custom tabs on Android is not a problem.  I can view a custom tab, look for "Open Tabs" page to refresh to a newer time ("refreshed 20 seconds ago"), and it still will show the long list of previously viewed tabs.  (For the record, it doesn't show the page being viewed in a custom tab.  I don't think that's particularly concerning.)
* Likewise, pressing back from the custom tabs view, loading another custom tab, pressing back, etc., also doesn't cause any problems.  It seems to mainly (entirely?) be due to "Open in Chrome" from a Custom Tab.
* I also tried opening GSA (knowing that sometimes it connects to a custom tabs services); this is also not a problem.
  - (I did not try clicking on feed results in GSA, which display in Custom Tabs.  Once I got the repro steps above from the gmail app, I didn't bother trying to analogous experiments in GSA.)

Taking this over, as new owner and active contributor to session sync.

mpearson@, I really appreciate that you surface these issues, since I've been fixing bugs for the last weeks. Code has changed a lot recently, but I do fear that we've been trading some bugs for others.

As per the last issues you described, I'll try to repro myself.

FYI, I'm also working on:
1. Migration to the datatype to a new implementation (USS migration). Almost done, but shares code and bugs with the legacy implementation.
2. A testing plan that covers Android use-cases, most notably related to CCTs.
3. The design of an API extension to let sync know more about CCTs, since the current API makes a proper solution infeasible. 

I still have this issue.
It may sync recently reloaded tabs, but not all tabs that are open.

I currently have 30 tabs open on my Android. If I open Chrome > History and view from other devices, there's only 1 tab shown, and that's the last one I loaded.

I failed to repro steps in #111 on trunk with CCT in custom_tabs_client_example_apk.

I did repro a similar bug (on trunk and stable 67): missing tabs (all except one) when Chrome is closed if a CCT was open at the time of closing Chrome. Repro steps:
1. Open a few tabs in Chrome.
2. Open a tab in a CCT (custom_tabs_client_example_apk).
3. Verify all tabs show up in another device's history.
4. Close Chrome.
5. Open Chrome.

Expected: all tabs should show up.
Actual: only two tabs are displayed.

I think the reason is that, during shutdown, sync code sees tabs being closed and "honors" that, without realizing that the whole browser is being closed. The have some code to detect such scenario in LocalSessionEventHandlerImpl::ScanForTabbedWindow(), but it's clearly broken: it assumes a single valid tab is sufficient to consider a window syncable, but I think it should be the opposite.

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

commit 7552e8c80bb4eaba75d58e366e0a9086979d77a6
Author: Mikel Astiz <mastiz@chromium.org>
Date: Tue Jun 12 14:18:13 2018

Fix missing synced tabs when tabbed activity closed

If a CCT is active at the time the browser app itself is closed, the
shutdown transition from having a tabbed window (prior to the browser
being closed) to not having one (when only the CCT remains) is
problematic. Unless the scenario has special treatment, sync code
sees how tabs disappear one by one (they become null) and proceeds as if
the tabs were closed.

Instead, let's be strict about when a window is considered syncable, and
prevent updates as soon as at least one tab is null.

Bug:  639009 
Change-Id: Iec64b31addc29a8c41e39b9f7aad8cd6fdfcd601
Reviewed-on: https://chromium-review.googlesource.com/1096235
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Sky Malice <skym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566413}
[modify] https://crrev.com/7552e8c80bb4eaba75d58e366e0a9086979d77a6/chrome/browser/sync/glue/synced_window_delegate_android.cc
[modify] https://crrev.com/7552e8c80bb4eaba75d58e366e0a9086979d77a6/components/sync_sessions/local_session_event_handler_impl.cc

Quick update: the issue described in #114 (about tabs disappearing when Chrome is closed while a CCT is active) is not only fixed on trunk, but also covered by our test plan https://testtracker.googleplex.com/testplans/details/1875?revision=107

There's an upcoming patch that should hopefully fix the remaining issues once and for all.

Another update: all known issues should be fixed on trunk and I'm still waiting for the dev release, which hasn't happened since June the 19th.

Latest dev-channel release contains various fixes and I believe address all known problems so far, so marking this as fixed.

Please open or (preferably) file new bugs if you encounter new issues.

Awesome, great job Mikel! Excited to see this resolved!

Thanks.  Will the fix work seamlessly (without any user action) for tabs that were already open prior to updating to whatever milestone it gets into, assuming this is practical?  Or will such tabs have to be viewed, reloaded, or closed/reopened in order to be synced?  Also would you mind updating the anticipated milestone for release when possible?  Is it M-69?

The target milestone is M69, which will hit stable somewhere in September.

There's no user action needed, but there will be an observable, one-off loss of Android tabs (affecting sync only, otherwise the tabs will be there). Those tabs will be synced and visible across devices when they are brought to the foreground (i.e. when they stop being placeholder tabs, as we name background tabs without a WebContents).

Thanks.  So sounds like users will have to view (aka bring to the foreground) each pre-existing tab (that was opened before M69 was installed) in order to get those tabs to sync.

This seems truly fixed!  I've had M-69 for more a week.  I haven't experienced any of the issues that I had on previous milestones.

To get my "lost" tabs to appear, I simply swiped through all my open tabs on my Android device.  My full list of tabs stays around when viewed from another device no matter how many weird things I do on the Android device, such as  "Open in Chrome" from custom tabs, leaving Chrome quickly, getting it killed due to memory pressure, etc.

Hurray! Nice job Mikel.

This is truly amazing to see come to a close! Fantastic work!