1659949 - Several links from the “About Firefox” panel are sometimes not working when it is opened for the second/third/nth time

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Emil Ghitta, QA [:emilghitta]]

Attachment: linksAboutt.gif

Affected versions

• 81.0a1 (BuildId:20200818214031)
• 80.0 (BuildId:20200818235255)
• 78.2.0esr (BuildId:20200817153328)

Affected platforms

• Windows 10 64bit
• macOS 10.14
• Ubuntu 18.04 64bit

Steps to reproduce

1. Launch Firefox.
2. Access the “About Firefox Nightly” panel via the Hamburger menu.
3. Close the panel.
4. Repeat step 2.
5. Click on the “Mozilla”, “global community”, “Make a donation” & “get involved!” links.

Expected result

• Clicking on the links mentioned in step 5 opens the corresponding page successfully.

Actual result

• Clicking on those 4 links doesn't open the pages in question.

Regression Window

• This seems to be a regression:
• Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=95ddb3213aecaf87c0a4f2d3559bd17e0194148c&tochange=6ba63b50d93083c798f4eccdbeded15441bd1979
• Potential regressor: Bug 1625333

Additional Information

• For further information regarding this issue please observe the attached screencast.
• I'm not sure which component fits bets for this, please feel free to change it.
• [Suggested Severity] S3.

Comment 1

[Emil Ghitta, QA [:emilghitta]]

Hi Gijs!

It seems that mozregression pointed out Bug 1625333: Ignore changes to the is attribute made by fluent. r=zbraniecki for causing this regression.

I'm not sure who to ping since Dave is out until 28th of September...

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 3

[:Gijs (he/him)]

Looking.

