903372 - Remove support for xml:base

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Anne (:annevk)]

Per http://dump.testsuite.org/xml:base/basic.xml none of IE/Safari/Chrome support it. We should take this opportunity. Fate of Node.prototype.baseURI to be decided on separately (as Safari/Chrome do support that).

Comment 1

[Boris Zbarsky [:bzbarsky]]

I thought we had XBL/SVG code that depended on this stuff, but I don't see it now.  I guess I was thinking of bug 211128 or something.

I'm all in favor or ripping out xml:base, and then making GetBaseURI return a raw nsIURI*!  I'll post to .platform accordingly.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think Node.baseURI might still make sense. But as something that's only settable on a per-node basis using internal APIs. That way we can set it for shadowroot nodes etc, without having to support xml:base.

So yes, I'm definitely in favor of removing xml:base support. What that means with regards to the Node.baseURI API and its implementation is less clear to me.

Comment 3

[Henri Sivonen (:hsivonen)]

I think it makes sense to get rid of xml:base. However, please note that Firefox's built-in feed reader uses it for base URL management:
http://mxr.mozilla.org/mozilla-central/source/parser/html/nsParserUtils.cpp#173
http://mxr.mozilla.org/mozilla-central/source/parser/html/nsParserUtils.cpp#214

One possibility would be passing the base URL to nsTreeSanitizer and making it rewrite URL attributes to use absolute URLs.

Comment 4

[Anne (:annevk)]

Per https://www.w3.org/Bugs/Public/show_bug.cgi?id=20976#c8 we might need most of this functionality after all for components.

Comment 5

[Anne (:annevk)]

Per https://www.w3.org/Bugs/Public/show_bug.cgi?id=20976#c28 comment 4 is now wrong. We should remove xml:base and when we get declarative components we should ensure to not bake in the same base URL mistakes as xml:base had (and not reuse xml:base as a name).

Comment 6

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

https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/ZvArHAXyAHM/ftybb0waCKYJ

Comment 7

[Cameron McCormack (:heycam)]

We also just resolved in the SVG WG to remove xml:base's effect in SVG, since no other browser pays attention to it.

Comment 8

[alta88]

For xml documents following the Atom feed spec[1], the baseURI is used to resolve relative urls. The new warning in the Thunderbird feed parser can be worked around with node.getAttribute("xml:base"), unless there's some less hacky feeling way to get such a thing in xml (I do not notice |base| being an xml node property).

Strictly per Atom spec, comment 3 isn't correct although would work in some/many cases. And removing xml:base from the Firefox feed parser test in Bug 1313278 will, sadly, not magically make the parser Atom spec compliant. Perhaps sayre should be consulted.

[1] https://tools.ietf.org/html/rfc4287#section-2

Comment 9

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 903372 - Remove support for xml:base

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

Comment 10

[Jonathan Kingston [:jkt] he/him]

The wpt will need to change for this to pass try: html/infrastructure/urls/dynamic-changes-to-base-urls/dynamic-urls.sub.xhtml looks like Chrome ignores this. 

I'm not sure if this can be fixed in the feed parser, I implemented in my patch the removal in FeedWriter.js like in Bug 1344976

Comment 11

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8948274 [details]
Bug 903372 - Remove support for xml:base

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217772/diff/1-2/

Comment 12

[Jonathan Kingston [:jkt] he/him]

Usage of this per document is at 0.00% according to our telemetry:
https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2018-02-03&keys=__none__!__none__!__none__&max_channel_version=nightly%252F60&measure=USE_COUNTER2_DEPRECATED_XMLBaseAttribute_DOCUMENT&min_channel_version=null&processType=*&product=Firefox&sanitize=0&sort_keys=submissions&start_date=2018-01-22&table=0&trim=1&use_submission_date=0

This appears to be completely safe to rip out. Shall I submit the wpt separately?

Comment 13

[:Gijs (out for now; he/him)]

