Closed Bug 903372 Opened 6 years ago Closed 9 months ago

Remove support for xml:base

Categories

(Core :: XML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: annevk, Assigned: jkt)

References

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(3 files, 4 obsolete files)

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).
Blocks: 73993, 773825
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.
Keywords: dev-doc-needed
Keywords: site-compat
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
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.
Depends on: 1313278
Depends on: 1340926
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
Depends on: 1343693
Depends on: 1344102
Depends on: 1344976
Depends on: 1345343
Depends on: 1349024
Depends on: 1349059
Depends on: 1350521
Depends on: 1406277
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
(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...
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(jkt)
> 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?
Flags: needinfo?(jkt) → needinfo?(xidorn+moz)
(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).
Flags: needinfo?(xidorn+moz)
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.
Assignee: nobody → jkt
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 ?
Flags: needinfo?(amarchesini)
Yes, and XML_BASE as well.
Flags: needinfo?(amarchesini)
See Also: → 1494356
Depends on: 1483458
Depends on: 1466449
Attachment #9018871 - Flags: review?(hsivonen)
Attached patch bug-903372-generated.patch (obsolete) — Splinter Review
Attachment #9018872 - Flags: review?(hsivonen)
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.
Status: NEW → ASSIGNED
Flags: needinfo?(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?
Flags: needinfo?(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 :-)
Attachment #9018872 - Flags: review?(hsivonen) → review+
Attachment #9018871 - Flags: review?(hsivonen) → review+
> 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?
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
Attachment #8948274 - Attachment is obsolete: true
Attachment #9028741 - Attachment is obsolete: true
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!
Keywords: checkin-needed
Attachment #9028741 - Attachment is obsolete: false
Attachment #9028741 - Attachment is obsolete: true
Flags: needinfo?(jkt)
Attached patch bug-903372-generated.patch (obsolete) — Splinter Review
R+ copied over from previous generated output. The only change being rebasing over the clang format change.
Attachment #9018872 - Attachment is obsolete: true
Attachment #9028944 - Flags: review+
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!
Keywords: checkin-needed
Investigating an issue.
Keywords: checkin-needed
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.
Attachment #9028741 - Attachment is obsolete: false
Attachment #9017814 - Attachment is obsolete: true
Copying r+ from previous patch from Henri as it's generated code and hasn't changed other than code style.
Attachment #9028944 - Attachment is obsolete: true
Attachment #9031161 - Flags: review+
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!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4b317b80ef5e
https://hg.mozilla.org/mozilla-central/rev/5da87c823aee
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
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!
Flags: needinfo?(hsivonen)
Closing Bug 303478 and Bug 773825 as this has since been resolved.
(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.
Flags: needinfo?(hsivonen)

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!

You need to log in before you can comment on or make changes to this bug.