1822854 - `Restore Previous Session` does not restore normal tab if pinned tab is existed

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect, P1)

Description

[Alice0775 White]

[Tracking Requested - why for this release]: Session cannot be restored.

Steps to reproduce:

1. Open Websites in several tabs
2. Pin a tab
3. Restart browser
4. Menu History > Restore previous session

Actual Results:
Normal tabs are not restored.

Expected Results:
Normal tabs should be restored.

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=2d55d82d13a852aad3d3800af7b21c53eb5edfad&tochange=21534b5fb9d330b94828069e9af2f6e418c0b403

Comment 1

[Alice0775 White]

Browser console shows the following error when Restore Previous Session is performed.

Failed to create tab tabbrowser.js:2889:17
TypeError: can't access property "lastAccessed", existingTabEl is undefined
    _ensureNoNullsInTabDataList resource:///modules/sessionstore/SessionStore.sys.mjs:4705
    setTabState resource:///modules/sessionstore/SessionStore.sys.mjs:3171
    ss_setTabState resource:///modules/sessionstore/SessionStore.sys.mjs:322
    addTab chrome://browser/content/tabbrowser.js:2862
    addTrustedTab chrome://browser/content/tabbrowser.js:2577
    addMultipleTabs chrome://browser/content/tabbrowser.js:3069
    ssi_restoreWindow resource:///modules/sessionstore/SessionStore.sys.mjs:4372
    _restoreWindowsFeaturesAndTabs resource:///modules/sessionstore/SessionStore.sys.mjs:4537
    _restoreWindowsInReversedZOrder resource:///modules/sessionstore/SessionStore.sys.mjs:4561
    ssi_restoreLastSession resource:///modules/sessionstore/SessionStore.sys.mjs:3832
tabbrowser.js:2890:17
TypeError: can't access property "hidden", tab is null tabbrowser.js:3105:15

Comment 2

[:Gijs (he/him)]

It's incredible to me that this somehow (a) has taken 2 weeks to be reported and (b) apparently there is no test coverage for this case. :-\

Comment 3

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #2)

It's incredible to me that this somehow (a) has taken 2 weeks to be reported and (b) apparently there is no test coverage for this case. :-\

To be clear, not at all a sleight on this report - but I'm confused about this not coming up sooner. Alice, does this generally affect any session restore with pinned tabs, or did it maybe start breaking intermittently and take time to run into? Or does it only happen for manual restores?

Comment 4

[Alice0775 White]

Yes, the problem occurs when doing the manual restores. And the problem is always reproduced.

All tabs (pinned and normal tabs) are restored after restart browser if enabled Open previous windows and tabs in about:preferences.
The average user probably has this option enabled. So, maybe nobody has found the problem.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The bug is marked as tracked for firefox112 (beta) and tracked for firefox113 (nightly). However, the bug still isn't assigned.

:cbellini, could you please find an assignee for this tracked bug? Given that it is a regression and we know the cause, we could also simply backout the regressor. If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit auto_nag documentation.

Comment 6

[:Gijs (he/him)]

I'm sorry I haven't yet got a patch up here - there are a number of wallpaper-y fixes I could apply, but:

