1576343 - Options/General/Network Settings/Enable DNS over HTTPS/Use Provider->listbox labels lost

Status: VERIFIED FIXED

(Core :: Internationalization, defect, P1)

Description

[atlanto]

STR:

1. Go to about:preferences and open Network Settings dialog.
2. Check 'Enable DNS over HTTPS'.
3. Click 'OK' to close the dialog.
4. Reopen the dialog.

ER:
'Use Provider' listbox has labels.

AR:
There is no label in 'Use Provider' listbox.

mozregression:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=9234c94b4b9ac47d82496fb2b6442e5754ec4fd9&tochange=743b76c2c4fe581711b4164eade1d54a2a5f96fb

Comment 1

[:Gijs (he/him)]

Zibi, can you take a look please?

Comment 2

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

I started debugging this today, but it doesn't seem to be anything trivial to fix.

Comment 3

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

Attachment: debug.diff

Here's a simple patch that prints when L10nMutations is being constructed and when AttributeChanged is being called (which is part of the nsMutationObserver).

Here's the STR:

1. Apply the patch
2. Launch Firefox
3. Open Preferences
4. Scroll down
5. Open Network Settings > Settings
6. Close The modal dialog
7. Reopen it

Current result:

When reopened the menulist doesn't get localized

Expected result:

When reopened the menulist should get localized

=============

Analysis:

The menulist is rebuilt from scratch on each dialog open in initDnsOverHttpsUI method and I verified that it's being called properly. I even replaced the call to DOMLocalization::setAttributes with manual l10n-id/args setting to ensure that it is not a bug in that function.

Now, the first time one opens the dialog with my patch applied, the console shows:

L10nMutations Constructor!
console.log: "Settings connection-dns-over-https-url-item-default data-l10n-id attribute."
AttributeChanged: connection-dns-over-https-url-item-default.
AttributeChanged: connection-dns-over-https-url-custom.

which is what we'd expect to see from the nsMutationObserver. We created the L10nMutations, and then two attributes got applied.

Now, when one closes the dialog and reopens it, the log is:

L10nMutations Constructor!
console.log: "Settings connection-dns-over-https-url-item-default data-l10n-id attribute."

Ergo, despite the nsMutationObserver being initialized, and the item.setAttribute("data-l10n-id", "..."); being called, the AttributeChanged is not being called this time.

I don't know how to further debug this, and I'll be on PTO till next Tuesday.

Olli, any chance you can take a look as to why the AttributeChanged is not being triggered here?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

I don't know what nsMutationObserver we're talking about. Searchfox doesn't reveal any such.

But is the observer there observing item, when setAttribute on it is called? I mean, is the item in the DOM subtree which the observer is observing?

Comment 5

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

I meant "nsIMutationObserver", which L10nMutations extends.

Comment 6

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

Yes, it is. It uses the XBL method: https://searchfox.org/mozilla-central/source/toolkit/content/widgets/menulist.js#369 to append an item, on the menu element which is in the observed tree.

I can also open the console scroll to that element, select it, select "Use in console" and then type temp0.ownerDocument.l10n.setAttributes("connection-dns-over-https-url-item-default", {name: "Foo"}); and it triggers the AttributeChanged.

I'm worried it is some entanglement between XBL and the nsIMutationObserver.

I know the original report suggests the fast-load cache, but I'm not sure how likely it is to be correlated since that happens when the document is created and alive, and not loaded from cache.

Comment 7

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

There seem to be a potential other case where this happens (similar symptoms - happens on reload/reopen?) - bug 1576343..

Comment 8

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

I didn't manage to find a cause of it, and I'm still circling around the fact that the nsIMutationObserver doesn't fire AttributeChanged after reopen. I suspect that it's the same bug as 1576343.

I'm unassigning myself because I'm on PTO till Tuesday, and if it is a blocker, we may need a fix before I'm back. Assuming this is indeed caused by the mini-cache, we may want to bring back the data-l10n-cache attribute on <window> element and set it only for browser.xhtml to keep the talos performance and remove the regression in Preferences, until we can find a real cause and fix it.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

When data-l10n-id is set next time, does it have the same value as before? And does l10n need to handle such case?
If so, AttributeSetToCurrentValue https://searchfox.org/mozilla-central/rev/325c1a707819602feff736f129cb36055ba6d94f/dom/base/nsIMutationObserver.h#210

Comment 10

[:Gijs (he/him)]

I assume the repeated self-references to this bug in comment #7 / comment #8 refer to bug 1573326.

Comment 11

[:Gijs (he/him)]

OK, I just spent far too much time on debugging this because this code is... super hairy. Anyway, it turns out the answer is much more mundane than something about XBL (not sure where that came from; there's only custom elements...) or mutation observers being broken.

In fact, the problem is almost apparent from a critical reading of the comments in DocumentL10n.cpp, if you ignore the code bits:

  // 1. Collect all localizable elements.
  // 2. Check if the document has a prototype that may cache translated elements.
  if (proto) {
    // 2.1. Handle the case when we have proto.
    // 2.1.1. Move elements that are not in the proto to a separate array.
    // We populate the sequence in reverse order. Let's bring it
    // back to top->bottom one.
    // 2.1.2. If we're not loading from cache, push the elements that
    //        are in the prototype to be translated and cached.
    if (!proto->WasL10nCached() && !elements.IsEmpty()) {
       ...
    }
    // 2.1.3. If there are elements that are not in the prototype,
    //        localize them without attempting to cache and
    //        independently of if we're loading from cache.
    if (!nonProtoElements.IsEmpty()) {
    }

    // 2.1.4. Check if anything has to be translated.
    if (promises.IsEmpty()) {
      // 2.1.4.1. If not, resolve the mReady and complete
      //          initial translation.
    } else {
      // 2.1.4.2. If we have any TranslateElements promises,
      //          collect them with Promise::All and schedule
      //          the L10nReadyHandler.
    }
  } else {
    // 2.2. Handle the case when we don't have proto.

    // 2.2.1. Otherwise, translate all available elements,
    //        without attempting to cache them and schedule
    //        the L10nReadyHandler.
  }

  // 3. Connect the root to L10nMutations observer.
  ConnectRoot(*elem, rv);

The problem is step 2.1.4.1 vs. step 3. Specifically, marking the initial translation as complete in step 2.1.4.1 tells the proto content sink that this has happened, which calls MaybeDoneWalking which calls DoneWalking, which calls mDocument->EndLoad(), which calls UnblockDOMContentLoaded which fires a DOMContentLoaded event.

That wouldn't be a problem, except that this is all synchronous, and so is the JS that runs as a result of the DCL event.

But we haven't connected up the mutation observer yet, because that only happens when we call ConnectRoot() in step 3.

I dunno if this is easily fixable or no, because I don't know what (inadvertently or otherwise) depends on the current state. But the current state means all document.l10n.setAttributes() calls that are made from inside DCL event handlers (or elsewhere in the sync calltree from that event) are ignored when we have a proto cache copy of the document. I expect that's not only a problem for connection.js and the fxa sidebar, but probably elsewhere as well...

Comment 12

[:Gijs (he/him)]

[Tracking Requested - why for this release]:

Oh, also, note that this problem becomes permanent as soon as something has loaded (ie you don't need the "close and open again" part of the steps after the first time), because we persist the proto cache - it only gets wiped for updates. Those happen regularly on nightly of course, but less so on beta/release... So we should probably track for 70; I dunno if we can get a fix in this week but otherwise it'll need to happen soon in beta.

Comment 13

[:Gijs (he/him)]

Attachment: Bug 1576343 - fix l10n.setAttributes use from inside DOMContentLoaded listeners when the proto is cached, r?smaug!,Mossop!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/adc5ed6d76d4
fix l10n.setAttributes use from inside DOMContentLoaded listeners when the proto is cached, r=smaug,zbraniecki

Comment 15

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/adc5ed6d76d4

Comment 17

[:Gijs (he/him)]

Comment on attachment 9088820 [details]
Bug 1576343 - fix l10n.setAttributes use from inside DOMContentLoaded listeners when the proto is cached, r?smaug!,Mossop!

Beta/Release Uplift Approval Request

• User impact if declined: Missing localized strings
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment #0
• List of other uplifts needed: n/a
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Specific minimal change to the affected code, pretty significant amounts of beta runway left.
• String changes made/needed: None

Comment 18

[Rares Doghi, Desktop QA]

This issue is verified as Fixed in our latest Nightly build 71.0a1 (2019-09-05) on Mac 10.14, Ubuntu 16.04 and Windows 10.

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9088820 [details]
Bug 1576343 - fix l10n.setAttributes use from inside DOMContentLoaded listeners when the proto is cached, r?smaug!,Mossop!

Fix for new regression, verified in nightly. Let's see how it does in beta 5.

Comment 20

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/234d078a056b

Comment 21

[Rares Doghi, Desktop QA]

This issue is verified as Fixed in Beta 70.b5 on Windows 10, Mac 10.14 and Ubuntu 18.04.

Comment 22

[:Gijs (he/him)]

(edited: oops, meant for bug 1578370)