1956101 - contentSearchHandoffUI.js depends on messaging to determine whether it's running in private browsing and therefore ends up requesting newtab strings in PB

Status: RESOLVED FIXED

(Firefox :: Search, defect, P3)

Description

[:Gijs (he/him)]

In bug 1889417 we saw lots of errors like this:

 0:38.83 GECKO(29056) [fluent] Missing message in locale en-US: newtab-search-box-input
 0:38.83 GECKO(29056) [fluent] Missing message in locale en-US: newtab-search-box-text
 0:38.83 GECKO(29056) [fluent] Couldn't find a message: newtab-search-box-input
 0:38.83 GECKO(29056) [fluent] Couldn't find a message: newtab-search-box-text
 0:38.84 GECKO(29056) [dom/l10n] Errors during l10n mutation frame.

in a wide variety of tests, including e.g. https://searchfox.org/mozilla-central/source/browser/base/content/test/general/browser_bug763468_perwindowpb.js

Eemeli thought this was due to tests opening new tab in private browsing, and it stopped us enabling more informative / "harder" error reporting for fluent issues.

Fast forward a few months/years, and I am looking at making all console errors (not just fluent-related ones) cause test failures and this is once again coming up as a really noisy warning.

So I did some debugging, and I initially thought that maybe fluent was complaining about localizations where it was unable to fully load ftl files due to the page getting unloaded or something. But AFAICT the truth is much more prosaic. There is simply a bug in contentSearchHandoffUI.js or the messaging code that informs it, which leads to it sometimes using non-private browsing strings in about:privatebrowsing.

Specifically, for some reason the code there relies on a private boolean which is updated when it receives an "Engine" event.

It relies on that boolean (and thus that message) to make the right l10n decision in _updatel10nIds, for 6 different strings: https://searchfox.org/mozilla-central/rev/3a0ca3dffd7ccf74a53066a097739f24dd8b6b10/browser/components/search/content/contentSearchHandoffUI.js#85,91,98,104,111,120 .

However, we can end up in _updatel10nIds for about:privatebrowsing documents but with _isAboutPrivateBrowsing set to false, with this stack (which is unfortunately not very informative; the test continues so I'm struggling to get this caught in a debugger and really look around much).

_updatel10nIds (chrome://browser/content/contentSearchHandoffUI.js#90)
_updateEngine (chrome://browser/content/contentSearchHandoffUI.js#75)
_onMsgEngine (chrome://browser/content/contentSearchHandoffUI.js#35)
handleEvent (chrome://browser/content/contentSearchHandoffUI.js#21)
_fireEvent (resource:///actors/ContentSearchChild.sys.mjs#31)
receiveMessage (resource:///actors/ContentSearchChild.sys.mjs#18)

Bug 1722395 talks about the need to get this correct in the extended commit message because, as it notes:

about:newtab doesn't load aboutPrivateBrowsing.ftl and conversely about:privatebrowsing doesn't load newtab.ftl. Since permanent private browsing mode uses about:newtab as its new tab, we need to make sure we load our strings from newtab.ftl in that case.

I'm not really sure why about:privatebrowsing is getting messages with isAboutPrivateBrowsing set to false. That seems bad.

In terms of fixing this though, it would seem that much the easiest fix would be to have the code actually check whether document.location.href is either about:privatebrowsing? I don't know why it doesn't do that. Maybe someone else has context?

Comment 1

[:Gijs (he/him)]

Meant to needinfo for comment 0.

Comment 2

[:Gijs (he/him)]

OK, actually the parent uses:

      state.isAboutPrivateBrowsing =
        window.gBrowser.currentURI.spec == "about:privatebrowsing";

to determine this, so I think the code is just racy and can fail if currentURI in the parent hasn't yet been updated and then gets sent to the child.

Comment 3

[:Gijs (he/him)]

Attachment: Bug 1956101 - use child context for private browsing determination in ContentSearch code, r?#search-reviewers

Also updating to use a sprinkling more modern JS and getting PB info directly from the actor's window global.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ab971ce38388
use child context for private browsing determination in ContentSearch code, r=Standard8

Comment 5

[Cristina Horotan [:chorotan]]

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