1807249 - videocontrols code can try to set undefined position which causes fluent exceptions on infra (Resolver error: Unknown variable: $position)

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect, P3)

Description

[:Gijs (he/him)]

Spotted in my completely unrelated trypush - log link, job link

[task 2022-12-23T14:52:39.928Z] 14:52:39     INFO - GECKO(26233) | JavaScript warning: chrome://global/content/elements/videocontrols.js, line 1283: Script terminated by timeout at:
[task 2022-12-23T14:52:39.929Z] 14:52:39     INFO - GECKO(26233) | showPosition@chrome://global/content/elements/videocontrols.js:1283:19
[task 2022-12-23T14:52:39.931Z] 14:52:39     INFO - GECKO(26233) | setupInitialState@chrome://global/content/elements/videocontrols.js:407:14
[task 2022-12-23T14:52:39.931Z] 14:52:39     INFO - GECKO(26233) | init@chrome://global/content/elements/videocontrols.js:2609:14
[task 2022-12-23T14:52:39.931Z] 14:52:39     INFO - GECKO(26233) | onsetup@chrome://global/content/elements/videocontrols.js:2830:16
[task 2022-12-23T14:52:39.932Z] 14:52:39     INFO - GECKO(26233) | switchImpl@chrome://global/content/elements/videocontrols.js:87:17
[task 2022-12-23T14:52:39.932Z] 14:52:39     INFO - GECKO(26233) | onchange@chrome://global/content/elements/videocontrols.js:36:10
[task 2022-12-23T14:52:39.933Z] 14:52:39     INFO - GECKO(26233) | [Child 30959, Main Thread] WARNING: NS_ENSURE_TRUE(JS_Stringify(aCx, &value, nullptr, JS::NullHandleValue, JSONCreator, &serializedValue)) failed: file /builds/worker/checkouts/gecko/dom/base/nsContentUtils.cpp:10627
[task 2022-12-23T14:52:39.971Z] 14:52:39     INFO - GECKO(26233) | Fluent error, the argument "$position" was not provided a value.
[task 2022-12-23T14:52:39.973Z] 14:52:39     INFO - GECKO(26233) | This error happened while formatting the following messages:
[task 2022-12-23T14:52:39.974Z] 14:52:39     INFO - GECKO(26233) |   "videocontrols-error-aborted"
[task 2022-12-23T14:52:39.975Z] 14:52:39     INFO - GECKO(26233) |   "videocontrols-error-network"
[task 2022-12-23T14:52:39.980Z] 14:52:39     INFO - GECKO(26233) |   "videocontrols-error-decode"
[task 2022-12-23T14:52:39.981Z] 14:52:39     INFO - GECKO(26233) |   "videocontrols-error-src-not-supported"
[task 2022-12-23T14:52:39.982Z] 14:52:39     INFO - GECKO(26233) |   "videocontrols-error-no-source"
[task 2022-12-23T14:52:39.983Z] 14:52:39     INFO - GECKO(26233) |   "videocontrols-error-generic"
[task 2022-12-23T14:52:39.984Z] 14:52:39     INFO - GECKO(26233) |   "videocontrols-status-picture-in-picture"
[task 2022-12-23T14:52:39.985Z] 14:52:39     INFO - GECKO(26233) |   "videocontrols-picture-in-picture-toggle-label2"
[task 2022-12-23T14:52:39.986Z] 14:52:39     INFO - GECKO(26233) |   "videocontrols-picture-in-picture-explainer3"
[task 2022-12-23T14:52:39.987Z] 14:52:39     INFO - GECKO(26233) |   "videocontrols-buffer-bar-label"
[task 2022-12-23T14:52:39.988Z] 14:52:39     INFO - GECKO(26233) |   "videocontrols-scrubber"
[task 2022-12-23T14:52:39.989Z] 14:52:39     INFO - GECKO(26233) |   "videocontrols-volume-control"
[task 2022-12-23T14:52:39.990Z] 14:52:39     INFO - GECKO(26233) |   "videocontrols-casting-button-label"
[task 2022-12-23T14:52:39.991Z] 14:52:39     INFO - GECKO(26233) |   "videocontrols-closed-caption-button"
[task 2022-12-23T14:52:39.991Z] 14:52:39     INFO - GECKO(26233) |   "videocontrols-closed-caption-off"
[task 2022-12-23T14:52:39.992Z] 14:52:39     INFO - GECKO(26233) |   "videocontrols-enterfullscreen-button"
[task 2022-12-23T14:52:39.993Z] 14:52:39     INFO - GECKO(26233) |   "videocontrols-play-button"
[task 2022-12-23T14:52:39.994Z] 14:52:39     INFO - GECKO(26233) |   "videocontrols-play-button"
[task 2022-12-23T14:52:39.995Z] 14:52:39     INFO - GECKO(26233) |   "videocontrols-position-and-duration-labels"
[task 2022-12-23T14:52:39.996Z] 14:52:39     INFO - GECKO(26233) | Hit MOZ_CRASH(Resolver error: Unknown variable: $position) at intl/l10n/rust/localization-ffi/src/lib.rs:620

