1789043 - Rationalize sessionstore-closed-objects-changed and updates to session store closed tab data so they make sense

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[:Gijs (he/him)]

In bug 1787565 it became clear that the sessionstore-closed-objects-changed notification fires when the data for a closed tab is still incomplete. There is some artificial delay there in firing the notification (setTimeout(..., 0)) but that is insufficient. The code in onTabStateUpdate which gets called through the two SessionStoreFunctions APIs, asynchronously, by CanonicalBrowsingContext code, does not fire any notifications but does modify the closed tabs data. If consumers consult this data before those calls have happened, they get stale URLs and/or other data.

This isn't a problem for any non-Firefox-View in-product surfaces that show this data (the recently closed tabs menus in various places in the UI, and the undo close tab shortcut) unless you time your tests extremely precisely - it'd be pretty hard to time it so you tried to obtain closed tab data after the removeTab handler has fired a TabClose event which in turn fires the aforementioned observer notification but before calls to onTabStateUpdate have fired and updated the tab data.

However... as noted in bug 1787565, in Firefox View, we listened for the observer notifications, and then update the UI. This then shows the incomplete data when a cross-origin navigation happens: the closed tab UI shows the outdated URL, and it stays there because we do not re-fetch the data from session store after the list item has been generated. So this was bad.

In bug 1787565 comment 10, Luca also showed that it is possible to hit a similar race condition in webextension code, albeit less consistently because of the async nature of various webextension APIs and notifications.

In that same bug, I tried to fix the issue by just sending more of the same observer notification when the closed tab data got updated by the browsingcontext. Unfortunately this ran into a number of automated tests that expected a specific number of observer notifications. This also revealed that, when closing tabs/BCs, whether either or both of the SessionStoreFunctions get called (and therefore we'd get either 1 or 2 additional firings of the observer topic) is not consistent/predictable across all tab closures.

I think ideally, we should end up in a place where there is 1 observer notification broadcast when a tab is closed, and that should only happen once the data for that tab is complete. This is also what the webextensions code and some sessionstore automated tests expect.

However, it is not clear to me how to get there because, not fully understanding the purpose/origin of the 2 browsingcontext callsites, I don't know if/how we can predict how many of those calls will follow after a CloseTab event.

I'm hoping Nika or other docshell/dom folks can help clarify what we can expect and how to fix this the Right way.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Hmm, I've not had enough time to properly dive into this so I'm leaving my needinfo for now, as I think I'll need to look at it more. After looking at the code a little bit, I'm a bit surprised this wasn't an issue before SHIP, as the notification seems like it would always fire before the final flush is complete (likely multiple times).

Perhaps we should add another notification for when a closed tab "finishes closing" and has had its sessionstore data fully written, rather than when the closed objects list is updated? It seems like listening to sessionstore-closed-objects-changed might be a bit unreliable as a way to determine this information, as we also fire it when e.g. restoring a tab, or when purging session history.

While looking at the sessionstore code, I did come across https://searchfox.org/mozilla-central/rev/685203e4bc211073284f3c36f7f3d34f0953bb9c/browser/components/sessionstore/SessionStore.jsm#1235-1242, which feels incorrect, as it looks like we're bouncing through the content process when collecting session history information which is already present in the parent process for some reason, so perhaps this has something to do with the issue? I'll need to dig a bit deeper to be sure.

(would ni? :farre, but they're out right now)

Comment 2

[Nika Layzell [:nika] (ni? for response)]

I didn't end up having the time to really dig into this in the last 2 months, and I believe :farre is back now, so redirecting the ni? to farre for now.

Comment 3

[Andreas Farre [:farre]]

So this behaviour is because of Fission. The two SessionStoreFunctions handle two separate, equally asynchronous, updates to the session store data, handled by CanonicalBrowsingContext:

1. UpdateSessionStore: writing data to the session store gathered from content processes together with some data from the parent process
2. UpdateSessionStoreFromStorage: data collected from the PBackgroundSessionStorageCache

UpdateSessionStore gets called after a content process has sent all its incremental updates. This happens either when a buffering timer runs out, and then we get one call to UpdateSessionStore in the parent. If we flush, by calling nsFrameLoader::RequestTabStateFlush or nsFrameLoader::RequestFinalTabStateFlush we'll get one for each content process.

UpdateSessionStoreFromStorage is only called from either nsFrameLoader::RequestTabStateFlush or nsFrameLoader::RequestFinalTabStateFlush, so we'll get one call per tree of browsing contexts.

The tricky part here is all the flushing. nsFrameLoader::RequestTabStateFlush is exposed through WebIDL, and returns a promise that resolves when the tab state has been flushed (but not written to file). But I don't think that's what's happening here. I think what we're seeing is nsFrameLoader::RequestFinalTabStateFlush happening, which is called when we shut down a browser (tab). That one just sends all the flush instructions and hope for the best. No waiting or anything. And waiting on a promise isn't what we want to do here either I think.

Good news though. I think NOTIFY_BROWSER_SHUTDOWN_FLUSH might be what we're looking for. No guarantees though, but it does indeed fire when the final flush for a tab happens, and the very last update to the tab state cache has completed.

Comment 4

[:Gijs (he/him)]

Attachment: Bug 1789043 - use existing NOTIFY_BROWSER_SHUTDOWN_FLUSH on firefox view to be told about closing tabs, r?sclements

Comment 5

[:Gijs (he/him)]

(In reply to Andreas Farre [:farre] from comment #3)

Good news though. I think NOTIFY_BROWSER_SHUTDOWN_FLUSH might be what we're looking for. No guarantees though, but it does indeed fire when the final flush for a tab happens, and the very last update to the tab state cache has completed.

Hm, I thought I checked this at the time but I don't see any issues using this at the moment so I guess that'll work...

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a64f296997c
use existing NOTIFY_BROWSER_SHUTDOWN_FLUSH on firefox view to be told about closing tabs, r=sclements

Comment 7

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/7a64f296997c