(In reply to Jonathan Kingston [:jkt] from comment #12)
> Usage of this per document is at 0.00% according to our telemetry:
> https://telemetry.mozilla.org/new-pipeline/dist.html#!
> cumulative=0&end_date=2018-02-03&keys=__none__!__none__!
> __none__&max_channel_version=nightly%252F60&measure=USE_COUNTER2_DEPRECATED_X
> MLBaseAttribute_DOCUMENT&min_channel_version=null&processType=*&product=Firef
> ox&sanitize=0&sort_keys=submissions&start_date=2018-01-
> 22&table=0&trim=1&use_submission_date=0
> 
> This appears to be completely safe to rip out. Shall I submit the wpt
> separately?

I don't really know what 'wpt' refers to here... If you mean 'wpt tests', I don't know anything about our web platform tests. Xidorn might be a better person to ask.

AIUI ripping this out is still blocked on fixing the feed usage in bug 1344976...

Comment 14

[Jonathan Kingston [:jkt] he/him]

> I don't really know what 'wpt' refers to here

Yeah 'wpt tests'.

Xidorn, do you know if I should modify the wpt tests in a separate patch? I know they are upstreamed just have never touched them.

> AIUI ripping this out is still blocked on fixing the feed usage in bug 1344976...

Do we really need to fix feed usage when the adoption is so low? Also it looks to me that feeds work from FeedProcessor.js rather than this code? Given that the attribute will still work, if that is the case maybe we are ok?

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Jonathan Kingston [:jkt] from comment #14)
> Xidorn, do you know if I should modify the wpt tests in a separate patch? I
> know they are upstreamed just have never touched them.

You can modify the tests in the same patch and have them reviewed together. Personally I think it is the better approach as reviewer can see both code change and test change. I think they are upstreamed regularly as a bunch (i.e. not in a commit-base manner).

Comment 16

[:Gijs (out for now; he/him)]

All the deps are closed but there's still a lot of stuff in-tree left that references this...

Comment 17

[Jonathan Kingston [:jkt] he/him]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=371959be178f744fd750efaaea23c27c63419b73

Rebased and puttting on phab as we don't have the failing wpt test or feeds I think this is fine to rip out now.

Comment 18

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 903372 - Removal of xml:base r?baku

Comment 19

[Jonathan Kingston [:jkt] he/him]

Do you think we should be removing this from the .java html parser too: https://searchfox.org/mozilla-central/rev/3d989d097fa35afe19e814c6d5bc2f2bf10867e1/parser/html/javasrc/AttributeName.java#804 ?

Comment 20

[Andrea Marchesini [:baku]]

Yes, and XML_BASE as well.

Comment 21

[Jonathan Kingston [:jkt] he/him]

Attachment: bug-903372-parser.patch

Comment 22

[Jonathan Kingston [:jkt] he/him]

Attachment: bug-903372-generated.patch

Comment 23

[Jonathan Kingston [:jkt] he/him]

In the UnbindFromTree implementations for HTMLAnchorElement HTMLAreaElement HTMLLinkElement nsMathMLElement and SVGAElement

There is the following code:
  // If this link is ever reinserted into a document, it might
  // be under a different xml:base, so forget the cached state now.
  Link::ResetLinkState(false, Link::ElementHasHref());

When I removed this I see many of your debug assertions firing:

[Child 30099, Main Thread] ###!!! ASSERTION: Document already knows about this Link!: '!entry', file /home/jonathan/debugging/mozilla-unified/dom/base/nsIDocument.h, line 2351

I'm guessing for anything with visible content the link might have been loaded in the time it has been deattached so probably only HTMLLinkElement is safe to remove this ResetLinkState from.
Is that assumption correct or is there perhaps another way we could reset link state in a smarter way?

Either way I can update the comments to these calls to reflect the current reason we reset the link.

Comment 24

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jonathan Kingston [:jkt] from comment #23)
> In the UnbindFromTree implementations for HTMLAnchorElement HTMLAreaElement
> HTMLLinkElement nsMathMLElement and SVGAElement
> 
> There is the following code:
>   // If this link is ever reinserted into a document, it might
>   // be under a different xml:base, so forget the cached state now.
>   Link::ResetLinkState(false, Link::ElementHasHref());
> 
> When I removed this I see many of your debug assertions firing:

FWIW that's not my assertion, I last touched that line but I was just moving code around.

> I'm guessing for anything with visible content the link might have been
> loaded in the time it has been deattached so probably only HTMLLinkElement
> is safe to remove this ResetLinkState from.

I'm not really sure what the 'visible content' is supposed to mean? Visible as in? I don't understand the reasoning for which HTMLLinkElement would be any different.

> Is that assumption correct or is there perhaps another way we could reset
> link state in a smarter way?

In any case it's definitely not fine to not call Link::ResetLinkState, if you do that you're leaving a dangling pointer in the mStyledLinks hashtable.

You can probably trigger a UAF with that line removed if you add a link to the document, remove it, wait for the link to be GCd / CCd, and then do history.pushState({ .. }).

> Either way I can update the comments to these calls to reflect the current
> reason we reset the link.

The reason to reset the link is because we only track links that are connected to the document, so we refresh the href correctly if they have a relative href and the document URI or base URI changes, for example.

I wonder, shouldn't this bug remove the core xml:base functionality as well, which is:

  https://searchfox.org/mozilla-central/rev/0ec9f5972ef3e4a479720630ad7285a03776cdfc/dom/base/FragmentOrElement.cpp#368

If not, why not?

Comment 25

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #24)
> I wonder, shouldn't this bug remove the core xml:base functionality as well,
> which is:
> 
>  
> https://searchfox.org/mozilla-central/rev/
> 0ec9f5972ef3e4a479720630ad7285a03776cdfc/dom/base/FragmentOrElement.cpp#368
> 
> If not, why not?

Oh you did, nvm about this :-)