with a rust stack trace (which is not much use - we want to know where the information came from that broke).

The JS stack points to this code

which does:

        let positionTime = this.formatTime(currentTime, this.showHours);

        this.scrubber.value = currentTime;
        this.positionDurationBox.position = positionTime;

        this.l10n.setAttributes(
          this.positionDurationBox,
          "videocontrols-position-and-duration-labels",
          {
            position: positionTime,
            duration: this.positionDurationBox.duration,
          }
        );

But AFAICT this.formatTime() always returns a non-undefined value:

formatTime(aTime, showHours = false) {
  // Format the duration as "h:mm:ss" or "m:ss"
  aTime = Math.round(aTime / 1000);
  let hours = Math.floor(aTime / 3600);
  let mins = Math.floor((aTime % 3600) / 60);
  let secs = Math.floor(aTime % 60);
  let timeString;
  if (secs < 10) {
    secs = "0" + secs;
  }
  if (hours || showHours) {
    if (mins < 10) {
      mins = "0" + mins;
    }
    timeString = hours + ":" + mins + ":" + secs;
  } else {
    timeString = mins + ":" + secs;
  }
  return timeString;
},

(because by the end of the function timeString must be a string)

However, it seems possible that this code:

let timeString = isInfinite ? "" : this.formatTime(duration);
this.positionDurationBox.duration = timeString;
this.l10n.setAttributes(
  this.positionDurationBox,
  "videocontrols-position-and-duration-labels",
  {
    position: this.positionDurationBox.position,
    duration: timeString,
  }
);

which reuses this.positionDurationBox.position could grab undefined at that point.

In particular, it looks like showDuration has exactly 1 caller which is in fact showPosition, which is also responsible for setting this.positionDurationBox.position -- but only does so after calling showDuration, right before also overwriting the l10n stuff again.

I think this means that in practice this shouldn't normally hurt users because the JS code is all sync, but it does look like it's... oddly structured. We should probably be setting the duration + position and associated l10n label just once, instead of twice with potentially wrong/undefined variables.

Comment 1

[Eemeli Aro [:eemeli]]

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

In particular, it looks like showDuration has exactly 1 caller which is in fact showPosition, which is also responsible for setting this.positionDurationBox.position -- but only does so after calling showDuration, right before also overwriting the l10n stuff again.

I think this means that in practice this shouldn't normally hurt users because the JS code is all sync, but it does look like it's... oddly structured. We should probably be setting the duration + position and associated l10n label just once, instead of twice with potentially wrong/undefined variables.

Definitely agree. As I read it, $position is going to always be undefined the first time showDuration() is called. This isn't normally complained about because the second l10n.setAttributes() call shadows the first.

It would probably be simplest to inline showDuration() within showPosition(), which would make this more obvious and remove the need for passing any values through the <bdi> element's properties.

There's also some odd DOM manipulation in initPositionDurationBox() that should probably be defined directly in the HTML.

Comment 2

[:Gijs (he/him)]

Attachment: Bug 1807249 - consolidate and simplify video/audio control position and duration setting, r?mconley

Notes:

• showHours could force the use of hours, and was true if the millisecond duration was >=3600000,
  but inside formatTime we also end up using hours if rounded division of ms duration by 1000 and
  then 3600 was non-0, so it would result in strictly more cases getting hours displayed, e.g.
  a 59 minute 59 second 500ms video would get hours shown anyway. So I removed showHours.
