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).

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.

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.

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.

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

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).

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

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

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

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?

(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...

> 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?

(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).

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

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.

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 ?

Yes, and XML_BASE as well.

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.

(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?

(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 :-)

> 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?

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.

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!

Pushed by ryanvm@gmail.com: 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

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

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

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!

Investigating an issue.

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.

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

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!

Pushed by ryanvm@gmail.com: 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

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

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!

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

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

(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.

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

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