After installing the linked extension, this immediately repros for me. Printing out the native window/tab information from SessionsSyncManager::OnLocalTabModified, I can still see all the tabs, their titles, and ShouldSync() still returns true. Looking at the header that gets committed, I don't see the discarded tab's ids. So the bug likely is in SessionsSyncManager and composing classes.

Watching the tab get discarded, I see that SyncedTabDelegate::GetSyncId() returns -1 right as it gets discarded, some sessions logic kicks in and doesn't include it in the header.

What really confuses me is why does the tab get added back after you select the tab, because it's the sessions logic that then sets that sync id again. What changes to make sessions think it should be included again?

I have the answers to my previous comment!

I don't know how I missed this earlier, but the values of SyncedTabDelegate::GetSessionId() change. These are from the objects that are returned when we ask sessions_client_ for local state.

And then we're then told to SessionsSyncManager::OnLocalTabModified with the _old_ tab delegate which is no longer a child in the local state objects. And we remove it, because it's gone.

Then if you select the previously discarded tab, state returned by sessions_client_ doesn't change, but we're called through SessionsSyncManager::OnLocalTabModified with the new tab, so we add it back.

Not quite sure what a solution looks like yet though. Reassociating everything on every OnLocalTabModified seems a bit too ham-handed, but it also seems like, with the amount of information we have right now, it's going to be difficult to match up correctly the old and the new tab objects when their id shifts.

First thing to note, matching up the changing tabs doesn't seem necessary, both old and new have the same set of navigation entries. We have to modify the record regardless because the id changed, and modifying the same persistent record or a different one should cost the same, because we don't aggressively delete old records, just stop referencing them.

So when discard gets called, during old WebContent's destructor, we are notified, see call stack

#2 0x5615c5edd4b8 sync_sessions::SessionsSyncManager::OnLocalTabModified()
#3 0x5615c2f3c7b1 sync_sessions::SyncSessionsWebContentsRouter::NotifyTabModified()
#4 0x5615c631b5d4 sync_sessions::SyncSessionsRouterTabHelper::NotifyRouter()
#5 0x5615c631b687 sync_sessions::SyncSessionsRouterTabHelper::WebContentsDestroyed()
#6 0x7fd3a055d7c0 content::WebContentsImpl::~WebContentsImpl()
#7 0x7fd3a055efe9 content::WebContentsImpl::~WebContentsImpl()
#8 0x5615c304a335 resource_coordinator::TabManager::DiscardWebContentsAt()
#9 0x5615c3049f7b resource_coordinator::TabManager::DiscardTabById()
#10 0x5615c304a389 resource_coordinator::TabManager::DiscardTabByExtension()
#11 0x5615c4b95503 extensions::TabsDiscardFunction::Run()
#12 0x5615c16109ad ExtensionFunction::RunWithValidation()
#13 0x5615c1617cb0 extensions::ExtensionFunctionDispatcher::DispatchWithCallbackInternal()
#14 0x5615c1616cc4 extensions::ExtensionFunctionDispatcher::Dispatch()
#15 0x5615c167f0ec extensions::ExtensionWebContentsObserver::OnRequest()

However, there's nothing that informs us this new tab was created in its place, and so we never pick it up. If you look at the current reasons for calling OnLocalTabModified, there's simply nothing that's hit. https://cs.chromium.org/chromium/src/chrome/browser/sync/sessions/sync_sessions_router_tab_helper.cc

We could try to create a new observable method for discarding tabs, we could try to hook into TabStripModelObserver::TabReplacedAt. These would be in line with the old approach. I'm partially worried that we don't want to invoke observing too early, before all the data is there.

Part of what's troubling about this whole scenario though, is how we do not quickly recover once we are given control in SessionsSyncManager. Today it is discarded tabs, but tomorrow there may be a new mechanism that we fail to correctly observer. Part of me wonders if maybe we should do a little more work in OnLocalTabModified that scans for and checks to see if we've fallen behind on tracking any sort of local change. Or try to detect when we've fallen behind or missed something and histogram about it.

Looks like BrowserListRouterHelper is already a TabStripModelObserver, and it's a few lines of code trigger off TabStripModelObserver::TabReplacedAt and fix this issue. Unfortunately, none of this is going to transfer to other session inconsistency bugs, especially given BrowserListRouterHelper is not used on Android.

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

commit 95245d4c4061fdee5fcec5d70b05e9aa3045e5e6
Author: Sky Malice <skym@chromium.org>
Date: Wed Jan 03 21:35:21 2018

[Sync] Notify sessions on TabReplacedAt keep discarded tabs.

When a tab is discarded, a new web contents (with a different id) is
created and the old one is destroyed. We correctly observed the old one
going away, but we missed the new one. This caused the
SessionsSyncManager to stop tracking the tab, and drop its reference
from the header node. This changes ensures the SessionsSyncManager
updates the native id of the tab correctly, which will keep it in the
header.

Bug:  777745 
Change-Id: I0c62cee82e28de1d3607ec2bcb9acfc053000417
Reviewed-on: https://chromium-review.googlesource.com/847995
Commit-Queue: Sky Malice <skym@chromium.org>
Reviewed-by: Patrick Noland <pnoland@google.com>
Cr-Commit-Position: refs/heads/master@{#526827}
[modify] https://crrev.com/95245d4c4061fdee5fcec5d70b05e9aa3045e5e6/chrome/browser/sync/sessions/browser_list_router_helper.cc
[modify] https://crrev.com/95245d4c4061fdee5fcec5d70b05e9aa3045e5e6/chrome/browser/sync/sessions/browser_list_router_helper.h
[modify] https://crrev.com/95245d4c4061fdee5fcec5d70b05e9aa3045e5e6/chrome/browser/sync/sessions/browser_list_router_helper_unittest.cc