Closed Bug 1532498 Opened 2 years ago Closed 2 years ago

Pinned tabs do not respect container settings during session restore

Categories

(Firefox :: Tabbed Browser, defect, P2)

67 Branch
defect

Tracking

()

RESOLVED FIXED
Firefox 67
Tracking Status
firefox-esr60 --- unaffected
firefox65 --- unaffected
firefox66 --- unaffected
firefox67 + fixed

People

(Reporter: cpeterson, Assigned: dthayer, NeedInfo)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [fxperf:p1])

Attachments

(1 file)

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.

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

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.

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.

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.

Duplicate of this bug: 1532540
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

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.
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/764939fcdaf3
Check userContextId when consuming preopened tabs r=Gijs
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67

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.

Duplicate of this bug: 1533301
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.

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)

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)

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