Closed Bug 1483036 Opened 6 years ago Closed 6 years ago

Fluent errors are not propagated

Categories

(Core :: Internationalization, defect, P2)

Product:
Core
Component:
Internationalization
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla68
Tracking Flags:
Tracking Status
firefox68 --- fixed

People

(Reporter: mossop, Assigned: zbraniecki)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

Bug 1483036 - Report meaningful Promise values from FluentDOM C++ bits
47 bytes, text/x-phabricator-request
Details | Review
Bug 1483036 - Fix fluent errors caught by the new test failures. r?gijs
47 bytes, text/x-phabricator-request
Details | Review
Bug 1483036 - Add MaybeResolveWithClone and MaybeRejectWithClone to Promise. r?smaug
47 bytes, text/x-phabricator-request
Details | Review
Bug 1483036 - Simplify the DocumentL10n::PromiseResolver handler. r?smaug
47 bytes, text/x-phabricator-request
Details | Review 
I was experimenting with moving brand.ftl somewhere new and so preferences couldn't find it. It threw an exception in the console: "JavaScript error: , line 0: uncaught exception: undefined". Rather unhelpful. Devtools tells me the exception was coming from https://searchfox.org/mozilla-central/source/intl/l10n/DOMLocalization.jsm#736
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Component: General → Internationalization
Flags: needinfo?(gandalf)
Product: L20n → Core
Yeah, we haven't done too much to help developers understand when the l10n context is not created.

I'll add a couple checks to warn if L10nRegistry returns an empty list, or if Localization doesn't get into any context, but I also would like to make the Node.localize rejections a bit more readable.

In particular, there are two places where I'd like to return a meaningful rejection rather than undefined:

1) https://searchfox.org/mozilla-central/source/dom/base/nsINode.cpp#3013 - this should reject with the aValue of the rejection it received so that I can throw from within of the callback.
2) https://searchfox.org/mozilla-central/source/dom/base/nsINode.cpp#2956 - here I'd like to throw with a string statinh something like "The number of translations received does not match the number of elements to be localized.".

:smaug - when I tried to use the `aCx` in both cases, I get `*** Compartment mismatch 0x7f242d0b07c0 vs. 0x7f241ffbdc40`. Is there any obvious reason for that?
Flags: needinfo?(gandalf) → needinfo?(bugs)
aCx's compartment is different than your object's compartment.
I'm not sure what you're doing, but you may need to JS_WrapObject or enter the right realm, 
https://searchfox.org/mozilla-central/rev/2466b82b729765fb0a3ab62f812c1a96a7362478/js/src/jsapi.h#1015
Flags: needinfo?(bugs)
Priority: -- → P2
Blocks: 1453765, 1394885

This patch enables console reporting of bugs such as:

  • <link rel="localization> with uri to a non-existing resource
  • missing translations
  • resolver errors in translations
  • missing terms/functions/arguments in resolution
  • errors during document translation

This covers the original report here and as reported in bug 1523761.

Summary: If brand.ftl is missing you get an undefined exception opening preferences → Fluent errors are not propagated

New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7b9904b98c3b041c66e7c1147e275b9ff9fab46&selectedJob=234922971

smaug: It seems that except of several properly reported fluent related errors, there's one error that has the following message: "uncaught exception - Error: Encountered unsupported value type writing stack-scoped structured clone at"

If I replace LocalizationHandler::RejectedCallback in dom/base/nsINode back to mReturnValuePromise->MaybeRejectWithUndefined(); the error disappears.

Can you help me fix it? I see it in the following tests:

  • linux/debug/bc7 - toolkit/components/aboutperformance/tests/browser/browser_aboutperformance.js
  • linux/debug|opt/c2 - intl/l10n/test/dom/test_docl10n_ready_rejected.html

My first guess was that those are some broken utf8 coming from JS, but I couldn't find anything that could produce them in those tests or the code...