• I consolidated attributes into markup and removed runtime initialization of the duration span
• I reorganized duration+position setting to deal with the labeling in 1 function, and a
  separate function to deal with normalizing the raw ms duration/position (which can be missing
  or infinity) and setting scrubber bounds and the 'modifier' prop which gets used by CSS logic.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1639812175b
consolidate and simplify video/audio control position and duration setting, r=mconley

Comment 4

[Sandor Molnar[:smolnar]]

Backed out for causing assertion failures in dom/security/DOMSecurityMonitor.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/7272e73dca36f2a96bf1aaafcd075c16388a0d7e

Push with failures

Failure log

Assertion failure: false, at /builds/worker/checkouts/gecko/dom/security/DOMSecurityMonitor.cpp:111

Comment 5

[:Gijs (he/him)]

(In reply to Sandor Molnar from comment #4)

Backed out for causing assertion failures in dom/security/DOMSecurityMonitor.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/7272e73dca36f2a96bf1aaafcd075c16388a0d7e

Push with failures

Failure log

Assertion failure: false, at /builds/worker/checkouts/gecko/dom/security/DOMSecurityMonitor.cpp:111

Eemeli and Christoph, can you help? The C++ stack:

#02: nsContentUtils::ParseFragmentHTML(nsTSubstring<char16_t> const&, nsIContent*, nsAtom*, int, bool, bool, int) [dom/base/nsContentUtils.cpp:5370]
#03: mozilla::dom::L10nOverlays::TranslateElement(mozilla::dom::Element&, mozilla::dom::L10nMessage const&, nsTArray<mozilla::dom::L10nOverlaysError>&, mozilla::ErrorResult&) [dom/l10n/L10nOverlays.cpp:528]
#04: mozilla::dom::DOMLocalization::ApplyTranslations(nsTArray<nsCOMPtr<mozilla::dom::Element> >&, nsTArray<mozilla::dom::Nullable<mozilla::dom::L10nMessage> >&, nsXULPrototypeDocument*, mozilla::ErrorResult&) [dom/l10n/DOMLocalization.cpp:523]
#05: mozilla::dom::DOMLocalization::TranslateElements(nsTArray<mozilla::OwningNonNull<mozilla::dom::Element> > const&, nsXULPrototypeDocument*, mozilla::ErrorResult&) [dom/l10n/DOMLocalization.cpp:357]
#06: mozilla::dom::DOMLocalization::TranslateFragment(nsINode&, mozilla::ErrorResult&) [dom/l10n/DOMLocalization.cpp:0]
#07: mozilla::dom::DOMLocalization::TranslateRoots(mozilla::ErrorResult&) [dom/l10n/DOMLocalization.cpp:413]
#08: mozilla::dom::DOMLocalization_Binding::translateRoots_promiseWrapper(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) [s3:gecko-generated-sources:ae4d48bc21d7de7cf712c1b7095f410c12291e754d3bde43d440e1bac142c42fdf34567da90dd6d15b051ab5cf9b5b66bdaad87423210fe9ebbb5b7223129994/dom/bindings/DOMLocalizationBinding.cpp::566]

(rest elided)

suggests this is from calling translateRoots() from JS. I don't understand why this would fail while calling l10n.setAttributes() and friends would succeed. AIUI the code in DOMSecurityMonitor (which I reviewed a few years back 😅) is there to prevent JS calls to innerHTML. But this isn't a JS call to innerHTML, but to translateRoots(), and I don't understand why it's being treated the same.

Comment 6

[:Gijs (he/him)]

(In reply to Eemeli Aro [:eemeli] from comment #1)

There's also some odd DOM manipulation in initPositionDurationBox() that should probably be defined directly in the HTML.

FWIW, this is a consequence of trying to address this - if I drop the l10n id or l10n args for the positionDurationBox element, the problem goes away.

Comment 7

[Eemeli Aro [:eemeli]]

It does seem a bit odd that the security warning is triggered by only one approach, and I'm not at all sure about the reasons why. However, I rather suspect that refactoring the controls following the instruction of the generateContent() code comments in the file may work around the problem:

/*
 * Pass the markup through XML parser purely for the reason of loading the localization DTD.
 * Remove it when migrate to Fluent.
 */

If that's likely to happen sometime "soon", it's probably easier to keep the current DOM manipulation, adding a comment there about this oddity, and to revisit the issue when the next refactoring step happens.

Comment 8

[:Gijs (he/him)]

(In reply to Eemeli Aro [:eemeli] from comment #7)

It does seem a bit odd that the security warning is triggered by only one approach, and I'm not at all sure about the reasons why. However, I rather suspect that refactoring the controls following the instruction of the generateContent() code comments in the file may work around the problem:

/*
 * Pass the markup through XML parser purely for the reason of loading the localization DTD.
 * Remove it when migrate to Fluent.
 */

I think this comment is obsolete after https://hg.mozilla.org/mozilla-central/rev/e881fd99de24fa48159390544dac07f373931d1c and not removing it was an oversight - maybe Mike can confirm. I don't see anything XML-y about the parsing we're doing here.

But to be clear, the DOM parser isn't what is causing us to crash, it's the l10n.translateRoots() call

If that's likely to happen sometime "soon", it's probably easier to keep the current DOM manipulation, adding a comment there about this oddity, and to revisit the issue when the next refactoring step happens.

My impression is that the l10n issue will remain in this case irrespective of refactoring. A workaround would (it appears) be to avoid assigning the l10n id on the positionDurationBox and doing so after DOM construction using document.l10n.setAttributes. I can do that here, but ISTM there's a deeper issue that perhaps needs a follow-up? I'd be interested in your opinion on that. :-)

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Oh, yeah, that comment should have been removed. Sorry about that. :/

Comment 10

[Eemeli Aro [:eemeli]]

Ah, yeah, you're right -- I was lead astray by that comment.

I would guess that the difference here could be that the translateRoots() call is made directly from "chrome code", which the security monitor is complaining about. Calling l10n.setAttributes() triggers the same code to be run, but indirectly as it's picked up by the DocumentL10n mutation observer.

Internally in Fluent, that's then leading to the flagged ParseFragmentHTML() call as the message in question includes markup, which in Fluent is handled by a second parsing pass over the formatted string.

I don't really agree with the characterisation of this as an "l10n issue", as it's the security monitor that's considering this to be bad practice. Tbf, it's not entirely wrong either, as Fluent here is ultimately doing just the sort of thing this rule is meant to prevent, though the sanitising that it's doing should make it pretty safe. So though it does show up as an l10n issue, that's not the root cause.

It might be sufficient to avoid this issue by making the l10n that's used here async, which would add a layer of indirection to the DOMLocalization internal. That would mean that this code would need a refactor as well. Maybe a mutation observer on positionDurationBox could be used to update the scrubber's aria-valuetext instead?

Comment 11

[:Gijs (he/him)]

OK. It seems the issue is that this a11y mochitest creates a <video> element in a system principal document, and so the DOMParser that the videocontrols.js code runs also has system principal, and then the DOM nodes that fluent translates and creates and then tries to parse with the innerHTML-adjacent "parse HTML" APIs also get system principal, and that makes the debug builds yell (because it's bad!).

This is all kinds of wrong for various reasons.

I don't know how hard it'd be to change the a11y mochitests to more closely mirror the majority of reality (ie "normal" web content) and not use system principal, but I guess I'll file a bug. That'd still mean that using <video> in a system principal document would break (which is maybe not entirely a bad thing esp. if the loaded data comes from somewhere non-local, but that's a separate discussion).

So I also think both the DOM parser and fluent code should downgrade to null principal instead of reusing system principal. I'll file follow-up bugs for that, too.

For now, I'll work around the immediate issue in this bug. I still need to figure out why we don't hit this same problem if the same element is translated later on...

Comment 12

[:Gijs (he/him)]

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

For now, I'll work around the immediate issue in this bug. I still need to figure out why we don't hit this same problem if the same element is translated later on...

OK, this turns out to be straightforward, it's because in that case the call to TranslateElement is async and so we can't find a JS context so the auditing code bails out. 🙃

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b436cbbbe8e
consolidate and simplify video/audio control position and duration setting, r=mconley

Comment 14

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/4b436cbbbe8e

Comment 15

[:Gijs (he/him)]

Attachment: Bug 1807249 - drop obsolete comments from various XUL components that no longer use DTDs, r?mconley

Comment 16

[Phabricator Automation]

Comment on attachment 9312060 [details]
Bug 1807249 - drop obsolete comments from various XUL components that no longer use DTDs, r?mconley

Revision D166731 was moved to bug 1810018. Setting attachment 9312060 [details] to obsolete.