1886753 - Broken context menu in sidebar due to missing translations fluent reference

Status: RESOLVED FIXED

(Firefox :: Translations, defect, P3)

Description

[:Gijs (he/him)]

Attachment: Screenshot

Starting nightly with a webextension installed that exists in the sidebar (e.g. Tree Style Tabs) produces warnings in the browser console (see attachment). I expect that if I enable whatever pref/nimbusfeature/whatever bug 1870305 is implemented behind, it would show empty menuitems or crash as the fluent message is not present.

For this to not break, browser/locales-preview/select-translations.ftl needs to be loaded anywhere nsContextMenu.js is loaded/invoked. As the warnings show, this should at least include webext-panels.xhtml.

I'm a little surprised this wasn't picked up on infra/tests - missing messages are supposed to crash the browser when encountered in automated tests. Is the feature disabled in tests generally, or something?

(seen on macOS but I expect this is a cross-platform bug)

Comment 1

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

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

:nordzilla, since you are the author of the regressor, bug 1870305, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 2

[Erik Nordin [:nordzilla]]

Thanks for reporting this, Gijs.

I'll go ahead and take this bug as the author of the regressor.

---

I'm a little surprised this wasn't picked up on infra/tests - missing messages are supposed to crash the browser when encountered in automated tests. Is the feature disabled in tests generally, or something?

I'm also surprised this wasn't caught by any tests. The code isn't disabled in tests. The only determination regarding whether those menu items should show up or not is right here in nsContextMenu.js.

---

For this to not break, browser/locales-preview/select-translations.ftl needs to be loaded anywhere nsContextMenu.js is loaded/invoked. As the warnings show, this should at least include webext-panels.xhtml.

If I'm understanding you correctly, instead of using document.l10n to localize the values like I am using it here, I should instead try to create a Localization object within a try block, such as this example in the same file?

Comment 3

[:Gijs (he/him)]

(In reply to Erik Nordin [:nordzilla] from comment #2)

For this to not break, browser/locales-preview/select-translations.ftl needs to be loaded anywhere nsContextMenu.js is loaded/invoked. As the warnings show, this should at least include webext-panels.xhtml.

If I'm understanding you correctly, instead of using document.l10n to localize the values like I am using it here, I should instead try to create a Localization object within a try block, such as this example in the same file?

That could be a solution but isn't ideal because of the imperative nature of the code, when in general fluent works better with declarative (ie markup-based) localization annotations. It looks like the example you linked only works the way it does because it used to use .properties stringbundles (which are always imperative and don't have a markup equivalent), and when the code was converted to use fluent in bug 1791178 we didn't insist on also changing the way the localization was sourced.

document.l10n will use all the fluent files loaded (via link elements) in the document that document is pointing to.

nsContextMenu.js runs in browser windows, so it'll pull strings from all the ftl files that are loaded in the browser.xhtml document. That works, because browser/locales-preview/select-translations.ftl is loaded here.

The breakage happens when the same code runs in webext-panels.xhtml, which does not have a similar <link> tag, but does load browser.js and thus nsContextMenu.js (and includes the same context menu). So the "trivial" solution would be adding a similar link tag in that document.

Another option would be using insertFTLIfNeeded from the code that relies on this string, something like https://searchfox.org/mozilla-central/source/browser/actors/SpeechDispatcherParent.sys.mjs#58 from the nsContextMenu.js code. However, that will only work for the dynamically assigned l10n id in the code in nsContextMenu.js you linked, not for the one that is in the markup (which will be present and cause issues before any of the runtime code from nsContextMenu.js has a chance to dynamically insert the right ftl file).

Hopefully that makes sense?

Comment 4

[Erik Nordin [:nordzilla]]

Attachment: Bug 1886753 - Include select-translations.ftl for webext context menu

Comment 5

[Erik Nordin [:nordzilla]]

Thank you for the deeper explanation.

I pushed up a WIP patch with the fix, but I have yet to devote time to exploring a test for this.

I could try to create a test that attempts to open a webext context menu and checks for fluent warnings, but that is only a band-aid over this symptom.

I don't feel that I have the cycles at the moment to devote diving into the L10n infrastructure and figuring out why this wasn't caught in a general way.

What are your thoughts about this?

I could land the fix as-is and file a follow-up bug for the L10n code.
Or if you feel that a generalized fix would be simple and already have a direction in mind, I'm happy to take a look.

Comment 6

[Erik Nordin [:nordzilla]]

^ Forgot to change the need-info states for the above.

Comment 7

[:Gijs (he/him)]

I don't think it's useful to have a test specifically for this. It would be helpful to understand why I was seeing this in the browser console but we're not seeing it in tests, nor are we failing tests because of the missing message.

Luca/Eemeli, I would have thought that this would be happening in webextension tests which test the sidebar, but I can't find any mentions in relevant logs, e.g. in https://treeherder.mozilla.org/logviewer?job_id=452250897&repo=mozilla-central . Any idea why that might be? I can trivially reproduce in a clean nightly profile by installing any sidebar extension (e.g. https://addons.mozilla.org/en-GB/firefox/addon/note-sidebar-for-firefox/ or https://addons.mozilla.org/en-GB/firefox/addon/tree-style-tab/ ) and opening its sidebar.

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

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

I don't think it's useful to have a test specifically for this. It would be helpful to understand why I was seeing this in the browser console but we're not seeing it in tests, nor are we failing tests because of the missing message.