Flags: needinfo?(bugs)

I'm lost now. Should I review here something or do something else?

But anyhow, sounds like someone is passing some object which can't be structure-cloned.
What kind of objects are being passed to reject?

Flags: needinfo?(bugs)

Ok, the patch is now ready for review.

All the errors that I see in the try run [0] are fluent reported errors that I'll fix in the next patch in this bug.

[0] https://treeherder.mozilla.org/#/jobs?repo=try&revision=964954a07128f3c406ec1adf5a90639260e3a62d

Those are almost all of the errors I found with the new test.

I left one, which I'm not sure how to fix.
Paolo - In bug 1506382 you added string about-config-pref-accessible-value-custom [0] which in its test browser/components/aboutconfig/test/browser/browser_clipboard.js fails with:

A promise chain failed to handle a rejection: [fluent][resolver] errors in en-US/about-config-pref-accessible-value-custom: RangeError: Too many characters in placeable (6955, max allowed is 2500). - stack: translateElements@resource://gre/modules/DOMLocalization.jsm:778:3

Is there a reason why we push 7k chars string to localization?

[0] https://searchfox.org/mozilla-central/rev/7c20ad925005fbad7b8e08813115f1ec7fa1c248/browser/locales/en-US/browser/aboutConfig.ftl#37

Flags: needinfo?(paolo.mozmail)

Florian - In bug 1241024 you added about:performance l10n [0] which in some cases attempts to pass NaN to localization as an argument. Fluent does not support NaN as a type of an argument and JSON.parse unfortunately turns it into null.

Is there a good reason for that, or is it a bug? If it's a good reason, are you ok with my fix in this patch?

[0] https://searchfox.org/mozilla-central/rev/7c20ad925005fbad7b8e08813115f1ec7fa1c248/toolkit/components/aboutperformance/content/aboutPerformance.js#474

Flags: needinfo?(florian)

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

Those are almost all of the errors I found with the new test.

I left one, which I'm not sure how to fix.
Paolo - In bug 1506382 you added string about-config-pref-accessible-value-custom [0] which in its test browser/components/aboutconfig/test/browser/browser_clipboard.js fails with:

A promise chain failed to handle a rejection: [fluent][resolver] errors in en-US/about-config-pref-accessible-value-custom: RangeError: Too many characters in placeable (6955, max allowed is 2500). - stack: translateElements@resource://gre/modules/DOMLocalization.jsm:778:3

Is there a reason why we push 7k chars string to localization?

[0] https://searchfox.org/mozilla-central/rev/7c20ad925005fbad7b8e08813115f1ec7fa1c248/browser/locales/en-US/browser/aboutConfig.ftl#37

It's a preference value, which could be any length, and we apparently suffix "(custom)". Does that help clarify?

Flags: needinfo?(paolo.mozmail) → needinfo?(gandalf)

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

Florian - In bug 1241024 you added about:performance l10n [0] which in some cases attempts to pass NaN to localization as an argument. Fluent does not support NaN as a type of an argument and JSON.parse unfortunately turns it into null.

Is there a good reason for that, or is it a bug?

Seems like a bug. Do you know when it happens (ie. steps to reproduce)?

Flags: needinfo?(florian)

It's a preference value, which could be any length, and we apparently suffix "(custom)". Does that help clarify?

Not really. I'm pretty confident a 7k chars string is not useful and I hope it's ok to either replace it, not display if it's that long, or truncate. I just don't know which one we should do. Can you make a decision?

Flags: needinfo?(gijskruitbosch+bugs)

Florian - you can also just do if (Number.isNaN(dispatchesSincePrevious)){console.log("ERROR")}

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

It's a preference value, which could be any length, and we apparently suffix "(custom)". Does that help clarify?

Not really. I'm pretty confident a 7k chars string is not useful and I hope it's ok to either replace it, not display if it's that long, or truncate. I just don't know which one we should do. Can you make a decision?

