1313278 - Make nsParserUtils::ParseFragment not rely on xml:base attribute

Status: VERIFIED FIXED

(Core :: XML, defect, P3)

Description

[Xidorn Quan [:xidorn] UTC+10]

Per bug 903372 comment 3, currently nsParserUtils::ParseFragment relies on xml:base attribute to ensure the url inside the given fragment is correct.

However, xml:base attribute is something not supported in other browsers, and supporting it hurts our performance, so we want to remove it.

To make ParseFragment continue working, we may pass the base url into nsTreeSanitizer and make it rewrite all urls in the fragment instead.

Comment 1

[Andrew Overholt [:overholt]]

Henri, do you have thoughts here?

Comment 2

[Henri Sivonen (:hsivonen) (not reading bugmail until 2025-02-06)]

(In reply to Andrew Overholt [:overholt] from comment #1)
> Henri, do you have thoughts here?

The last paragraph of comment 0 is what I'd have suggested myself.

Comment 3

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Based on the dev-platform thread, it seems this may affect the Reader Mode, where we also don't have great test coverage. If that's the case we could compensate with manual testing. Is this correct Xidorn?

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

I'm not familiar with how Reader Mode is implemented and the status of its test coverage, so I may not be the right person to answer your question.

Comment 5

[:Gijs (he/him)]

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #3)
> Based on the dev-platform thread, it seems this may affect the Reader Mode,
> where we also don't have great test coverage. If that's the case we could
> compensate with manual testing. Is this correct Xidorn?

You could do manual testing based on the testcase in bug 1280888 comment #0, yes. I've also filed bug 1340799 to add an automated test, but I don't know if we can get that addressed before this bug is fixed.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

It seems to me that we can simply remove the corresponding code without any change to tree sanitizer, if the only users are feed reader and reader mode.

For feed reader, since it always uses the same url as the feed it displays, any URL inside it is already resolved based on the
url of the feed. The reader mode rewrites url itself, so it's less concerned.

I construct some simple examples, and I don't see any breakage after removing the code.

In addition, majority big websites nowadays use CDN for their static content, and thus urls are usually absolute path to some other domain. They are unlikely affected by this removal. That says, even it breaks something, I wouldn't expect the impact to be significant.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1313278 - Remove code of adding xml:base attribute in nsParserUtils::ParseFragment.

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

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c5ed2a53a6489006d696aae2988b7e988e9c9ad

Comment 9

[Henri Sivonen (:hsivonen) (not reading bugmail until 2025-02-06)]

Comment on attachment 8839318 [details]
Bug 1313278 - Remove code of adding xml:base attribute in nsParserUtils::ParseFragment.

https://reviewboard.mozilla.org/r/113988/#review115520

::: parser/html/nsParserUtils.cpp:117
(Diff revision 1)
>  }
>  
>  NS_IMETHODIMP
>  nsParserUtils::ParseFragment(const nsAString& aFragment,
>                               bool aIsXML,
>                               nsIURI* aBaseURI,

Please remove the `aBaseURI` argument.

::: parser/html/nsParserUtils.cpp:133
(Diff revision 1)
>  
>  NS_IMETHODIMP
>  nsParserUtils::ParseFragment(const nsAString& aFragment,
>                               uint32_t aFlags,
>                               bool aIsXML,
>                               nsIURI* aBaseURI,

Here, too.

::: parser/html/nsParserUtils.cpp
(Diff revision 1)
>    nsresult rv = NS_OK;
>    AutoTArray<nsString, 2> tagStack;
> -  nsAutoCString base, spec;
> +  nsCOMPtr<nsIContent> fragment;
>    if (aIsXML) {
>      // XHTML
> -    if (aBaseURI) {

Not sure if comments made outside the diff area show up so:
Please remove the `aBaseURI` argument from this method and the callers.

Other than that looks good, but marking r-, because it's not clear to me if ReviewBoard supports "r+ with open issues".

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Henri Sivonen (:hsivonen) from comment #9)
> Not sure if comments made outside the diff area show up so:
> Please remove the `aBaseURI` argument from this method and the callers.

We probably cannot remove them at this moment, because this function is exposed in nsIScriptableUnescapeHTML.idl and  nsIParserUtils.idl, and aBaseURI is not the last parameter of the functions.

A brief searching in addons repo indicates that there are many extensions call `parseFragment` (both this one and the other one). Although most of them pass `null` for aBaseURI, they do use parameters after it, so removing this parameter would likely break all of them.

We probably should keep it around, and perhaps file a followup bug to remove this parameter later when we stop supporting the current extension system.

WDYT?

Comment 11

[Henri Sivonen (:hsivonen) (not reading bugmail until 2025-02-06)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #10)
> (In reply to Henri Sivonen (:hsivonen) from comment #9)
> > Not sure if comments made outside the diff area show up so:
> > Please remove the `aBaseURI` argument from this method and the callers.
> 
> We probably cannot remove them at this moment, because this function is
> exposed in nsIScriptableUnescapeHTML.idl and  nsIParserUtils.idl, and
> aBaseURI is not the last parameter of the functions.
> 
> A brief searching in addons repo indicates that there are many extensions
> call `parseFragment` (both this one and the other one). Although most of
> them pass `null` for aBaseURI, they do use parameters after it, so removing
> this parameter would likely break all of them.
> 
> We probably should keep it around, and perhaps file a followup bug to remove
> this parameter later when we stop supporting the current extension system.
> 
> WDYT?

OK. Makes sense.

Comment 12

[Henri Sivonen (:hsivonen) (not reading bugmail until 2025-02-06)]

Comment on attachment 8839318 [details]
Bug 1313278 - Remove code of adding xml:base attribute in nsParserUtils::ParseFragment.

https://reviewboard.mozilla.org/r/113988/#review115524

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ebea9daabb96
Remove code of adding xml:base attribute in nsParserUtils::ParseFragment. r=hsivonen

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 15

[roxleitan]

Verified as fixed using the latest Nightly 54.0a1 (2017-02-26) on  Windows 10 x64, Mac OS X 10.11 and Ubuntu 16.04.