Comment 26

[Jonathan Kingston [:jkt] he/him]

> I'm not really sure what the 'visible content' is supposed to mean? Visible as in? I don't understand the reasoning for which HTMLLinkElement would be any different.

Ah I assume I just don't know enough about how the style system works. I'll just update the comments thank you!

---


:bz there are some tests that are breaking try: https://hg.mozilla.org/try/rev/ea66fcb3084a0d1601475e37506c13b3ad182477

dom/html/test/test_bug481335.xhtml
layout/reftests/svg/moz-only/xbl-grad-ref--grad-in-binding-02.svg and layout/reftests/svg/moz-only/xbl-grad-ref--grad-in-binding-02.xbl
layout/reftests/svg/moz-only/xbl-grad-ref--grad-in-binding-04.svg and layout/reftests/svg/moz-only/xbl-grad-ref--grad-in-binding-04.xbl 

I think all three are able to be removed with this patch (I removed the xbl specific code that renders svg in xbl, it can be added back in as an exepmtion but I think only datetimebox.xml needs this as per: https://bugzilla.mozilla.org/show_bug.cgi?id=1494356#c19)

test_bug481335.xhtml appears to be something you worked on https://bugzilla.mozilla.org/show_bug.cgi?id=481335 is it still needed or should I refactor it to not use xml:base?

Comment 27

[Boris Zbarsky [:bzbarsky]]

The xml:base tests in test_bug481335.xhtml are just testing that we invalidate the cached href when moving the link around to  a new xml:base (which should change its .href).  If we don't support xml:base, we can just remove that part of the test, since moving the link around would not change its .href.

Comment 28

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 903372 - Removal of xml:base r?baku

Comment 29

[Jonathan Kingston [:jkt] he/him]

bug-903372-generated.patch and the phabricator patch need checking in as part of this change.

The bug-903372-generated.patch HTMLParser can be added into tree if it sticks.

Thank you!

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6562c2392ac9
Removal of xml:base. r=baku
https://hg.mozilla.org/integration/autoland/rev/bf919773f9d1
Remove xml:base from parser. r=hsivonen

Comment 31

[Andreea Pavel [:apavel]]

Backed out for reftest and wpt failrues:

Push with failures:  https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=214743145&revision=bf919773f9d19a9032c1b5805d08723843dac701

Failure logs:
Reftest: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=214745870&repo=autoland&lineNumber=7730

WPT: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=214745886&repo=autoland&lineNumber=3387

Backout: https://hg.mozilla.org/integration/autoland/rev/d4574d73506372785c910a68447ed4354d7ea561

Comment 32

[Jonathan Kingston [:jkt] he/him]

Attachment: bug-903372-generated.patch

R+ copied over from previous generated output. The only change being rebasing over the clang format change.

Comment 33

[Jonathan Kingston [:jkt] he/him]

checkin-needed for the same patches. In the same order as before (phabricator first then the generated code removal). HOwever ordering shouldn't matter if they are in the same push.

[1] https://phabricator.services.mozilla.com//D8965
[2] bug-903372-generated.patch - https://bug903372.bmoattachments.org/attachment.cgi?id=9028944

The tests were marked as failing for the xbl code as we don't need this anymore as we changed how datetimebox was coded. The wpt tests were marked as passing as the web platform now expects this code to be removed. The only other changes were to rebase past the clang format change.

Thanks!

Comment 34

[Jonathan Kingston [:jkt] he/him]

Investigating an issue.

Comment 35

[Jonathan Kingston [:jkt] he/him]

Bug 1511414 outlines some of the issues in the generated file, it won't compile again and I don't have time today to fix this.

Comment 36

[Jonathan Kingston [:jkt] he/him]

Attachment: bug-903372-generated2.patch

Copying r+ from previous patch from Henri as it's generated code and hasn't changed other than code style.

Comment 37

[Jonathan Kingston [:jkt] he/him]

checkin-needed for the same patches. In the same order as before (phabricator first then the generated code removal). However ordering shouldn't matter if they are in the same push.

[1] https://phabricator.services.mozilla.com//D13457
[2] bug-903372-generated2.patch - https://bugzilla.mozilla.org/attachment.cgi?id=9031161

Thanks!

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b317b80ef5e
Removal of xml:base. r=baku
https://hg.mozilla.org/integration/autoland/rev/5da87c823aee
Remove xml:base from parser. r=hsivonen

Comment 39

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/4b317b80ef5e
https://hg.mozilla.org/mozilla-central/rev/5da87c823aee

Comment 40

[Jonathan Kingston [:jkt] he/him]

Hey Henri,

Would you be able to merge the parser patch (bug-903372-parser.patch) now that the other has landed?

I'm not sure if there is an easier way to flag you for these?

Thanks!

Comment 41

[Jonathan Kingston [:jkt] he/him]

Closing Bug 303478 and Bug 773825 as this has since been resolved.

Comment 42

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/4b317b80ef5e
https://hg.mozilla.org/mozilla-central/rev/5da87c823aee

Comment 43

[Henri Sivonen (:hsivonen)]

(In reply to Jonathan Kingston [:jkt] from comment #40)
> Would you be able to merge the parser patch (bug-903372-parser.patch) now
> that the other has landed?

https://hg.mozilla.org/projects/htmlparser/rev/7b33409159753ac6397e3e7cee2810da7814695e

> I'm not sure if there is an easier way to flag you for these?

This seems to be the best way we have. Thanks.

Comment 44

[ExE Boss]

I have updated the following MDN pages:
- https://developer.mozilla.org/docs/Mozilla/Firefox/Releases/66
- https://developer.mozilla.org/docs/XML_introduction/xml:base

Comment 45

[Kohei Yoshino]

Posted site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/xml-base-attribute-is-no-longer-supported/

Comment 46

[Chris Mills [:cmills]]

Setting this to dev-doc-complete. The docs page has been updated, and a note has been added to the Fx66 rel notes. Thanks Exe Boss!