Make browser/components/sessionstore/test/browser_491168.js pass with BFCache in parent
Categories
(Firefox :: Session Restore, defect, P2)
Tracking
()
Fission Milestone | M7a |
People
(Reporter: peterv, Assigned: annyG)
References
(Blocks 1 open bug)
Details
Assignee | ||
Comment 1•3 years ago
|
||
I'm looking into this.
Assignee | ||
Comment 2•3 years ago
•
|
||
To make it easier to debug the issue, I have commented out the rest of the test right away.
Observations so far:
- Right away after running this test for a few times, you might notice that sometimes it passes and sometimes it fails. In cases when it fails, the issue is that
tabState.entries[0]
is undefined here. - I added some logging and see that the expected
historychange
update does indeed happen, but in cases when we have noentries
, the update happens too late, after the test has finished, or at least after the assertion. That makes it seem that sometimes thehistorychange
update takes too long. So this was my next question, why does it take longer sometimes? - I traced down code that initiates the correct
historychange
update, and it happens to berequestTabStateFlush()
that gets called because ofawait TabStateFlusher.flush(browser);
in the test. - So following the previous theory, how can it be that sometimes this update "takes longer" if we await for this promise? So it looks like the promise that we return from
TabStateFlusher.flush
is
Promise.race([
nativePromise.then(_ => requestPromise),
request.cancelPromise,
]);
- I decided to add debug prints to places where might either resolve
requestPromise
orrequest.cancelPromise
, which was inresolve()
and inresolveAll()
. The latter is used used for "when the content process crashed or the final update message was seen". When the test failed the next time, I saw thatresolveAll
has been called first. It was called fromonFinalTabStateUpdateComplete
, which at some point was triggered by a call here within when message "browser-shutdown-tabstate-updated" was received. This message is sent fromWindowGlobalParent::NotifySessionStoreUpdatesComplete
, which was called bynsFrameLoader::RequestFinalTabStateFlush
(but not invoked by JS code). When I printed out the bc, it was one belonging to the previous navigation, about:blank. I also checked if that bc's current window global was equal tothis
and it was not (GetBrowsingContext()->GetCurrentWindowGlobal() == this
). Both of these things indicate that this is a session store update call that was corresponding to a previous navigation, and is not for the current navigation. The test passes if I add a change withinWindowGlobalParent::NotifySessionStoreUpdatesComplete
to also check forIsCurrentGlobal
here. Going to see if try looks good for other tests as well.
Assignee | ||
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Comment 3•3 years ago
|
||
Unfortunately, checking if we check if the wgp is the current window global before sending "browser-shutdown-tabstate-updated", we have incorrect behaviour in cases when are removing a tab and expect pertinent updates to go through, e.g. here.
Maybe we don't need the above check, but instead need to augment resolveAll
code (and perhaps other code too) in TabStateFlusher.jsm to take into account that a call to resolveAll
for a specific browser might not be made by the latest session store update. Perhaps, when issuing message "browser-shutdown-tabstate-updated", WindowGlobalParent should also send along it's associated browsing context's id, so that in resolveAll
we can see if we should ignore the update.
Updated•3 years ago
|
Updated•3 years ago
|
Comment 4•3 years ago
|
||
Does bug 1716444 happen to help here?
Description
•