1298509 - nsReferencedElement is not notified for elements that are removed through document.open()

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Markus Stange [:mstange]]

Attachment: testcase

See the testcase. It should print PASS, not FAIL.

I'm not sure if this is causing security bugs, but I'm filing this as a security bug because the testcase shows how to trigger bug 1287316.

Canvas filters observe the referenced SVG filter elements through CanvasFilterChainObserver, which uses nsReferencedElement to observe the referenced ID. If the document is destroyed through document.open(), we call mIdentifierMap.Clear() with this callstack:

#4  0x00007efe0b0cf077 in nsDocument::DestroyElementMaps() (this=0x7efdf47d8000)
    at /home/mstange/code/mozilla-central/dom/base/nsDocument.cpp:9512
#5  0x00007efe0b0b0ca1 in nsDocument::ResetToURI(nsIURI*, nsILoadGroup*, nsIPrincipal*) (this=0x7efdf47d8000, aURI=0x7efde5358900, aLoadGroup=0x7efdfcb84450, aPrincipal=0x7efde3fddd70)
    at /home/mstange/code/mozilla-central/dom/base/nsDocument.cpp:2175
#6  0x00007efe0c4fb71d in nsHTMLDocument::ResetToURI(nsIURI*, nsILoadGroup*, nsIPrincipal*) (this=0x7efdf47d8000, aURI=0x7efde5358900, aLoadGroup=0x7efdfcb84450, aPrincipal=0x7efde3fddd70)
    at /home/mstange/code/mozilla-central/dom/html/nsHTMLDocument.cpp:247
#7  0x00007efe0b0b08c2 in nsDocument::Reset(nsIChannel*, nsILoadGroup*) (this=0x7efdf47d8000, aChannel=0x7efdf47b2888, aLoadGroup=0x7efdfcb84450) at /home/mstange/code/mozilla-central/dom/base/nsDocument.cpp:2123
#8  0x00007efe0c4fb6a1 in nsHTMLDocument::Reset(nsIChannel*, nsILoadGroup*) (this=0x7efdf47d8000, aChannel=0x7efdf47b2888, aLoadGroup=0x7efdfcb84450) at /home/mstange/code/mozilla-central/dom/html/nsHTMLDocument.cpp:234
#9  0x00007efe0c501846 in nsHTMLDocument::Open(JSContext*, nsAString_internal const&, nsAString_internal const&, mozilla::ErrorResult&) (this=0x7efdf47d8000, cx=0x7efdfb7e5000, aType=..., aReplace=..., rv=...)
    at /home/mstange/code/mozilla-central/dom/html/nsHTMLDocument.cpp:1665
#10 0x00007efe0c500106 in nsHTMLDocument::Open(nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, JSContext*, unsigned char, nsISupports**) (this=0x7efdf47d8000, aContentTypeOrUrl=..., aReplaceOrName=..., aFeatures=..., cx=0x7efdfb7e5000, aOptionalArgCount=1 '\001', aReturn=0x7ffd29081d10)
    at /home/mstange/code/mozilla-central/dom/html/nsHTMLDocument.cpp:1368
#11 0x00007efe0c5025c6 in nsHTMLDocument::WriteCommon(JSContext*, nsAString_internal const&, bool) (this=0x7efdf47d8000, cx=0x7efdfb7e5000, aText=..., aNewlineTerminate=false)
    at /home/mstange/code/mozilla-central/dom/html/nsHTMLDocument.cpp:1880
#12 0x00007efe0c5020f6 in nsHTMLDocument::WriteCommon(JSContext*, mozilla::dom::Sequence<nsString> const&, bool, mozilla::ErrorResult&) (this=0x7efdf47d8000, cx=0x7efdfb7e5000, aText=..., aNewlineTerminate=false, rv=...)
    at /home/mstange/code/mozilla-central/dom/html/nsHTMLDocument.cpp:1814
#13 0x00007efe0c5029b8 in nsHTMLDocument::Write(JSContext*, mozilla::dom::Sequence<nsString> const&, mozilla::ErrorResult&) (this=0x7efdf47d8000, cx=0x7efdfb7e5000, aText=..., rv=...)
    at /home/mstange/code/mozilla-central/dom/html/nsHTMLDocument.cpp:1936
