1532498 - Pinned tabs do not respect container settings during session restore

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P2)

Description

[Chris Peterson [:cpeterson]]

STR:

1. Install "Firefox Multi-Account Containers" extension: https://addons.mozilla.org/en-US/firefox/addon/multi-account-containers/
2. Open some pages in a container.
3. Set those sites to always open in that container.
4. Open more pages on those sites so the Multi-Account Containers extension will confirm that you want to keep opening those sites in that container.
5. Pin the tabs with those contained pages.
6. Restart Firefox.

RESULT:
Pinned tabs that are configured to always open in a specific container open in non-pinned tabs instead of pinned tabs.

Comment 1

[Chris Peterson [:cpeterson]]

@ Doug, I think this is a regression from your fix for pinned tab session restore bug 1442694 in this pushlog:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fd67a4332060ea5389d448b3453ea23064e50b5e&tochange=8602628e7edaecadea855a64179b329da6ff1f20

Comment 3

[Alex Thayer [:alexical] (she/her)]

Taking a look.

Comment 4

[Martin Thomson [:mt:]]

I had something like the experience Chris describes, except weirder.

I have 5 container tabs pinned.

The first two, which are not set to always open in the container, switched to non-container.

The next two, which are set to always open in the container, just disappeared.

The last one, which is also set to open in the container, became unpinned.

I hope that helps narrow the problem down better.

Comment 5

[Alex Thayer [:alexical] (she/her)]

Looking into this and hoping to get a fix up for review by this evening. I don't think it's necessary to roll back
1442694 just yet if I can get a fix out very soon, since it's only affecting the intersection of containers, pinned tabs, and session restore, which isn't overwhelmingly common. Still common enough though that I am prioritizing a fix.

Comment 6

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1532498 - Check userContextId when consuming preopened tabs r?Gijs

This will fix the behavior, I think it might still be worth deliberating
over whether we want to consider non-default userContextId's when tracking
the number of pinned tabs or not. This will be visually correct - the
number of pinned tabs won't change, so things won't be moving around in
the tab bar, but we will throw away the preopened tab once we get further
down in SessionStore.jsm, which is less than ideal.

Comment 7

[Alex Thayer [:alexical] (she/her)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a183a0bc05fb3b1be8650e3deb1bdeccd1c5584a

Comment 9

[John Gage]

Sorry if someone already mentioned this, but if you perform the following, there is also interesting new behavior that discloses some information about the normal session:

1. Have startup session restore enabled.
2. Close Nightly.
3. Start Nightly in Private browsing mode session.
4. Pinned tabs from the normal session get drawn before the private about:newtab appears.
5. The pinned tabs are blank and don't appear to lazy load anything as clicking on them does not load any URI, but the quantity of pinned tabs does match what was pinned in the normal session.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/764939fcdaf3
Check userContextId when consuming preopened tabs r=Gijs

Comment 11

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/764939fcdaf3

Comment 12

[John Gage]

Looks like this patch has fixed the issue, but may have introduced some new undesirable behavior. I have never opened a new bug, and I'm afraid to mess it up, so I'm posting this here and hoping it helps.

STR:

1. Have normal browsing session open with:
   a) Pinned tabs (containerized)
   b) Pinned tabs (non-containerized)
   c) Normal tabs
   b) Session restore on startup enabled
2. Have normal tab selected/active.
3. Exit Nightly session.
4. Start Nightly.

Result: Non-containerized pinned tabs are moved left before all other containerized pinned tabs. Proper ordering is maintained among the tabs with the exception of the segregation based on containerization. This does not occur if a pinned tab is active at exit, only when a normal tab is active.

Key:
|N#| = Pinned tab (Non-containerized)
|C#| = Pinned tab (Containerized)
|__N__| = Normal tab
(A) = Active tab

Example #1:
Tabs before exiting session w/ session restore enabled:
|C1|C2|C3|C4|N1|C5|N2|C6|__N__|__N(A)_|