We've always displayed entire preference values in the previous version of about:config.

I don't think we should stop doing this because fluent can't cope with strings longer than 2500 (which, btw, is a weird limit?) characters. This might also bite us in other cases, like when we insert a full URL in an href attribute, which could be much longer. So I think we should file an issue with fluent to increase this limit.

For the immediate future, I expect I can't reproduce - which pref are you seeing this for? If it contains json or similar, messing with the string will just make it unusable. There is also no convenient way of copying the full name/value anymore (no context menu), so we can't even elide it and rely on that.

So this is a decision for whoever owns the new about:config (which, mostly unrelated, I find a pain to use compared to the old one, so it biases whatever I come up with). That's not me, but I don't know who it is now that Paolo has left. Dave?

Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(gandalf)
Flags: needinfo?(dtownsend)

Stas - can you provide the rationale for [0]?

In this particular case, I would question the rationality of "localizing" a string that has 7k characters that are not localizable and adds " (custom)", but maybe it does make sense?
Flod may be the right person the make the call.

Maybe we can just skip localizing it since it seems like it's a debug code that is not really part of any UI (or at least I hope it's not?)

[0] https://searchfox.org/mozilla-central/source/intl/l10n/Fluent.jsm#190

Flags: needinfo?(stas)
Flags: needinfo?(francesco.lodolo)

It doesn't look like it's debug code? The value + "(custom|default)" is set as aria-label, and the span containing the actual value is aria-hidden. Without the added custom|default, a screen reader wouldn't know if the value is customized or not, since the difference is only visual.

Having said that:
a) I don't think it's a call for the localization team to make.
b) Would it be possible to expose the custom vs default information in a different way (e.g. an accessible hidden field), and leave the actual value accessible to screen readers, without passing it through Fluent.

Flags: needinfo?(francesco.lodolo)

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

Stas - can you provide the rationale for [0]?

The limit is intended to protect against so-called quadratic blowup attacks. I chose 2500 arbitrarily, assuming that we wouldn't need to show more than a one-page-worth of text in a single placeable. https://bugzilla.mozilla.org/show_bug.cgi?id=803931#c1 has the details.

Flags: needinfo?(stas)

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e283f5a51c8b7f811225e4dad5e6d0e034725484

Ok, so we seem to have two open issues left:

  1. about_performance is passing NaN to localization - florian is going to look into this.
  2. aboutconfig is attempting to pass 7k chars long string as an argument for localization and Fluent rejects it.

I dirtyhacked those two in this try to see if there's anything else left, but I'm not sure how to really solve (2). We can try to lift the limit and expose ourselves to the attack listed by Stas, or we can remodel how we expose that string.

My naive interpretation is that localization string of above 2.5k characters makes no sense. In comment 19 Gijs gives two examples of why an argument may be longer:

  • serialized JSON string
  • long URL

I don't understand why we would want to put either in a localization string for the UI. JSON output is not a reasonable user-readable string, and I doubt that passing an over page-long serialized JSON with added " (custom)" at the very end is going to be a friendly UX in any sense, or that the " (custom)" at all provides any value.
An URL of above 2500 characters is also questionable because it will for sure break the UI, so I'd prefer us to protect against that by placing it in a textbox or truncating anyway.

Keeping NI on Dave in case he can help us find the person to make a decision.

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

My naive interpretation is that localization string of above 2.5k characters makes no sense. In comment 19 Gijs gives two examples of why an argument may be longer:

  • serialized JSON string
  • long URL

I wasn't very clear here - I'm not so much trying to give 2 examples as make 2 somewhat orthogonal points:

  1. prefs (and therefore about:config pref values) contain long JSON strings. That's not great, but changing it is definitely out of scope for this issue. We just need to make fluent-ized about:config deal with this reality, one way or another.
  2. the limit of 2500 characters is going to be problematic because of URLs that might not be app-provided. A URL that gets put in an href attribute wouldn't break any UI I'm aware of, but it would be problematic if, say, an error page like about:blocked which we show for a page with a longer URL, which would then have broken links for "try again" or whatever because the href attribute got truncated or not set because the URL is too long.

