1291695 - Missing { references } can break bindings

Status: RESOLVED FIXED

(L20n :: JS Library, defect)

Description

[Axel Hecht [:Pike]]

In bug 1288406 comment 34, we found a crash in MessageContext.jsm:


ReferenceError: Unknown entity: brand-shorter-name
Stack trace:
EntityReference@resource://gre/modules/IntlMessageContext.jsm:946:16
Value@resource://gre/modules/IntlMessageContext.jsm:1025:26
Pattern@resource://gre/modules/IntlMessageContext.jsm:1127:16
Value@resource://gre/modules/IntlMessageContext.jsm:1010:19
Value@resource://gre/modules/IntlMessageContext.jsm:1032:21
Pattern@resource://gre/modules/IntlMessageContext.jsm:1127:16

We should never crash, 'cause that takes the browser down.

Comment 1

[Staś Małolepszy :stas]

The output you're seeing is a console.error that we should probably remove at some point.

That said, there's definitely an issue here related to how we're handling broken { references } to other entities.  Half of the menubar just disappears when I misspell a single reference in bug 1288406.

I'll investigate.

Comment 2

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

So, I tried to debug it, and I'm not sure if I found the same bug or different, but I noticed that if I add a broken reference to "file-menu" xul/label, it breaks completely.

There are two parts. One is a graceful recovery - for xul it may be that displaying ID in label is better than as value - and the other is why we didn't show the broken attribute.

Oh boy, what a ride.

So, the problem is this line: https://github.com/l20n/l20n.js/blob/69fe01ff11422ffd9a0e68d91eaffa6fa8e4c321/src/lib/format.js#L53

Basically, we don't differentiate between types of errors and in this scenario, if the first bundle errors with "File menu brand-shorter-name2", and the latter with fallback to ID, we take the latter because previous had errors.

Not sure how we want to fix that and if it's this bug, but I couldn't reproduce losing multiple translations due to an error in one entity.

Comment 3

[Staś Małolepszy :stas]

Thanks for investigating.  I'll try the same scenario tomorrow to refresh my memory, and will try to provide a helpful opinion.

Comment 4

[Staś Małolepszy :stas]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #2)

> There are two parts. One is a graceful recovery - for xul it may be that
> displaying ID in label is better than as value - and the other is why we
> didn't show the broken attribute.

Let's fix the first issue in bug 1288443 and discuss the second one here.

> So, the problem is this line:
> https://github.com/l20n/l20n.js/blob/
> 69fe01ff11422ffd9a0e68d91eaffa6fa8e4c321/src/lib/format.js#L53
> 
> Basically, we don't differentiate between types of errors and in this
> scenario, if the first bundle errors with "File menu brand-shorter-name2",
> and the latter with fallback to ID, we take the latter because previous had
> errors.

Would it make sense to differentiate between L10nErrors (meaning: "this entity is missing") and other Errors returned by MessageContext (they're all standard JS errors: ReferenceError, RangeError, TypeError)?

I'll try to prepare a patch today to demonstrate what I mean.

> Not sure how we want to fix that and if it's this bug, but I couldn't
> reproduce losing multiple translations due to an error in one entity.

Me neither.

Comment 5

[Staś Małolepszy :stas]

Attachment: Pull request

Here's what I've been thinking about, expressed in code.

Comment 6

[Staś Małolepszy :stas]

Landed with tests in https://github.com/l20n/l20n.js/commit/1eba14f64d4d71b757a8af0b327a6f790a60fe8f.

Comment 7

[Staś Małolepszy :stas]

https://hg.mozilla.org/projects/larch/rev/9d04883619b221b65d67ce66b168c1ec1dc4566c
Bug 1291695 - Improve fallback strategy based on the type of error. r=gandalf

Comment 8

[Staś Małolepszy :stas]

(In reply to Staś Małolepszy :stas from comment #5)

> Here's what I've been thinking about, expressed in code.

For archiving purposes:

The patch differentiates between fatal errors caused by missing translations and non-fatal formatting errors returned by MessageContext.

The fallback strategy is now to trigger the loading of the next language only if there were fatal errors in the current language. Translations with only non-fatal errors are considered to be good fallback values.  If a fatal error in another translation triggers fallback, the Localization will prefer translations from the fallback bundle if translations from the first bundle had non-fatal errors.

So to sum up:

 - non-fatal (formatting) errors don't trigger fallback on their own
 - instead they return strings with as much of the translation as possible
 - a fallback might be triggered by another missing translation in the same call to formatValues/formatEntities
 - in this case, we try to find a better value also for the non-missing but broken translations from the first bundle.