1357098 - Pinned tabs are not restored after browser restart

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Aleksandrs Ļedovskis]

Steps:

1. Created new profile
2. Navigated to "example.com" page
3. Pinned "example.com" tab
4. Opened new window
5. Navigated to "example.org" in a new window
6. Pinned "example.org" tab
7. Closed browser
8. Reopened browser

Result:

Browser started with two windows. Only "example.com" pinned tab has been restored in its window. Second window contains two blank tabs. "example.org" pinned tab has been lost


Expected result:

Browser starts with two windows. Both "example.com" and "example.com" are pinned and available in respective windows.

Comment 1

[Aleksandrs Ļedovskis]

Correction

> Both "example.com" and "example.com" are pinned and available in respective windows.

should be read as

> Both "example.com" and "example.org" are pinned and available in respective windows.

Comment 2

[Aleksandrs Ļedovskis]

FWIW, right after relaunch browser console contains error - "TypeError: tabAtTargetIndex is undefined" with backtrace to https://dxr.mozilla.org/mozilla-central/rev/c697e756f738ce37abc56f31bfbc48f55625d617/browser/components/sessionstore/SessionStore.jsm#3329

Comment 3

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=05ba929d3d1b5d678960a89210ead7d8d6dd4541&tochange=77a91768f10977b06436d79e4551d5c257ae4c32

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Thanks for the bug report! I guess I missed a case here. Investigating...

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1357098 - Fix off-by-one error in SessionStore when preparing deferred restore data.

Bug 588482 made it possible to restore pinned tabs in windows before restoring the rest of the
tabs. Part of this involves extracting the pinned tab data from the last session, and make it
the default state to restore. The rest can be restored later.

If one of the pinned tabs in a window were selected, the code that was preparing the default
state for that window set the selection value by calculating the length of the tabs array
(before adding the selected one), and adding 2. Presumably, 1 count is because of the tab
we're about to add, and another count is due to how the "selected" value in SessionStore
is 1-indexed (where "0" means, "don't change the selection").

This is an off-by-one error though, since the tabs array length already cancels out the
need for the extra "1-index" difference. It is sufficient to just add 1 to the length of
the tabs array to calculate the selected value.

This appears to have been a bug for a while, but was covered up by the fact that the old
tab selection code exited early if the 0-indexed value of the selected tab was not within
the length of the tabs array. Bug 1351677 uncovered this bug by removing that second check.

This patch fixes the off-by-one error, and also puts a check back in to ensure that the tab
we're going to select is at a valid index.

Review commit: https://reviewboard.mozilla.org/r/133656/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/133656/

Comment 6

[Mike Conley (:mconley) (:⚙️)]

I thought it'd be straight-forward to write a test for this. I was wrong. It appears as if _prepDataForDeferredRestore is only ever called when initting the session, and I don't believe I can trigger that from a Mochitest. That leaves Firefox UI Tests. Unfortunately, I can't seem to pin tabs from that framework. :/

Comment 7

[Ryan VanderMeulen [:RyanVM]]

(In reply to Mike Conley (:mconley) - PTO until April 27th. from comment #6)
> That leaves Firefox UI Tests. Unfortunately, I can't seem to pin tabs from that
> framework. :/

Any thoughts on that, Henrik?

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8861666 [details]
Bug 1357098 - Fix off-by-one error in SessionStore when preparing deferred restore data.

https://reviewboard.mozilla.org/r/133656/#review136670

Well, kudos for finding this, Mike. Thanks!

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #7)
> (In reply to Mike Conley (:mconley) - PTO until April 27th. from comment #6)
> > That leaves Firefox UI Tests. Unfortunately, I can't seem to pin tabs from that
> > framework. :/

We don't have an implementation yet which uses the UI to pin a tab. But if calling a GeckoAPI is fine in this case it wouldn't matter. You can just do it. There is no restriction in that you only have to use the provided classes from Firefox Puppeteer.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df0562354e0b
Fix off-by-one error in SessionStore when preparing deferred restore data. r=mikedeboer

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/df0562354e0b

Comment 12

[Fahima Zulfath]

The issue is no longer reproducible on Firefox Nightly 55.0a1 (build ID: 20170503030212).
Test were performed under Windows 10x64.
[bugday-20170503]

Comment 13

[alex_mayorga]

¡Hola Fahima!

Thanks on verifying.

¡Gracias!
Alex