• I'm struggling to come up with an automated test that reproduces the problem (I can reproduce manually just fine, it's just getting the test to do this automatically that is apparently not trivial)
• using the JS debugger to step through session restore steps when manually reproducing makes the problem go away (so there's some race condition and/or debugger weirdness)
• the manual restore step is quite... odd... compared to other paths through session restore (in particular, we split the saved session into "pinned tabs that get restored immediately" and "everything else"), which is presumably how this didn't get picked up on existing tests.

I'd really like to have a more thorough understanding + automated test...

Comment 7

[Oriol Brufau [:Oriol]]

While the debugger seems to affect the outcome, I could see that there are some things broken things here, like _ensureNoNullsInTabDataList assuming that changedTabPos will not be undefined, but it can be when tabs are created with batchInsertingTabs = true, so _tPos is not set at that point.

Comment 8

[:Gijs (he/him)]

(In reply to Oriol Brufau [:Oriol] from comment #7)

While the debugger seems to affect the outcome, I could see that there are some things broken things here, like _ensureNoNullsInTabDataList assuming that changedTabPos will not be undefined, but it can be when tabs are created with batchInsertingTabs = true, so _tPos is not set at that point.

Sure, like I said, I have wallpapery fixes. But I'd rather understand the whole house of cards a bit better before I add another card.

Comment 9

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #8)

(In reply to Oriol Brufau [:Oriol] from comment #7)

While the debugger seems to affect the outcome, I could see that there are some things broken things here, like _ensureNoNullsInTabDataList assuming that changedTabPos will not be undefined, but it can be when tabs are created with batchInsertingTabs = true, so _tPos is not set at that point.

Sure, like I said, I have wallpapery fixes. But I'd rather understand the whole house of cards a bit better before I add another card.

To elaborate a bit: what else breaks because _tPos isn't set, and is that maybe the bit that wants fixing, rather than the assumption in _ensureNoNullsInTabDataList?

Comment 10

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #9)

To elaborate a bit: what else breaks because _tPos isn't set, and is that maybe the bit that wants fixing, rather than the assumption in _ensureNoNullsInTabDataList?

Straight after calling this, setTabState calls restoreTab, which also assumes _tPos is valid, so logging there yields:

console.error: "Updating tab with _tPos: " (void 0)

That's Really Not Good. :-(

Comment 11

[:Gijs (he/him)]

Attachment: Bug 1822854 - ensure session store doesn't assume _tPos is set and has good stub info for recently restored tabs so we don't break manual session restore, r?dao,Standard8

Either of these changes (ie dropping the setTabState call for batch restored
tabs, or ensuring the restoreTabs code correctly fills its array with dummy
entries) is sufficient here. I chose to do both because I think in both cases
the brokenness is not limited to this scenario or the issues at hand.

Specifically, the setTabState call was added in bug 1521346 to deal with
moved lazy tabs, but is now being invoked for session restore because of
the batchInsertingTabs optimization work. It doesn't actually need to be,
as far as I can tell, and the lacking _tPos in this case (because we don't
insert the tab into the tabstrip a few lines above) is what breaks things
inside _ensureNoNullsInTabDataList. Note that this already was breaking
things in restoreTab(), which would assign into tabs[undefined] on the
window state object, so just dropping the call seemed better than wallpapering
the absence of _tPos.

The restoreTabs code, pre-patch, calls _ensureNoNullsInTabDataList but that
will never do anything, because right before calling it we change the array
length, so maxPos was always smaller than the size of the list. This meant
we still had empty slots in the array, which was also causing confusion down
the line.

I added the explicit exception for the broken _tPos in restoreTab so that we
notice any future issues with this more quickly. Doing so without any of the
other fixes broke the pre-existing browser_586068-apptabs.js test, so
hopefully that will catch any future changes that break the code's assumptions.

Comment 12

[:Gijs (he/him)]

Attachment: Bug 1822854 - add marionette test for manual session restore while a pinned tab is present, r?dao,Standard8

Depends on D173070

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/613d402b20d2
ensure session store doesn't assume _tPos is set and has good stub info for recently restored tabs so we don't break manual session restore, r=dao
https://hg.mozilla.org/integration/autoland/rev/b45164ede017
add marionette test for manual session restore while a pinned tab is present, r=dao

Comment 14

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/613d402b20d2
https://hg.mozilla.org/mozilla-central/rev/b45164ede017

Comment 15

[:Gijs (he/him)]

Comment on attachment 9324055 [details]
Bug 1822854 - add marionette test for manual session restore while a pinned tab is present, r?dao,Standard8

Beta/Release Uplift Approval Request

• User impact if declined: Broken manual session restore if the session has pinned tabs
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment 0. Would be good to "kick the tires" wrt session restore a bit on top of that.
• List of other uplifts needed: N/A but will doublecheck if I need a beta patch due to the refactor from bug 1822072
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Medium because it's pretty clear we have several different codepaths through session restore and they're not as well-tested as we'd like them to be, but they do impact critical functionality.

The less risky alternative would be backing out the performance fix from bug 1763279 from beta. Honestly I can see arguments either way.

• String changes made/needed: No
• Is Android affected?: No

Comment 16

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #15)

• List of other uplifts needed: N/A but will doublecheck if I need a beta patch due to the refactor from bug 1822072

Seems the refactor bit was maybe fine but bug 1819794 was not.

Honestly at this point I think the less risky approach is backing bug 1763279 out of beta. It's a shame about the performance penalty for large sessions but session restore is finnicky enough that I'd rather live with the known perf liability than run more risk completely breaking things for end users.

Comment 17

[Dianna Smith [:diannaS]]

Comment on attachment 9324054 [details]
Bug 1822854 - ensure session store doesn't assume _tPos is set and has good stub info for recently restored tabs so we don't break manual session restore, r?dao,Standard8

Will back out bug 1763279 out of 112.0b6 instead

Comment 18

[Dianna Smith [:diannaS]]

-fixed by backout of bug 1763279 in fx112.0b6
-patches remain in fx113 nightly

Comment 19

[Hani Yacoub, Desktop QA]

Verified as fixed on Windows 10 x64, macOS 11.6 and on Ubuntu 20.04 x64.