621618 - "ASSERTION: Removing id entry that doesn't exist" with <svg:use>

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase (triggers assertion when closed)

1. Load the testcase.
2. Close the tab or window or quit Firefox.

###!!! ASSERTION: Removing id entry that doesn't exist: '!aElement->GetOwnerDoc() || !aElement->GetOwnerDoc()->IsHTML() || mIdContentList.IndexOf(aElement) >= 0', file content/base/src/nsDocument.cpp, line 425

(Same assertion as in bug 593302, which had a very different testcase.)

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

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

Could be bad, but I need to investigate. Definitely a regression since id handling is much more sensitive now.

Comment 3

[Johnny Stenback (:jst)]

I think we can ship 2.0 w/o this, but if this is more severe than we think we'll reconsider.

Comment 4

[Jesse Ruderman]

This can lead to additional badness, such as

ASSERTION: No Document Request!: 'mDocumentRequest', file uriloader/base/nsDocLoader.cpp

ASSERTION: Firing OnStateChange(...) notification with a NULL request!: 'aRequest', file uriloader/base/nsDocLoader.cpp

Comment 6

[John Schoenick [:johns]]

This is being hit by dom/plugins/test/mochitest/test_bug813906.html

Comment 7

[Boris Zbarsky [:bzbarsky]]

The basic issue here is that things only add themselves to the id table if they're not anonymous, but they remove unconditionally....

Comment 8

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

The patch in bug 741295 would fix this I'm pretty sure.

Comment 9

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

Hmm.... on second thought, maybe it wouldn't.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This is the most common assertion I see, since it shows up all the time on the http://www.openstreetmap.org/ iD editor.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://tbpl.mozilla.org/?tree=Try&rev=bff969a4d2b9

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1: Remove BindToTree overrides in nsXMLElement that exist only to make AddToIdTable/RemoveFromIdTable calls already in the base class (Element)

I think this could have been done as part of Bug 700981 part 2, which
moved AddToIdTable and RemoveFromIdTable calls from nsStyledElement to
nsGenericElement.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2: Make nsXMLElement reuse Element::AddToIdTable and Element::RemoveFromIdTable thoroughly

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3: Add FIXMEs in nsXULDocument for ID handling

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 4: Make conditions in Element::RemoveFromIdTable match those in AddToIdTable, to avoid asymmetric calls to the document's functions

Comment 16

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

The patch in bug 741295 probably also fixes this one.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Well, it certainly makes patches 1 and 2 irrelevant.

I'm not sure about patch 4, which is actually the part most relevant to the cases for which we have testcases for the assertion firing.  It seems to make that inconsistency worse.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8384428 [details] [diff] [review]
patch 1:  Remove BindToTree overrides in nsXMLElement that exist only to make AddToIdTable/RemoveFromIdTable calls already in the base class (Element)

r=me

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8384429 [details] [diff] [review]
patch 2:  Make nsXMLElement reuse Element::AddToIdTable and Element::RemoveFromIdTable thoroughly

r=me

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8384430 [details] [diff] [review]
patch 3:  Add FIXMEs in nsXULDocument for ID handling

r=me

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8384431 [details] [diff] [review]
patch 4:  Make conditions in Element::RemoveFromIdTable match those in AddToIdTable, to avoid asymmetric calls to the document's functions

r=me

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/57d5722c785b
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bd1a27cf9e6
https://hg.mozilla.org/integration/mozilla-inbound/rev/90990d488dcc
https://hg.mozilla.org/integration/mozilla-inbound/rev/50462316925c

(see also try run at https://tbpl.mozilla.org/?tree=Try&rev=d7e48cf00e0e)

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/57d5722c785b
https://hg.mozilla.org/mozilla-central/rev/5bd1a27cf9e6
https://hg.mozilla.org/mozilla-central/rev/90990d488dcc
https://hg.mozilla.org/mozilla-central/rev/50462316925c