1755216 - [macOS] Opening the library produces a pile of "Attempt to override an existing message" fluent errors from browserSets.ftl and textActions.ftl

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P3)

Description

[:Gijs (out for now; he/him)]

As in summary. I haven't checked when this started, but it's been in beta for a while; I didn't get around to filing it before, so I expect 97 at least is affected; it may be yet older.

Comment 1

[:Gijs (out for now; he/him)]

Looks like this is mac-specific

Comment 2

[Virgil Sangerean]

Managed to find regression range around the commit adressed here: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5d3b6ace0a3b0a38f16f92688998e1d0f55ea3cb&tochange=5a8a664696bb69d3f113ea7abd27f07c3a7686c3

Comment 3

[:Gijs (out for now; he/him)]

(In reply to Virgil Sangerean from comment #2)

Managed to find regression range around the commit adressed here: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5d3b6ace0a3b0a38f16f92688998e1d0f55ea3cb&tochange=5a8a664696bb69d3f113ea7abd27f07c3a7686c3

Although it is the right date, this revision isn't the regressor. This is probably caused by bug 1613705.

Comment 4

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

:zbraniecki, since you are the author of the regressor, bug 1613705, could you take a look?
For more information, please visit auto_nag documentation.

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

this is not a regression. It's an improvement in reporting of improper build-up of l10n contexts.

Comment 6

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

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

Comment 7

[Erik Nordin [:nordzilla]]

If I'm understanding correctly, the cause of the issue is pre-existing, but we are now reporting it more clearly?

Comment 8

[:Gijs (out for now; he/him)]

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

If I'm understanding correctly, the cause of the issue is pre-existing, but we are now reporting it more clearly?

Before it wasn't reported at all. TBH I don't think the current reporting is great - I think it'd be better if we detected double references to the same file. But in either case, I would actually expect that we fail automated tests when this happens, which is how fluent errors used to work (and maybe some of the other errors fluent generate still do? I don't know how I'd check). See also e.g. bug 1685180. Without that happening I am worried we will ship broken stuff - in this case, there is no user impact, but that won't be the case for all fluent errors.

Comment 9

[Mark Banner (:standard8)]

Example output:

16:39:36.933 Attempt to override an existing message: "text-action-undo". toolkit/global/textActions.ftl
16:39:36.933 Attempt to override an existing message: "text-action-undo-shortcut". toolkit/global/textActions.ftl
16:39:36.933 Attempt to override an existing message: "text-action-redo". toolkit/global/textActions.ftl
16:39:36.933 Attempt to override an existing message: "text-action-redo-shortcut". toolkit/global/textActions.ftl
16:39:36.933 Attempt to override an existing message: "text-action-cut". toolkit/global/textActions.ftl

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #5)

this is not a regression. It's an improvement in reporting of improper build-up of l10n contexts.

Do you have any hints on how we can solve this, and/or what is causing the error?

Comment 10

[:Gijs (out for now; he/him)]

(In reply to Mark Banner (:standard8) from comment #9)

Example output:

16:39:36.933 Attempt to override an existing message: "text-action-undo". toolkit/global/textActions.ftl
16:39:36.933 Attempt to override an existing message: "text-action-undo-shortcut". toolkit/global/textActions.ftl
16:39:36.933 Attempt to override an existing message: "text-action-redo". toolkit/global/textActions.ftl
16:39:36.933 Attempt to override an existing message: "text-action-redo-shortcut". toolkit/global/textActions.ftl
16:39:36.933 Attempt to override an existing message: "text-action-cut". toolkit/global/textActions.ftl

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #5)

this is not a regression. It's an improvement in reporting of improper build-up of l10n contexts.

Do you have any hints on how we can solve this, and/or what is causing the error?

More than 1 link element for textActions.ftl is created, is my guess.

Comment 11

[:Gijs (out for now; he/him)]

Attachment: Bug 1755216 - stop double-including FTL files in the library window on macOS, r?Standard8

Comment 12

[:Gijs (out for now; he/him)]

OK, I've decided that trying to fix this in frontend is tedious and is just going to be a game of whack-a-mole, ie more problems for the next person who runs into this -- because of file includes and/or varying configurations in which we use the same markup files and/or scripts, where it's just pretty hard to ensure that every ftl file makes its way into the document exactly once (fewer than that and we get missing strings, more than that and fluent yells at us lots).

I think this should be fixed in l10n code. Specifically, each document already has a set of resource IDs. Unfortunately, it's not actually a set, it's a vector. I don't understand why, because AFAICT it never makes sense to have more than 1 of the same resource ID. The only operations on that vector that are being used are addition, removal and iteration; the order appears completely immaterial (if there were conflicts, the same yelling we're seeing here would yell about those!).

However, to use a set afaict from a quick web search we'd need to implement Eq and Hash traits on ResourceId, which lives in 1 rust crate, and then use it in a different rust crate for ffi, and then potentially update the DOM l10n code (but hopefully not). That's a bunch of work across several rust crates, which I haven't had to deal with before. Some of them appear to live in m-c, some don't.

Does doing this work actually make sense, or am I missing some reason this is a bad idea? Can the l10n team do this work? (Greg, feel free to forward to whoever the right person is; I'm not sure how to find out.)

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

the order appears completely immaterial (if there were conflicts, the same yelling we're seeing here would yell about those!).

The reason Fluent DOM flags conflicts is driven by the scenario where reordering of the list impacts results.

I think your solution is a good one - list of DOM links may be thought of as a Set. This is trickier for imperative constructed contexts where you just call addResource but I think that for those reporting warnings is ok (and there's override flag to allow for them explicitly!).

Comment 14

[Greg Tatum [:gregtatum] (REO Fx132)]

Specifically, each document already has a set of resource IDs. Unfortunately, it's not actually a set, it's a vector. I don't understand why, because AFAICT it never makes sense to have more than 1 of the same resource ID. The only operations on that vector that are being used are addition, removal and iteration; the order appears completely immaterial (if there were conflicts, the same yelling we're seeing here would yell about those!).

When I'm looking at fluent-bundle it's already using a FxHashMap.

https://searchfox.org/mozilla-central/source/third_party/rust/fluent-bundle/src/bundle.rs#137

The addResource method returns a result with the Occupied error: https://searchfox.org/mozilla-central/rev/49011d374b626d5f0e7dc751a8a57365878e65f1/third_party/rust/fluent-bundle/src/bundle.rs#214

That error appears to be converted to the string: https://searchfox.org/mozilla-central/source/third_party/rust/fluent-bundle/src/errors.rs#66

Here in the FFI layer, we pass the Errors through: https://searchfox.org/mozilla-central/rev/49011d374b626d5f0e7dc751a8a57365878e65f1/intl/l10n/rust/fluent-ffi/src/bundle.rs#307

So it seems like we could see any attempts to override, and just not surface them, as here it seems like we're saying it is alright for this to be permissive. I haven't manually validated that the Library errors are coming from here, but that seems like an easy enough fix to test.

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

Notice FluentBundle::add_resource_overriding - that allows for overrides where latest-wins.

The reason AST is a vec is to allow for storing duplicated entries.

Whether we should allow for duplicated entries is more questionable to me.
You're considering oldest-wins model where second entry gets ignored. That may be racy (what is oldest may depend on registration order which may be intermittent).
I have to say that so far I liked the hygiene of encouraging developers to avoid registering duplicated entries.

The solution that may be better here is to deduplicate Localization resourceIds to be a Set.

In that scenario, registering the same URI second time doesn't trigger extension of resourceIds array, because the resource is already in a set.

But registering two different resources that both provide the same entry I'd will trigger a warning.

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

The code to handle it would be here https://github.com/projectfluent/fluent-rs/blob/master/fluent-fallback/src/localization.rs#L58

Comment 17

[Greg Tatum [:gregtatum] (REO Fx132)]

Yeah, looking at it this morning, it appears these errors are being added here:

https://searchfox.org/mozilla-central/rev/eddb810ffd5499f0984123fe4bfea6064ebad3c8/intl/l10n/rust/l10nregistry-rs/src/registry/synchronous.rs#37

I caught a profiler stack of it in action here: https://share.firefox.dev/3FIzmqU

The other place I linked was wired into the JS API.

Comment 18

[Zibi Braniecki [:zbraniecki][:gandalf]]

allow_overriding is about overriding entries. What i'm suggesting is to skip adding resourceIds to Localization if one is already added.

So, right now if you do:

let l10n = new Localization();
l10n.addResource("toolkit/textActions.ftl");
l10n.addResource("toolkit/textActions.ftl"); // warnings because all messages from textActions.ftl are already present

but it does make little sense. there's no way that the second exactly same resourceId brings different values. So we could just skip it in addResource (ergo - treat resourcesIds of Localization as a Set, not Array).

Comment 19

[Zibi Braniecki [:zbraniecki][:gandalf]]

In other words, what you're debugging Greg now leads you to paths that are responsible for any entry overrides. If you mute those warnings there, you'll mute it in case when the same resourceId is added twice but also you'll mute if two different resources add the same entry.

I think the suggestion from Gijs in Comment 12 is allowing us to prevent addition of the same resourceId twice (which in result make it not attempt to override all of its previous entries) but sill will warn if two different resources attempt to insert the same entry.

Comment 20

[:Gijs (out for now; he/him)]

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #19)

I think the suggestion from Gijs in Comment 12 is allowing us to prevent addition of the same resourceId twice (which in result make it not attempt to override all of its previous entries) but sill will warn if two different resources attempt to insert the same entry.

Yep - I think warning for having 2 different files with the same fluent msg id makes sense. It's just the double identical resourceId which feels like it should just be a no-op with no warning.

Comment 21

[Greg Tatum [:gregtatum] (REO Fx132)]

https://github.com/projectfluent/fluent-rs/pull/277

Comment 22

[Greg Tatum [:gregtatum] (REO Fx132)]

Attachment: Bug 1755216 - Certify fluent vendoring upgrades; r?nordzilla

Comment 23

[Greg Tatum [:gregtatum] (REO Fx132)]

Attachment: Bug 1755216 - Vendor in fluent-fallback@0.7.0; r?nordzilla

Depends on D161733

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bc1e84b62eb3
Certify fluent vendoring upgrades; r=supply-chain-reviewers,bholley
https://hg.mozilla.org/integration/autoland/rev/45a20db05ce8
Vendor in fluent-fallback@0.7.0; r=nordzilla

Comment 25

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/bc1e84b62eb3
https://hg.mozilla.org/mozilla-central/rev/45a20db05ce8