1343693 - Add a telemetry for xml:base with setting URL value via CSSOM on style attribute

Status: RESOLVED WONTFIX

(Core :: DOM: CSS Object Model, enhancement)

Description

[Xidorn Quan [:xidorn] UTC+10]

So far, the telemetry for xml:base doesn't look quite well. It is reported to affect over 3% of pages (although only about 0.5% of documents), so we may still need to leave it around for a while before further investigation.

Since the only thing I am looking to improve from removing xml:base at the moment is changing style attribute of elements via CSSOM API.

Currently, URL inside style attribute from the initial parsing doesn't respect xml:base at all, so the CSSOM setter respecting xml:base seems more likely to be a footgun rather than something useful.

So I'd like to measure and remove xml:base's affection in this specific case.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1343693 - Add a telemetry for setting relative url via CSSOM to style attribute with effect from xml:base.

Review commit: https://reviewboard.mozilla.org/r/116498/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116498/

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

This is really just adding a telemetry, because xml:base is already deprecated in bug 1340926, and no message would be shown for this telemetry because no warning message is added. This is done this way to help uplift as well.

Comment 3

[(no longer active)]

Comment on attachment 8842725 [details]
Bug 1343693 - Add a telemetry for setting relative url via CSSOM to style attribute with effect from xml:base.

https://reviewboard.mozilla.org/r/116498/#review118286

::: layout/style/nsDOMCSSDeclaration.h:152
(Diff revision 1)
>      mozilla::css::Loader* MOZ_UNSAFE_REF("user of CSSParsingEnviroment must hold an owning "
>                                           "reference; reference counting here has unacceptable "
>                                           "performance overhead (see bug 649163)") mCSSLoader;
> +    // This is set to an element when mBaseURI is affected by some
> +    // xml:base attribute on it or its ancestors.
> +    mozilla::dom::Element* mBaseElementWithActiveXMLBase;

What guarantees that this element will be alive when you dereference the pointer?  You should probably make this a strong ref.

::: layout/style/nsDOMCSSDeclaration.cpp:425
(Diff revision 1)
> +  int32_t pos = 2;
> +  // Searching if there is any relative url value in the given text.
> +  while ((pos = aText.FindChar('(', pos + 1)) >= 0) {
> +    // Find a url function.
> +    if (!nsDependentSubstring(aText, pos - 3, 3).LowerCaseEqualsLiteral("url")) {
> +      continue;

Can't we use our real CSS parser here?  This matches url() as well as curl(), for example.

::: layout/style/nsDOMCSSDeclaration.cpp:435
(Diff revision 1)
> +      if (aText[pos] == '"' || aText[pos] == '\'') {
> +        ++pos;
> +      }
> +    }
> +    // Check whether the url starts with /https?:\/\//. If not,
> +    // record that we found a relative url.

Nope, you might have found an FTP URL.  ;-)  You should just parse the URL normally instead of trying to do it manually yourself.

(Note that I'm not being unnecessarily pedantic here, who knows if there's some old code out there feeding some junk to us that would make this code send highly misleading telemetry.  Since we can't see the data the telemetry is coming from, we should be really careful to do the probe collections correctly.)

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Doesn't seem to be relevant anymore. xml:base on style attribute has been disabled since last release.