Luca/Eemeli, I would have thought that this would be happening in webextension tests which test the sidebar, but I can't find any mentions in relevant logs, e.g. in https://treeherder.mozilla.org/logviewer?job_id=452250897&repo=mozilla-central . Any idea why that might be? I can trivially reproduce in a clean nightly profile by installing any sidebar extension (e.g. https://addons.mozilla.org/en-GB/firefox/addon/note-sidebar-for-firefox/ or https://addons.mozilla.org/en-GB/firefox/addon/tree-style-tab/ ) and opening its sidebar.

Hi Gijs, it took me a few days to be able to take a better look into this, but today I've finally being able to dig a bit more into why the fluent error seems to be consistently logged when manually trying in a Nightly instance while it does never show in the test cases that would be supposed to trigger it.

The reason for the error not being logged from mochitests is due to the fact that the error is only logged on non release builds and outside of automation, while when that issue is being hit in automation the error ends up to not be logged anywhere. In particular see:

• here in mozilla::intl::MaybeReportErrorsToGecko where we are checking if running in automation and returning earlier with an InvalidStateError
• and then here in L10nReadyHandler::RejectedCallback where we are handling the promise rejection due to that error and then calling mozilla::intl::MaybeReportErrorsToGecko again to (eventually) log a more generic error if NOT running in automation, ignoring the error and resolving the outer promise as undefined

Let me know if these details are enough (or not) to determine if we should apply changes to the related logic to be able to hit this kind of failures from the automated tests covering features that happen to missing fluent translations like in this case.

(I'm also posting some more details about the investigation below, in case some more details about how I tracked it down may turn out to be useful, but feel free to ignore everything that follows in the short summary above provided already what we needed).

---

Follows some more details from the investigation, in particular how I have determined what was going on.

The first "experiment" I did to help me to determine what was going on has been to pause a test case where I was expecting to see the error logged (e.g. by adding a temporary await new Promise(() => {}) here https://searchfox.org/mozilla-central/rev/511d9c9e2eb0f2291dc2b2a5462ba054dfc2edfe/browser/components/extensions/test/browser/browser_ext_sidebarAction.js#152) and then installed manually from a local xpi file one of the extensions mentioned in comment 7, that confirmed that no log was emitted in the console neither and that it wasn't some difference between what the test extension vs the real extensions sidebars.

At that point I wanted to trace back what was the underlying sequence of calls the leads to logging that error, and so I recorded me triggering the bug using a debug build and rr, and based on the stack trace determined that the message was being logged from mozilla::intl::MaybeReportErrorsToGecko, based on a quick look to that short method it did become clear that in automation we return earlier throwing an InvalidStateError (https://searchfox.org/mozilla-central/rev/511d9c9e2eb0f2291dc2b2a5462ba054dfc2edfe/intl/l10n/Localization.h#49-52), while outside of automation if running on nightly/devedition/debug builds then we will be logging the error, otherwise we just not log anything nor failing the promise.

As a final step, I was curious about which "black hole" the promise rejection was ending, based on the stack trace I gathered from digging into the call stack from rr, I determined that the fluent localization was being triggered from DocumentL10n::TriggerInitialTranslation through a call to DocumentL10n::TranslateDocument and the promise returned by that is then going to be handled by L10nReadyHandler::RejectedCallback which is also using MaybeReportErrorsToGecko to determine if it should be logging a more generic "[dom/l10n] Could not complete initial document translation." error to the console... but in automation that is going to be returning a InvalidStateError that L10nReadyHandler::RejectedCallback is ignoring... and so that's the "black hole" where the errors we are not seeing in the tests are going, mistery solved :-)

Comment 9

[Eemeli Aro [:eemeli]]

It looks like this behaviour is effectively a regression from bug 1739143, which introduced the error swallowing "because there is nothing actionable for the user pending on mReady to do here". I agree with the original logic for the real-world use case, but my sense is that as in MaybeReportErrorsToGecko, we might want to do something different in automation.

Specifically, we could do either or both of the following:

1. Reverse the order of the IsInAutomation() and #if defined blocks in MaybeReportErrorsToGecko. The intent here would be to ensure that even a swallowed error gets logged in tests.
2. In L10nReadyHandler, reject rather than resolve if an IsInAutomation() is true. This would help ensure that tests that encounter missing data-l10n-id values will always throw, but I'm not really sure if this would have some unwanted side effects, such as those mentioned in the code comment.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c4236da910d7
Include select-translations.ftl for webext context menu r=Gijs

Comment 11

[Serban Stanca [:SerbanS]]

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

Comment 12

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

The patch landed in nightly and beta is affected.
:nordzilla, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox125 to wontfix.

For more information, please visit BugBot documentation.

Comment 13

[Erik Nordin [:nordzilla]]

Attachment: Bug 1886753 - Include select-translations.ftl for webext context menu

Original Revision: https://phabricator.services.mozilla.com/D205387

Comment 14

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: This may add unwanted console spam to any extensions, such as tree-style tabs, which attempt to load this fluent file. In a rare case, it may cause a crash, but I am not aware of any addons that actually try to access the fluent strings.
• Code covered by automated testing: no
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: Load the tree-style tabs extension and check for fluent erros in the console.
• Risk associated with taking this patch: Low risk
• Explanation of risk level: This patch adds a link to the missing ftl file that may get pulled in to context.
• String changes made/needed: None
• Is Android affected?: no

Comment 15

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/73328c39254f