Tabs on next launch (Order rearranged):
|N1|N2|C1|C2|C3|C4|C5|C6|__N__|__N(A)_|

Example #2:
Tabs before exiting session w/ session restore enabled:
|C1|C2|C3|C4|N1|C5|N2(A)|C6|__N__|__N__|

Tabs on next launch (Order maintained):
|C1|C2|C3|C4|N1|C5|N2(A)|C6|__N__|__N__|

Comment 13

[Dan Ceban [:okazki98]]

(In reply to John Gage from comment #12)

Looks like this patch has fixed the issue, but may have introduced some new undesirable behavior. I have never opened a new bug, and I'm afraid to mess it up, so I'm posting this here and hoping it helps.

STR:

1. Have normal browsing session open with:
   a) Pinned tabs (containerized)
   b) Pinned tabs (non-containerized)
   c) Normal tabs
   b) Session restore on startup enabled
2. Have normal tab selected/active.
3. Exit Nightly session.
4. Start Nightly.

Result: Non-containerized pinned tabs are moved left before all other containerized pinned tabs. Proper ordering is maintained among the tabs with the exception of the segregation based on containerization. This does not occur if a pinned tab is active at exit, only when a normal tab is active.

Key:
|N#| = Pinned tab (Non-containerized)
|C#| = Pinned tab (Containerized)
|__N__| = Normal tab
(A) = Active tab

Example #1:
Tabs before exiting session w/ session restore enabled:
|C1|C2|C3|C4|N1|C5|N2|C6|__N__|__N(A)_|

Tabs on next launch (Order rearranged):
|N1|N2|C1|C2|C3|C4|C5|C6|__N__|__N(A)_|

Example #2:
Tabs before exiting session w/ session restore enabled:
|C1|C2|C3|C4|N1|C5|N2(A)|C6|__N__|__N__|

Tabs on next launch (Order maintained):
|C1|C2|C3|C4|N1|C5|N2(A)|C6|__N__|__N__|

Can confirm this issue. However, in my case, the pinned tabs are being reordered no matter which pinned tab is Active.

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/be70692904a1
Backed out changeset 764939fcdaf3

Comment 17

[Dão Gottwald [:dao]]

(In reply to Pulsebot from comment #16)

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/be70692904a1
Backed out changeset 764939fcdaf3

This was part of the backout of bug 1442694, see bug 1442694 comment 53. This bug can remain resolved as it was originally introduced by bug 1442694.

Comment 18

[Victor Carciu]

Verified as fixed for all scenarios described above on Win10x64, macOS 10.13.6 using latest Beta and Nightly builds. I also can confirm that I managed to reproduce the issues using https://ftp.mozilla.org/pub/firefox/nightly/2019/03/2019-03-05-09-27-47-mozilla-central/.

I was not able to reproduce the problem from https://bugzilla.mozilla.org/show_bug.cgi?id=1532498#c12 after the fix so please confirm that the problem is solved for you too.

Comment 19

[Dan Ceban [:okazki98]]

Verified on 67.0b6 (20190328152334) and 68.0a1 (20190329094433). The browser handles the pinned tabs (including the contained ones) as expected. Executed different use cases and did not run into any issue.

Comment 20

[Chris Peterson [:cpeterson]]

The problem I reported in comment 0 is fixed. I'm not familiar with the tab-ordering issue John Gage described in comment 12.

Comment 21

[John Gage]

(In reply to Victor Carciu from comment #18)

I was not able to reproduce the problem from https://bugzilla.mozilla.org/show_bug.cgi?id=1532498#c12 after the fix so please confirm that the problem is solved for you too.

I believe Dão backed out the "preopen pinned tab on session restore" commits that caused all of this in the first place and the goal was to revisit adding it back in later with Doug Thayer and Gijs. This was inferred from bug 1442694 comment 53 and previous comments. I don't want my inference to be taken as fact, so I hope Dão can clarify on the plans for "preopening of pinned tabs on session restore".