Also, bad bot. :-(

Comment 4

[:Gijs (he/him)]

The about dialog has some links:

            <description class="text-blurb" id="communityExperimentalDesc" data-l10n-id="community-exp">
              <label is="text-link" href="http://www.mozilla.org/" data-l10n-name="community-exp-mozillaLink"></label>
              <label is="text-link" useoriginprincipal="true" href="about:credits" data-l10n-name="community-exp-creditsLink"></label>
            </description>

that are inside fluent strings:

community-exp = <label data-l10n-name="community-exp-mozillaLink">{ -vendor-short-name }</label> is a <label data-l10n-name="community-exp-creditsLink">global community</label> working together to keep the Web open, public and accessible to all.

When things work, those links correctly end up getting the MozTextLink custom element bound to them, which adds a click and keypress handler that looks like:

      this.addEventListener(
        "click",
        event => {
          if (event.button == 0 || event.button == 1) {
            this.open(event);
          }
        },
        true
      );

which makes the link work.

When things break, instead these elements are getting a MozTextLabel custom element bound to them, which has a click handler that looks like this:

    _onClick(event) {
      let controlElement = this.labeledControlElement;
      if (!controlElement || this.disabled) {
        return;
      }
      ...
    }

which bails out immediately because these labels aren't connected to any controls - they're meant to be links.

The markup of these links looks the same both times:

<label xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" is="text-link" href="http://www.mozilla.org/" data-l10n-name="community-exp-mozillaLink" class="text-link">Mozilla</label>

So I think internally something is wrong. I don't know what. Zibi or Olli, can you take a look given your comments in bug 1625333 ? I think this is an issue with the l10n implementation (and maybe the xul proto cache?) and how the is attribute is applied. Note that in both cases, this is a XUL label, not an HTML one.

(I'm upping this to S2 from the suggested S3 in comment #0 because I think (a) broken links are already pretty bad, and (b) this likely won't only affect the about dialog, or only these bindings)

Comment 5

[:Gijs (he/him)]

The effects were a little intermittent for me when I started debugging, but once things broke, they stayed broken.

Comment 7

[Francesco Lodolo [:flod]]

Moving this to Core::Internationalization for now, as we're in the process of graveyarding Core::Localization.

Comment 8

[:Gijs (he/him)]

Dave, now that you're back, are you able to have a look at this?

Comment 9

[Dave Townsend [:mossop]]

Ok I think I see what is going on here. The problem stems from us holding the is attribute value in two places on the element prototype, as an attribute value and as mIsAtom. Importantly we only update the mIsAtom value when calling SetAttrAt which is mostly fine as it covers creating the prototype from a source file and deserializing the prototype from the startup cache.

But things work differently when we rebuild an element's prototype after fluent has applied translations. When we rebuild the prototype's attributes we do not go through the SetAttrAt path, instead just writing directly to the attribute buffer. Before bug 1625333 when encountering an is attribute we would attempt to update the mIsAtom which would break if the attribute value was not an atom. We concluded that Fluent should never be altering the is attribute anyway so the mIsAtom should never need to be updated (side-note: we are still updating the is attribute in the prototype here so it is technically possible for these two to get out of sync if somehow that got changed).

That's all fine for normal elements that have their contents translated, we have their prototype and we are simply updating the attributes, the mIsAtom retains its value. But there are elements inside the translation here and when updating their prototypes we do something different. Instead of applying each element's attributes to their existing prototypes we instead throw away the existing prototypes and create new ones from scratch: https://searchfox.org/mozilla-central/source/dom/xul/nsXULPrototypeDocument.cpp#484-506. Because we use the same method of writing attributes directly in to the buffer instead of SetAttrAt these new prototypes never get mIsAtom updated at all.

So, this bug will happen like this:

1. When loading the UI document from the source (not the startup cache) we will apply translations and then update the prototypes. The first display of the UI will work fine as the original elements were created with the correct is value.
2. Subsequent loads of the same UI in the same Firefox session will be broken as they will be generated from the in-memory prototypes that have the missing mIsAtom.
3. After restarting Firefox the prototypes will be loaded from the startup cache which will follow the normal path and so set a correct mIsAtom so displays of the UI will work from now on.
4. Until a Firefox update or something else causes the startup cache to be purged.

I guess the ideal fix would be to update the inner-element's existing prototypes but that is challenging primarily because we do not currently have a way to get to an element's prototype from the element. We currently do it for the main translated elements by caching anything that has a data-l10n-id attribute during document load and then clearing it after translation is complete. We could extend that to cover these elements somehow though.

An alternative would be to either use SetAttrAt when rebuilding these attributes or otherwise re-introduce the code with a fix from bug 1625333 to set the mIsAtom correctly when encountering an is attribute. This seems like the most straightforward fix.

A potential improvement would be to drop mIsAtom entirely and rely on the attribute value so we are only holding this in one place but I think there would be performance implications with that.

Smaug, do you have a preference or alternate suggestion here?

Comment 10

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

So why do we miss the mIsAtom? Are we not building the prototypes correctly?
Should nsXULPrototypeDocument::RebuildPrototypeFromElement do something like

CustomElementData* ceData = aElement->GetCustomElementData();
nsAtom* isAtom = ceData ? ceData->GetIs(this) : nullptr;
aPrototype->mIsAtom = isAtom

Comment 11

[Dave Townsend [:mossop]]

(In reply to Olli Pettay [:smaug] from comment #10)

So why do we miss the mIsAtom? Are we not building the prototypes correctly?

We removed the (broken) attempt to do that in bug 1625333 with the assumption that the mIsAtom was already correct, I forgot about the case of elements within the translated element.

Should nsXULPrototypeDocument::RebuildPrototypeFromElement do something like

CustomElementData* ceData = aElement->GetCustomElementData();
nsAtom* isAtom = ceData ? ceData->GetIs(this) : nullptr;
aPrototype->mIsAtom = isAtom

Ah yeah that looks right.

Comment 12

[Dave Townsend [:mossop]]

Attachment: Bug 1659949: Fix is atom setting for elements within localized elements. r=smaug

When re-creating element prototypes for elements within localized elements we
must set the mIsAtom correctly on the prototype.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2985d50c1c35
Fix is atom setting for elements within localized elements. r=smaug

Comment 14

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/2985d50c1c35

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Is this something we should nominate for uplift for 84 still or can this ride 85 to release?

Comment 17

[Dave Townsend [:mossop]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)

Is this something we should nominate for uplift for 84 still or can this ride 85 to release?

The user impact here is quite low (only occurs if you open the dialog more than once in the same session after a new Firefox has been installed) and while I think the risk of regression here is pretty low I don't think there is any need to push this out faster than normal.

Comment 18

[Gabi Cheta [:Gabi] Desktop QA]

Verified issue as fixed with Firefox 85.0b3 and Nightly 86.0a1 on Windows 10x64, Ubuntu 18.4 and macOS 10.12.6.