1848877 - Tabs List blank if getPublicIdentityFromId() returns undefined for tab.userContextId

Status: RESOLVED FIXED

(Firefox :: Menus, defect, P1)

Description

[jscher2000]

Attachment: Images of broken Tabs list and tab title from the thread

In the fediverse, a user reported the Tabs List was not populating. Eventually, this was traced to a tab that appears to show the separator that precedes the name of the tab's container, but there is no name there. The Browser Console showed:

TypeError: lazy.ContextualIdentityService.getPublicIdentityFromId(...) is undefined3 TabsList.sys.mjs:323:40
_createRow resource:///modules/TabsList.sys.mjs:323
_populate resource:///modules/TabsList.sys.mjs:110
_populate resource:///modules/TabsList.sys.mjs:275
handleEvent resource:///modules/TabsList.sys.mjs:254
dispatchCustomEvent resource:///modules/PanelMultiView.sys.mjs:189
dispatchCustomEvent resource:///modules/PanelMultiView.sys.mjs:1484
_blockersPromise resource:///modules/PanelMultiView.sys.mjs:223

https://fedia.io/m/firefox/t/200278/FIXED-List-all-tabs-button-won-t-show-any-tabs

I don't know what caused there to be no container associated with tab.userContextId in this case. Closed container? Corrupted tab data? Whatever the cause, it would be useful to fail more gracefully here in case it is more than a once in a lifetime occurrence:

      let identityColor =
        "identity-color-" +
        lazy.ContextualIdentityService.getPublicIdentityFromId(
          tab.userContextId
        ).color;

https://searchfox.org/mozilla-central/source/browser/modules/TabsList.sys.mjs#320

P.S. I have no idea how to replicate this problem.

Comment 1

[Danny Colin [:sdk]]

From the original author:

So I just found out the issue was some tabs that had a " --" at the end of the title probably caused by some addon made this issue happen. It
probably isn't really a - but I don't know what character is that.

All you have to do is just copy the url, open a new tab, paste the url and go. Close the previous tab with " --" at the end.
Just make sure you are not opening a new tab with CTRL+(clicking the + icon) from the "broken" tab with the " --".

Comment 2

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

Set release status flags based on info from the regressing bug 1814542

Comment 3

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

Set release status flags based on info from the regressing bug 1814542

Comment 4

[Danny Colin [:sdk]]

Attachment: Bug 1848877 - Get the container class from `tab` instead of ContextualIdentity.sys.mjs r?mconley

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Some em-dash-ending page.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

I can't repro this, not even with containers etc.

Comment 7

[jscher2000]

Attachment: Session restore before-and-after removing one custom context from containers.json

I strongly disagree with the hypothesis that this is caused by a problem with the page title. The text of the page title does not appear to be relevant to the script error identified in comment 0:

TypeError: lazy.ContextualIdentityService.getPublicIdentityFromId(...) is undefined

Here are two ways to replicate the error (back up the relevant files if it's a working profile):

(1) In a profile with custom containers defined, edit the containers.json file to remove one of the defined containers before restoring the previous session. In the attached before-and-after, I deleted the following for the test:

,{"userContextId":7,"public":true,"icon":"circle","color":"pink","name":"Google Searches"}

(Apologies for not disabling the userChrome.css file in this test profile, but hopefully it is clear enough.)

(2) Copy/paste the sessionstore.jsonlz4 file from a profile with custom containers into a different profile that doesn't have any such containers.

Comment 8

[Charles Darwin]

So i had the same issue for the longest time.
Its caused by an interaction of temporary containers and Firefox Multi account containers!
Every tab opened without being part of a container will just have -- at the end every tab while a container will be --youtube or --reddit or the likes at the end. Closing all the non container tabs (that have --) at the end fixes the issue.
This is also how you can reproduce it. Hope this will be fixes as its annoying and i like using containers. As a workaround you can make every new tab be part of a random temp container.

Comment 10

[:Gijs (he/him)]

Danny, are you still working on the patch here?

Comment 11

[Danny Colin [:sdk]]

:Gijs I tried to reproduce it with the information from this thread without any success. I suspect that it might be because it you lazy but I don't see how to debug without experiencing the bug myself. I'd still like to help even tho I don't have a lot of free time currently. However, if you'd like this to be fixed asap, feel free to re-assign it to someone else.

Comment 12

[jscher2000]

With apologies if this is not the Mozilla way, and keeping in mind that I'm not a programmer:

Could you just try/catch the code related to the .color property as a Band-Aid for now and punt on detecting the source of the problem?

if (tab.userContextId) {
  try {
    let identityColor =
      "identity-color-" +
      lazy.ContextualIdentityService.getPublicIdentityFromId(
        tab.userContextId
      ).color;
    button.classList.add(identityColor);
  } catch(e) {}
  button.classList.add("all-tabs-container-indicator");
}

Comment 13

[Danny Colin [:sdk]]

:jscher2000 This will create a new bug where the two list can be out-of-sync.

I tought about a different approach and pushed a revised patch that gets the information from the tab itself instead of the container module. I'm not sure if there's any performance downside but if it's an acceptable solution, It should prevent the current bug.

Comment 14

[:Gijs (he/him)]

(In reply to Danny Colin [:sdk] from comment #13)

:jscher2000 This will create a new bug where the two list can be out-of-sync.

I tought about a different approach and pushed a revised patch that gets the information from the tab itself instead of the container module. I'm not sure if there's any performance downside but if it's an acceptable solution, It should prevent the current bug.

I think the new patch looks great. I'd probably keep tab.userContextId as a property check, but otherwise this fix makes sense to me. Thanks for working on this!

Comment 15

[Danny Colin [:sdk]]

I updated the patch and it should be ready for review.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ddaaa4590a82
Get the container class from `tab` instead of ContextualIdentity.sys.mjs r=Gijs

Comment 17

[Cristian Tuns]

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