Pinned tabs do not respect container settings during session restore

RESOLVED FIXED in Firefox 67
(NeedInfo from)

Status

()

defect
P2
normal
RESOLVED FIXED
2 months ago
24 days ago

People

(Reporter: cpeterson, Assigned: dthayer, NeedInfo)

Tracking

(Blocks 1 bug, {regression})

67 Branch
Firefox 67
Points:
---

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox65 unaffected, firefox66 unaffected, firefox67+ fixed)

Details

(Whiteboard: [fxperf:p1])

Attachments

(1 attachment)

(Reporter)

Description

2 months ago

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.

(Reporter)

Comment 1

2 months ago
Blocks: 1442694
Flags: needinfo?(dothayer)
Version: 16 Branch → 67 Branch
Priority: -- → P2
Duplicate of this bug: 1532631
(Assignee)

Comment 3

2 months ago

Taking a look.

Assignee: nobody → dothayer
Status: NEW → ASSIGNED
Flags: needinfo?(dothayer)
Whiteboard: [fxperf:p1]

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.

(Assignee)

Comment 5

2 months ago

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.

(Assignee)

Comment 6

2 months ago

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.

(Assignee)

Updated

2 months ago
Duplicate of this bug: 1532540
(Assignee)

Updated

2 months ago
Summary: Pinned tabs configured to "Always open in" some container open in non-pinned tabs (Multi-Account Containers) → Pinned tabs do not respect container settings during session restore

Comment 9

2 months ago

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.
(Assignee)

Updated

2 months ago
See Also: → 1532841

Comment 10

2 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/764939fcdaf3
Check userContextId when consuming preopened tabs r=Gijs

Comment 11

2 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67

Comment 12

2 months ago

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__|

(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.

Updated

2 months ago
Duplicate of this bug: 1533301

Updated

2 months ago
Duplicate of this bug: 1533264

(In reply to Pulsebot from comment #16)

Backout by dgottwald@mozilla.com:
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

24 days ago

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.

Flags: needinfo?(okazki98)
Flags: needinfo?(grepkeen)
Flags: needinfo?(cpeterson)

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.

Flags: needinfo?(okazki98)
(Reporter)

Comment 20

24 days ago

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

Flags: needinfo?(cpeterson)

Comment 21

24 days ago

(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".

Flags: needinfo?(grepkeen) → needinfo?(dao+bmo)
You need to log in before you can comment on or make changes to this bug.