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

VERIFIED FIXED in Firefox 54

Status

()

Core
XML
P3
normal
VERIFIED FIXED
a year ago
11 months ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla54
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox52 affected, firefox54 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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.
Henri, do you have thoughts here?
Flags: needinfo?(hsivonen)
Priority: -- → P3
(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.
Flags: needinfo?(hsivonen)
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?
Flags: qe-verify?
Flags: needinfo?(xidorn+moz)
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.
Flags: needinfo?(xidorn+moz)

Comment 5

11 months ago
(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.
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 hidden (mozreview-request)
Assignee: nobody → xidorn+moz

Comment 9

11 months ago
mozreview-review
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".
Attachment #8839318 - Flags: review?(hsivonen) → review-
(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?
Flags: needinfo?(hsivonen)
(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.
Flags: needinfo?(hsivonen)

Comment 12

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

https://reviewboard.mozilla.org/r/113988/#review115524
Attachment #8839318 - Flags: review- → review+
Blocks: 1341220

Comment 13

11 months ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ebea9daabb96
Remove code of adding xml:base attribute in nsParserUtils::ParseFragment. r=hsivonen

Comment 14

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ebea9daabb96
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Flags: qe-verify? → qe-verify+
Depends on: 1342348
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.
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified
You need to log in before you can comment on or make changes to this bug.