1857622 - Race condition when data-l10n attributes are updated when customize mode is in the foreground

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Tim Giles [:tgiles]]

It appears that L10nMutations reliance on nsRefreshDriver can cause a race condition when the customize mode page is in the foreground causing background tabs to lose the ability to dynamically update their strings. This bug comes from https://bugzilla.mozilla.org/show_bug.cgi?id=1856572#c11 where :mconley found the underlying issue. (If this needs to be duplicated back to the blocking bug, feel free to do so).

STR:

1. Navigate to Firefox View
2. Right-click in the toolbar and select Customize Toolbar
3. Navigate back to Firefox View and either open the migration wizard (if available) or go to Open Tabs, open the submenu, and hover/activate the "Move Tab" option

ER:

1. Strings are translated correctly

AR:

1. Translated strings are missing

Additional STR from the blocked bug (works on Mac):

1. Visit about:preferences, and scroll to the "Tabs" section
2. Open the devtools for the page, and paste in this script and press enter:

let changeCount = 0;
let node = document.querySelector("#warnOnQuitKey");
setInterval(() => {
  node.setAttribute("data-l10n-args", JSON.stringify({"quitKey":changeCount}))
  changeCount++;
}, 1000);

1. Notice that the "Confirm before quitting" string is having a number increment once a second.
2. Here's where experience playing Guitar Hero comes in - you have to time it so that you enter customize mode just as the number is about to change.
3. Exit customize mode, and notice how the number has stopped incrementing. If you open up the DevTools, however, you'll notice that the data-l10n-args attribute is being updated still. This might take a few tries, but I can do it fairly consistently with one or two attempts.

I think this bug belongs here but please move to a more appropriate spot as needed.

Comment 1

[:Gijs (he/him)]

Actually, I suppose this bug has always existed, bug 1842809 just made it obvious with customize mode.

Comment 2

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

This bug has been marked as a regression. Setting status flag for Nightly to affected.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1857622 - Remove a useless null-check. r=smaug

This doesn't fix the bug but seems worth doing. I can't think of any
reason why this null-check would be needed.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1857622 - Make sure to get the new refresh driver when getting a new presshell. r=smaug

This is the actual bug fix.

If we were an observer of a detached refresh driver, we won't observe
the new one.

Depends on D190574

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/483fb312c27a
Remove a useless null-check. r=smaug
https://hg.mozilla.org/integration/autoland/rev/fcde87df3b93
Make sure to get the new refresh driver when getting a new presshell. r=smaug

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Hey emilio,

I think the Firefox View gang was hoping to have the issue in bug 1856572 (which is caused by this bug) fixed within Firefox 119. This patch stack seems fairly minimal - do you think we'd be safe to request an uplift for it?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9357618 [details]
Bug 1857622 - Make sure to get the new refresh driver when getting a new presshell. r=smaug

Beta/Release Uplift Approval Request

• User impact if declined: bug 1856572
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: comment 0 or bug 1856572.
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): trivial-ish fix. Note that I didn't include a test in the hopes that :tgiles had one ready but I should add test coverage for this otherwise. Still shouldn't block uplift, it's a fairly low-risk fix.
• String changes made/needed: none
• Is Android affected?: No

Comment 8

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/483fb312c27a
https://hg.mozilla.org/mozilla-central/rev/fcde87df3b93

Comment 9

[Donal Meehan [:dmeehan]]

Comment on attachment 9357617 [details]
Bug 1857622 - Remove a useless null-check. r=smaug

Approved for 119.0b9

Comment 10

[Donal Meehan [:dmeehan]]

Comment on attachment 9357618 [details]
Bug 1857622 - Make sure to get the new refresh driver when getting a new presshell. r=smaug

Approved for 119.0b9

Comment 11

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/1d7911c191ac
https://hg.mozilla.org/releases/mozilla-beta/rev/5144465c67bc

Comment 12

[Gabi Cheta [:Gabi] Desktop QA]

Verified as fixed in Firefox 120.0a1 and Firefox 119.0b9 Windows 10x64, macOS 14 and Ubuntu 18.