I agree the about:config UI doesn't need to use localized strings to include the pref values, though once someone's made a decision in the design not to have a separate default/custom column anymore, and include that information in the "value" column, I think things get tricky:

My understanding is that the "right" way to mix localizer- and app/user-provided text in fluent in one fragment is to have a localized string that includes the variable text, so that localizers can decide themselves whether (in their language) it makes more sense to put the localized text before/after/around the variable. In DTDs we used to do before/after strings for this, which was widely considered to be a crutch (that often led to bugs) for which fluent obviates the need. Philosophically, it is kind of odd to be claiming that this stops being the "right" solution once the length of the app/user-provided string passes some arbitrary limit. :-)

Flags: needinfo?(gandalf)
Depends on: 1538188
Flags: needinfo?(florian)

Philosophically, it is kind of odd to be claiming that this stops being the "right" solution once the length of the app/user-provided string passes some arbitrary limit. :-)

Gotcha. That's a good point.

Pike, Stas, Flod - any thoughts on that? This is the last remaining open issue in this bug, so I'd like to at least understand what we need to resolve it. Is the 7k long string something we want to support? If not, what should we do here?

Flags: needinfo?(stas)
Flags: needinfo?(l10n)
Flags: needinfo?(gandalf)
Flags: needinfo?(francesco.lodolo)

Not totally sure what I'm being asked here, but here are some comments:

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

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

  1. prefs (and therefore about:config pref values) contain long JSON strings. That's not great, but changing it is definitely out of scope for this issue. We just need to make fluent-ized about:config deal with this reality, one way or another.

If I'm reading right the issue here is that we're using fluent to essentially append a "default" or "custom" string onto a pref value so we can add it as the aria-label for, presumably mostly screenreaders.

I put it to you that whether we use Fluent for this or not, having a screenreader attempt to read out a 7k long string, which at that point is likely json, is helpful to nobody. I think it would make sense to have some simple heuristic for whether the value should be aria accessible and if not add some kind of alternative. Maybe limit top a certain length and always exclude json-looking values? Not sure, but I'd like to hear thoughts from folks who know more about accessibility than me..

  1. the limit of 2500 characters is going to be problematic because of URLs that might not be app-provided. A URL that gets put in an href attribute wouldn't break any UI I'm aware of, but it would be problematic if, say, an error page like about:blocked which we show for a page with a longer URL, which would then have broken links for "try again" or whatever because the href attribute got truncated or not set because the URL is too long.

Is this just a theoretical problem or an actual one. Is there anywhere where we localize a url (that isn't from us) where we would hit this problem. I don't see it happening in the blocked site page for example.

Not to say that the 2500 limit isn't somewhat arbitrary and maybe needs more consideration, but I'm not seeing anything that shows it needs to be longer right now.

Flags: needinfo?(dtownsend)

I agree with Dave. Having a screenreader attempt to read out a 7k string, either JSON or not, doesn't make sense.

Which gives us a solution here - we could do:

if (string > 2500) {
  document.l10n.setAttributes(elem, "key-without-aria");
} else {
  document.l10n.setAttributes(elem, "key-with-aria", {text});
}

Does it sound like a reasonable solution? Gijs? Flod?

This is the last unresolved issue before we can land this patchset AFAIK.

Flags: needinfo?(gijskruitbosch+bugs)

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

I agree with Dave. Having a screenreader attempt to read out a 7k string, either JSON or not, doesn't make sense.

I didn't realize when I wrote my previous comment that this string is only used for aria-label.

It's not correct to assume that aria-label will only be used by screen readers to be read out per se, though indeed for use with screen readers that would be unhelpful. I don't know if there's other Assistive Technology that might make more productive use of it. But it's worth pointing out that the current solution was implemented in bug 1506382 based on feedback from actual AT users, so we shouldn't just clobber it without consulting.

Which gives us a solution here - we could do:

if (string > 2500) {
  document.l10n.setAttributes(elem, "key-without-aria");
} else {
  document.l10n.setAttributes(elem, "key-with-aria", {text});
}

Yuck, and hardcode the fluent internal constant that we're arguing should change? Plus, is 2500 chars really better for screenreaders to read out? No, we shouldn't do that.

Also, if you look at the implementation, there's no with/without aria here. Fluent is only used for the aria-label here, and the span with the actual text that is visible to sighted users gets aria-hidden, ie is hidden from AT.

Whatever we do, we should still expose the custom/default state to AT, because that's the salient detail that they can otherwise not get (ie it's impossible for AT to notice that the text is bold and interpret that). We should also ensure that there is still some way for AT to get some indication of what the value is.

