1539714 - Fluent localization can fail to happen at all, if a document flushes layout before parsing finishes

Status: RESOLVED FIXED

(Core :: Internationalization, enhancement, P3)

Description

[Boris Zbarsky [:bzbarsky]]

When debugging the intermittent failure from bug 1525935 landing, I discovered that what happens is the following:

1. We call Document::LocalizationLinkAdded for about:plugins, with "toolkit/about/aboutPlugins.ftl" as the link href.
2. We add the href to mL10nResources and block the load event.
3. We call Document::OnL10nResourceContainerParsed and do the various InitializeLocalization() bits.
4. We later call into nsHtml5TreeOpExecutor::DidBuildModel. mLayoutStarted is true, so we never call TriggerInitialDocumentTranslation() and hence never end up in InitialDocumentTranslationCompleted() and hence never unblock the load event.

Obviously in this case the document never gets localized (either before my patch or after it).

OK, why is mLayoutStarted true? Because about:plugins has a subframe it loads from <chrome://global/content/directionDetector.html>. And when that subframe is ready to fire its load event, it flushes layout (because you need to do that before firing load, because of web fonts). And flushing layout on the subframe needs to flush layout on the parent. And a forced flush like this will start layout if it hasn't started yet. If the subframe finishes loading before the parent document finishes parsing, we end up with a failure here.

Seems to me that we should be doing TriggerInitialDocumentTranslation() even if mLayoutStarted is true. That's what we do for XML (including XHTML), but not what we do for HTML or XUL, for some reason...

Are there other reasons localization could fail? For example, what happens if in DocumentL10n::TriggerInitialDocumentTranslation "elem" ends up null? Will that prevent the promise from settling? What about if "promise" ends up null? Should we just sync-call InitialDocumentTranslationCompleted() in that case?

Comment 1

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

Seems to me that we should be doing TriggerInitialDocumentTranslation() even if mLayoutStarted is true. That's what we do for XML (including XHTML), but not what we do for HTML or XUL, for some reason...

Hmmm, That means that we can't actually block layout on localization, because subframe can bypass that block anyway? I'm not sure if I understand. Can we fix that by blocking the layout anyway until l10n is completed or do we have to allow for layout to start even if l10n is not done?

Are there other reasons localization could fail? For example, what happens if in DocumentL10n::TriggerInitialDocumentTranslation "elem" ends up null? Will that prevent the promise from settling? What about if "promise" ends up null?

There shouldn't be a scenario like that, so if you see one, we should fix it by always returning.

Comment 2

[Boris Zbarsky [:bzbarsky]]

That means that we can't actually block layout on localization

You can't realiably block layout on anything. If the page has script and the script asks for layout info, we will do layout.

What you can do is have us not do our default (unforced) layout-starting until localization is done. That's what we do right now.

do we have to allow for layout to start even if l10n is not done

Right now you have to allow that; see above.

There shouldn't be a scenario like that

Why are we null-checking "promise", then?

Comment 3

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

Right now you have to allow that; see above.

Makes sense.

Why are we null-checking "promise", then?

Where?

Seems to me that we should be doing TriggerInitialDocumentTranslation() even if mLayoutStarted is true.

That sounds reasonable then.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Where?

https://searchfox.org/mozilla-central/rev/3d469329a42644b41e66944d8da22d78d8c0f5ec/intl/l10n/DocumentL10n.cpp#389-391

That sounds reasonable then.

OK, do we have someone planning to work on that?

Comment 5

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

OK, do we have someone planning to work on that?

It'll be me. I'll try to get to it within a week or two.

Comment 6

[Neha Kochar [:neha]]

Zibi, will the patch be safe for beta uplift? That would decide whether or not the patch for bug 1525935 gets uplifted or not as that’s blocked by this.

Comment 7

[Boris Zbarsky [:bzbarsky]]

It's not clear to me that the patch for bug 1525935 itself is safe for beta uplift, especially at this late date.

Comment 8

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

Attachment: Bug 1539714 - Trigger initial localization even if layout started.

Comment 9

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

Filed bug 1558309 for follow-up cleanups

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/050cd8a6467f
Trigger initial localization even if layout started. r=bzbarsky

Comment 11

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/050cd8a6467f