#14 0x00007efe0bf73c82 in mozilla::dom::HTMLDocumentBinding::write(JSContext*, JS::Handle<JSObject*>, nsHTMLDocument*, JSJitMethodCallArgs const&) (cx=0x7efdfb7e5000, obj=(JSObject * const) 0x7efdf2bf8100 [object Proxy], self=0x7efdf47d8000

So later, when the SVG filter element is unbound from the document tree, we get to nsDocument::RemoveFromIdTable but mIdentifierMap is already empty so we don't notify the nsReferencedElement. For reference, here's the callstack when the SVG filter element is removed:

#0  0x00007efe0b0b51e0 in nsDocument::RemoveFromIdTable(mozilla::dom::Element*, nsIAtom*) (this=0x7efdf47d8000, aElement=0x7efde3ff9c70, aId=0x7efdfa902640) at /home/mstange/code/mozilla-central/dom/base/nsDocument.cpp:3033
#1  0x00007efe0af9a0ef in mozilla::dom::Element::RemoveFromIdTable() (this=0x7efde3ff9c70)
    at /home/mstange/code/mozilla-central/dom/base/Element.cpp:1042
#2  0x00007efe0af9d2f2 in mozilla::dom::Element::UnbindFromTree(bool, bool) (this=0x7efde3ff9c70, aDeep=true, aNullParent=false) at /home/mstange/code/mozilla-central/dom/base/Element.cpp:1780
#3  0x00007efe0af9d8fa in mozilla::dom::Element::UnbindFromTree(bool, bool) (this=0x7efde3ff9b80, aDeep=true, aNullParent=false) at /home/mstange/code/mozilla-central/dom/base/Element.cpp:1920
#4  0x00007efe0af9d8fa in mozilla::dom::Element::UnbindFromTree(bool, bool) (this=0x7efdfcbf2060, aDeep=true, aNullParent=false) at /home/mstange/code/mozilla-central/dom/base/Element.cpp:1920
#5  0x00007efe0cab9c21 in mozilla::dom::SVGSVGElement::UnbindFromTree(bool, bool) (this=0x7efdfcbf2060, aDeep=true, aNullParent=false) at /home/mstange/code/mozilla-central/dom/svg/SVGSVGElement.cpp:777
#6  0x00007efe0af9d8fa in mozilla::dom::Element::UnbindFromTree(bool, bool) (this=0x7efde448bce0, aDeep=true, aNullParent=false) at /home/mstange/code/mozilla-central/dom/base/Element.cpp:1920
#7  0x00007efe0c4eb12b in nsGenericHTMLElement::UnbindFromTree(bool, bool) (this=0x7efde448bce0, aDeep=true, aNullParent=false) at /home/mstange/code/mozilla-central/dom/html/nsGenericHTMLElement.cpp:516
#8  0x00007efe0c3ffc55 in mozilla::dom::HTMLBodyElement::UnbindFromTree(bool, bool) (this=0x7efde448bce0, aDeep=true, aNullParent=false) at /home/mstange/code/mozilla-central/dom/html/HTMLBodyElement.cpp:358
#9  0x00007efe0af9d8fa in mozilla::dom::Element::UnbindFromTree(bool, bool) (this=0x7efde3fdb690, aDeep=true, aNullParent=true) at /home/mstange/code/mozilla-central/dom/base/Element.cpp:1920
#10 0x00007efe0c4eb12b in nsGenericHTMLElement::UnbindFromTree(bool, bool) (this=0x7efde3fdb690, aDeep=true, aNullParent=true) at /home/mstange/code/mozilla-central/dom/html/nsGenericHTMLElement.cpp:516
#11 0x00007efe0c4a6ec5 in mozilla::dom::HTMLSharedElement::UnbindFromTree(bool, bool) (this=0x7efde3fdb690, aDeep=true, aNullParent=true) at /home/mstange/code/mozilla-central/dom/html/HTMLSharedElement.cpp:312
#12 0x00007efe0b0b0e3e in nsDocument::ResetToURI(nsIURI*, nsILoadGroup*, nsIPrincipal*) (this=0x7efdf47d8000, aURI=0x7efde5358900, aLoadGroup=0x7efdfcb84450, aPrincipal=0x7efde3fddd70)
    at /home/mstange/code/mozilla-central/dom/base/nsDocument.cpp:2192