TBH, I don't really understand why the aria-label mirrors the span's textcontent, which is then aria-hidden. Jamie can probably help with that.

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

This is the last unresolved issue before we can land this patchset AFAIK.

Why does this need to be resolved prior to landing the patchset? This is already broken, right, just without errors? We can just file a followup and tell relevant tests to ignore uncaught exceptions.

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(jteh)

(In reply to Dave Townsend [:mossop] (he/him) from comment #28)

Is there anywhere where we [currently] localize a url (that isn't from us) where we would hit this problem. I don't see it happening in the blocked site page for example.

Our network error pages currently use the host name (which, AFAIK, is spec-limited to less than the 2500 fluent limit), rather than the full URL. The page info dialog and the identity popup either do the same or just .textContent things in without involving localization (e.g. because things are in tables or otherwise separated from their localized labels). But the latter type of labeling (<span>Foo:</span><span>Value</span>) is discouraged, because it's awkward in some languages other than English.

Note also that there's not that big a guarantee that URLs "from us" are short, and that some of those are alterable by Firefox distributions.

It just seems like a peculiar limit that we're bound to run into when we don't expect it. 640K is enough for everybody etc. (though TIL, Bill Gates apparently never said that.)

But it's worth pointing out that the current solution was implemented in bug 1506382 based on feedback from actual AT users, so we shouldn't just clobber it without consulting.

good point.

Yuck, and hardcode the fluent internal constant that we're arguing should change? Plus, is 2500 chars really better for screenreaders to read out? No, we shouldn't do that.

Fair

Why does this need to be resolved prior to landing the patchset? This is already broken, right, just without errors? We can just file a followup and tell relevant tests to ignore uncaught exceptions.

True. Would you prefer me to do that?

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

Why does this need to be resolved prior to landing the patchset? This is already broken, right, just without errors? We can just file a followup and tell relevant tests to ignore uncaught exceptions.

True. Would you prefer me to do that?

Yes please.

Filed bug 1539000.

Flags: needinfo?(stas)
Flags: needinfo?(l10n)
Flags: needinfo?(francesco.lodolo)
Attachment #9052502 - Attachment description: Bug 1483036 - Fix fluent errors caught by the new test failures. → Bug 1483036 - Fix fluent errors caught by the new test failures. r?gijs

I'll answer my NI in bug 1539000, where this will now be dealt with.

Flags: needinfo?(jteh)
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0bd696ed34ea
Add MaybeResolveWithClone and MaybeRejectWithClone to Promise. r=smaug
https://hg.mozilla.org/integration/autoland/rev/5100b0065b01
Report meaningful Promise values from FluentDOM C++ bits r=Gijs,smaug
https://hg.mozilla.org/integration/autoland/rev/498699c9be80
Fix fluent errors caught by the new test failures. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/ba00aeb3eb20
Simplify the DocumentL10n::PromiseResolver handler. r=smaug
Regressions: 1565930
See Also: → 1581098
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: