Closed Bug 1576343 Opened 4 months ago Closed 3 months ago

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

Categories

(Core :: Internationalization, defect, P1)

70 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox68 --- unaffected
firefox69 --- unaffected
firefox70 + verified
firefox71 --- verified

People

(Reporter: euthanasia_waltz, Assigned: Gijs)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files)

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

Zibi, can you take a look please?

Flags: needinfo?(gandalf)
Regressed by: 1517880
Has Regression Range: --- → yes
Has STR: --- → yes
Keywords: regression
Priority: -- → P1

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

Attached patch debug.diffSplinter Review

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?

Assignee: nobody → gandalf
Flags: needinfo?(gandalf) → needinfo?(bugs)

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?

Flags: needinfo?(bugs)

I meant "nsIMutationObserver", which L10nMutations extends.

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.

Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

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

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.

Assignee: gandalf → nobody
Status: ASSIGNED → NEW

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

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

See Also: → 1573326

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...

[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.

Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/adc5ed6d76d4
fix l10n.setAttributes use from inside DOMContentLoaded listeners when the proto is cached, r=smaug,zbraniecki
Component: Preferences → Localization
Product: Firefox → Core
Blocks: 1573326
See Also: 1573326
Component: Localization → Internationalization
Blocks: 1578370
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
No longer blocks: 1573326
Duplicate of this bug: 1573326

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
Attachment #9088820 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

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 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.

Attachment #9088820 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+

(edited: oops, meant for bug 1578370)

You need to log in before you can comment on or make changes to this bug.