1783289 - Tab bar sometimes broken because this.PictureInPicture is undefined

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Oriol Brufau [:Oriol]]

Doesn't happen much frequently, but has already happened multiple times that I run ./mach run to start my compiled firefox, but then the tabbar is broken, e.g.

• I can't switch tabs
• If I open the context menu in a tab, all entries are missing (mostly blank space) except for "Mute tab".
• If I enter customize mode, there is no item to be dragged into the toolbars

The console says:

JavaScript error: chrome://browser/content/tabbrowser.js, line 5322: TypeError: can't access property "isOriginatingBrowser", this.PictureInPicture is undefined
JavaScript error: resource://gre/modules/TerminatorTelemetry.jsm, line 76: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsITelemetry.getHistogramById]
JavaScript error: resource://gre/modules/TerminatorTelemetry.jsm, line 76: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsITelemetry.getHistogramById]
JavaScript error: resource://gre/modules/TerminatorTelemetry.jsm, line 76: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsITelemetry.getHistogramById]
JavaScript error: chrome://browser/content/tabbrowser.js, line 1913: NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIWebProgress.removeProgressListener]

https://searchfox.org/mozilla-central/rev/d28f7751c47d75699c6ab1afd4852ad84ebb7399/browser/base/content/tabbrowser.js#5322

In that state, customize mode produces things like

console.error: CustomizableUI: 
  TypeError: can't access property "getElementsByAttribute", aWindow.gNavToolbox.palette is undefined -- resource:///modules/CustomizableUI.jsm:4839
console.error: CustomizeMode: 
  Message: TypeError: can't access property "ownerGlobal", aWindowPalette is undefined
console.error: CustomizeMode: 
  Error entering customize mode
  Message: TypeError: can't access property "join", currentPlacements is undefined

I don't know why PictureInPicture is undefined, but this shouldn't break the browser UI.

I have to reload the browser to make it functional, it's annoying.

Comment 1

[Oriol Brufau [:Oriol]]

OK, so the problem seems that _gBrowser.init does this:

      this._setupInitialBrowserAndTab();
      // ...
      XPCOMUtils.defineLazyModuleGetters(this, {
        E10SUtils: "resource://gre/modules/E10SUtils.jsm",
        PictureInPicture: "resource://gre/modules/PictureInPicture.jsm",
      });

where _setupInitialBrowserAndTab contains

      browser.docShellIsActive = this.shouldActivateDocShell(browser);

which relies on PictureInPicture which has not been defined yet!

    shouldActivateDocShell(aBrowser) {
      if (this._switcher) {
        return this._switcher.shouldActivateDocShell(aBrowser);
      }
      return (
        (aBrowser == this.selectedBrowser && !document.hidden) ||
        this._printPreviewBrowsers.has(aBrowser) ||
        this.PictureInPicture.isOriginatingBrowser(aBrowser)
      );
    }

This seems to always happen, but I guess that usually shouldActivateDocShell doesn't reach this.PictureInPicture.

Comment 2

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

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

Comment 3

[Niklas Baumgardner [:niklas]]

Attachment: Bug 1783289 - Define lazy PictureInPicture before reference. r=mconley

Comment 4

[Niklas Baumgardner [:niklas]]

Thanks for reporting this!

Comment 5

[:Gijs (he/him)]

What's required to get into this state / why doesn't it happen 100% of the time?

Comment 6

[Oriol Brufau [:Oriol]]

During startup, shouldActivateDocShell is always reached before defining this.PictureInPicture.

But if this._switcher, aBrowser == this.selectedBrowser && !document.hidden or this._printPreviewBrowsers.has(aBrowser), then it doesn't matter. I don't know why these conditions may vary, though.

Possibly a timing issue? I have only observed this with my locally compiled Firefox that runs slower than released builds.

Comment 7

[:Gijs (he/him)]

I wonder if this isn't actually a regression from the bug you listed but from bug 1779559 which will have altered the outcome of document.hidden. Is that possible?

Comment 8

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #7)

I wonder if this isn't actually a regression from the bug you listed but from bug 1779559 which will have altered the outcome of document.hidden. Is that possible?

In fact, that seems likely:

aBrowser == this.selectedBrowser && !document.hidden

The initial browser is always the selected browser, so that check should always pass, and it used to be the case that document.hidden was always false for toplevel chrome documents. So that would make sense of why this changed, right? In which case this is new in 104 and we should probably track for 104.

Comment 9

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
Requesting tracking per previous comments.

Comment 10

[Oriol Brufau [:Oriol]]

That seems likely, yeah.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef58ff3d97eb
Define lazy PictureInPicture before reference. r=Gijs

Comment 12

[Niklas Baumgardner [:niklas]]

Comment on attachment 9288647 [details]
Bug 1783289 - Define lazy PictureInPicture before reference. r=mconley

Beta/Release Uplift Approval Request

• User impact if declined: This error will cause the entire toolbar to break and the only fix is restarting and hope the error doesn't occur again.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Not risky because we only moved the definition of PictureInPicture to an earlier section so PictureInPicture won't be undefined anymore.
• String changes made/needed: No
• Is Android affected?: No

Comment 13

[Cristian Tuns]

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

Comment 14

[Dianna Smith [:diannaS]]

the attached patch has conflicts when grafting to beta. I think it is because of the changes made by bug 1780017

Comment 15

[Niklas Baumgardner [:niklas]]

Attachment: bug_1783289_beta_uplift.diff

Comment 16

[Niklas Baumgardner [:niklas]]

Comment on attachment 9289450 [details] [diff] [review]
bug_1783289_beta_uplift.diff

This uplift request is to fix the conflict when trying to uplift the original to beta

Beta/Release Uplift Approval Request

• User impact if declined: This error will cause the entire toolbar to break and the only fix is restarting and hope the error doesn't occur again.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Not risky because we only moved the definition of PictureInPicture to an earlier section so PictureInPicture won't be undefined anymore.
• String changes made/needed: No
• Is Android affected?: No

Comment 17

[Dianna Smith [:diannaS]]

Comment on attachment 9289450 [details] [diff] [review]
bug_1783289_beta_uplift.diff

Approved for 104.0b9

Comment 18

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/384e76b2e90d