CC sebmarchand@ as an FYI because this may bias tab usage metrics

Ha, thanks for cc'ing me. This could effectively affect the tab usage metrics (still a WIP), I'll look at using |closing_all()| in my code.

Hrmm, on second thought, checking closing_all() will be good, but not sufficient.

We'll have this problem any time the active tab is closed and the next active tab will also be closed. So any operation that closes multiple tabs could cause this problem:
* selecting "Close tabs to the right" while one of the tabs to the right is active
* selecting "Close other tabs" while one of the "other" tabs is active
* selecting multiple tabs and choosing "Close tabs" while one of them is active

I notice that TabStripModelObserver::WillCloseAllTabs() is only overridden by a few observers. We could change this to WillCloseMultipleTabs(bool closing_all) to help observers detect this case.

With the TabRanker experiment launching, this is a bigger deal since it skews our success metrics.

+sky@ for his thoughts here.

Potentially add a TabWillBeClosed to TabStripModelObserver, and call that for all tabs to be closed before they are closed? This could potentially be complicated by sites with an unload handler that interrupt a flow of tab close events?

(I'm going to run some experiments, brb.)

Okay, the unload handler presence doesn't stop other tabs from being unloaded, so doesn't interrupt the closing of other tabs in the operation.

Ideally TabStripModel would atomically apply changes (including changes that impact multiple tabs) and then notify observers. Perhaps OnStateChanged() with an object that allowed you to determine exactly what changed. I believe this would avoid the problem you are running into of not being able to tell changes are still being applied.
Erik started a cleanup of observer notification here: 842194. I suggested he try to provide a single function as I outlined above. This is a big task. I'm not sure if Erik intends to tackle it anytime soon.

Agreed with sky. I think this is blocked on a TabStripModelObserver refactor, more details here: https://bugs.chromium.org/p/chromium/issues/detail?id=842194#c5

Agreed that the proposed single observer function would address this issue, and also offer a bunch of nice cleanup paths. Let's block this bug on that one and move any discussion to that bug for now.

Actually, I think this bug should be fixed. My recent work to make sure that all tabs are closed first, and then notifications are sent should prevent this issue from happening. See  Issue 826287 .

Can someone from chrisha's team confirm?

Oh, I didn't see that there was already some refactoring done. I'll take a look.

Okay, thanks for the heads up Eric, confirmed that this is already fixed:

- Open one window to chrome://histograms/TabManager.Discarding.ReloadCount
- Open a second window with multiple tabs, each navigated to a site.
- Navigate the rightmost and active tab in the second window to chrome://discards
- Use the chrome://discards UI to discard every tab in the second window.
- Close the second window.
- Refresh the UMA in the first window.

In Chrome M66 the UMA will have one report for every tab that was closed, as they successively started reloading as they were being detached. On M69 this is tested as no longer occuring (the histogram stays empty).