#13 0x00007efe0c4fb71d in nsHTMLDocument::ResetToURI(nsIURI*, nsILoadGroup*, nsIPrincipal*) (this=0x7efdf47d8000, aURI=0x7efde5358900, aLoadGroup=0x7efdfcb84450, aPrincipal=0x7efde3fddd70)
    at /home/mstange/code/mozilla-central/dom/html/nsHTMLDocument.cpp:247
#14 0x00007efe0b0b08c2 in nsDocument::Reset(nsIChannel*, nsILoadGroup*) (this=0x7efdf47d8000, aChannel=0x7efdf47b2888, aLoadGroup=0x7efdfcb84450) at /home/mstange/code/mozilla-central/dom/base/nsDocument.cpp:2123
#15 0x00007efe0c4fb6a1 in nsHTMLDocument::Reset(nsIChannel*, nsILoadGroup*) (this=0x7efdf47d8000, aChannel=0x7efdf47b2888, aLoadGroup=0x7efdfcb84450) at /home/mstange/code/mozilla-central/dom/html/nsHTMLDocument.cpp:234
#16 0x00007efe0c501846 in nsHTMLDocument::Open(JSContext*, nsAString_internal const&, nsAString_internal const&, mozilla::ErrorResult&) (this=0x7efdf47d8000, cx=0x7efdfb7e5000, aType=..., aReplace=..., rv=...)
    at /home/mstange/code/mozilla-central/dom/html/nsHTMLDocument.cpp:1665
#17 0x00007efe0c500106 in nsHTMLDocument::Open(nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, JSContext*, unsigned char, nsISupports**) (this=0x7efdf47d8000, aContentTypeOrUrl=..., aReplaceOrName=..., aFea 

The call to nsDocument::DestroyElementMaps() at http://searchfox.org/mozilla-central/rev/f80822840bc5f3d2d3cae3ece621ddbce72e7f54/dom/base/nsDocument.cpp#2201 has this comment:
  // Destroy link map now so we don't waste time removing
  // links one by one
It was added in bug 304759.

I'm not sure what to do about this. Boris, do you have an opinion?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Bug 304759 was just about the link map.

The change to clear all the maps was from bug 564863.  I think the right answer is probably to either go back to just clearing the link map in ResetToURI or to clear only the non-id parts of mIdentifierMap, or to go through mIdentifierMap before clearing it and notify all the referenced elements that they now reference nothing.

This last is probably simplest.  In fact, we should perhaps do it in DestroyElementMaps, so it's always done, for safety.

Comment 2

[Markus Stange [:mstange]]

The last one sounds great, thank you.

Comment 3

[Markus Stange [:mstange]]

Attachment: patch

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8786084 [details] [diff] [review]
patch

r=me

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 6

[Andrew McCreight [:mccr8]]

Unhiding, as bug 1287316 is unhidden. Markus, can the patch here land?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Hsin-yi, this might need a different owner to finish driving this in. I just checked, and this is NOT fixed by bug 1489308, because while we no longer ResetToURI in document.open() we are still doing Document::DisconnectNodeTree which is still calling DestroyElementMaps().

Comment 8

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #7)

Hsin-yi, this might need a different owner to finish driving this in. I just checked, and this is NOT fixed by bug 1489308, because while we no longer ResetToURI in document.open() we are still doing Document::DisconnectNodeTree which is still calling DestroyElementMaps().

Thank you for bringing it up. I'll take a look and work on a plan for it.

Comment 9

[Edgar Chen [:edgar]]

Attachment: Bug 1298509 - Notify identifier map entries if they get removed via DestroyElementMaps;

Comment 10

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=14ba5581561420052747ce8ff7d8e0c4a7887634

Comment 11

[Edgar Chen [:edgar]]

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/908c9c793e92
Notify identifier map entries if they get removed via DestroyElementMaps; r=bzbarsky

Comment 13

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/908c9c793e92