680922 - Crash [@ nsIdentifierMapEntry::RemoveNameElement] on reload with form in binding and name, id, observes

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

[Martijn Wargers (dead)]

See zipped up testcase.
You need to set the dom.allowXULXBL_for_file or dom.allow_XUL_XBL_for_file pref to true in about:config to see the crash.
Open the file named 'crash1.xul' to see the crash.

https://crash-stats.mozilla.com/report/index/bp-bdc3dfaf-2308-4be6-bb46-310672110822
0 	xul.dll 	nsVoidArray::IndexOf 	obj-firefox/xpcom/build/nsVoidArray.cpp:427
1 	xul.dll 	nsCOMArray_base::RemoveObject 	obj-firefox/xpcom/build/nsCOMArray.cpp:123
2 	xul.dll 	nsIdentifierMapEntry::RemoveNameElement 	content/base/src/nsDocument.cpp:446
3 	xul.dll 	nsDocument::RemoveFromNameTable 	content/base/src/nsDocument.cpp:2592
4 	xul.dll 	nsGenericHTMLElement::UnbindFromTree 	
5 	xul.dll 	nsHTMLFormElement::UnbindFromTree 	content/html/content/src/nsHTMLFormElement.cpp:548
6 	xul.dll 	nsXBLBinding::UninstallAnonymousContent 	content/xbl/src/nsXBLBinding.cpp:393
7 	xul.dll 	nsXBLBinding::ChangeDocument 	content/xbl/src/nsXBLBinding.cpp:1175
8 	xul.dll 	nsBindingManager::RemovedFromDocumentInternal 	
9 	xul.dll 	nsBindingManager::RemovedFromDocument 	obj-firefox/dist/include/nsBindingManager.h:98
10 	xul.dll 	nsGenericElement::DestroyContent 	content/base/src/nsGenericElement.cpp:3654
11 	xul.dll 	nsGenericElement::DestroyContent 	content/base/src/nsGenericElement.cpp:3665
12 	xul.dll 	nsGenericElement::DestroyContent 	content/base/src/nsGenericElement.cpp:3665
13 	xul.dll 	nsGenericElement::DestroyContent 	content/base/src/nsGenericElement.cpp:3665
14 	xul.dll 	nsDocument::Destroy 	content/base/src/nsDocument.cpp:7040
15 	xul.dll 	DocumentViewerImpl::Destroy 	layout/base/nsDocumentViewer.cpp:1652
16 	xul.dll 	DocumentViewerImpl::Show 	layout/base/nsDocumentViewer.cpp:1961
17 	xul.dll 	nsPresContext::EnsureVisible 	layout/base/nsPresContext.cpp:1753
18 	xul.dll 	PresShell::UnsuppressAndInvalidate 	layout/base/nsPresShell.cpp:4504
19 	xul.dll 	PresShell::UnsuppressPainting 	layout/base/nsPresShell.cpp:4553
20 	xul.dll 	DocumentViewerImpl::LoadComplete 	layout/base/nsDocumentViewer.cpp:1096
21 	xul.dll 	nsDocShell::EndPageLoad 	docshell/base/nsDocShell.cpp:6154
etc..

Comment 1

[Martijn Wargers (dead)]

Attachment: zipped up testcase

Comment 2

[Martijn Wargers (dead)]

This regressed between 2011-04-22 and 2011-04-23:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2011-04-22+02%3A00%3A00&enddate=2011-04-23+08%3A00%3A00
Regression from bug 588990 is my guess.

Comment 3

[Boris Zbarsky [:bzbarsky]]

We're crashing because in nsIdentifierMapEntry::RemoveNameElement mNameContentList is null.

We get to RemoveNameElement via nsHTMLFormElement::UnbindFromTree calling nsGenericHTMLElement::RemoveFromNameTable.

And in particular, nsGenericHTMLElement::AddToNameTable checks IsInAnonymousSubtree, but nsGenericHTMLElement::RemoveFromNameTable does not.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Don't try to remove anonymous elements from the name map.

Comment 5

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

Comment on attachment 555493 [details] [diff] [review]
Don't try to remove anonymous elements from the name map.

Review of attachment 555493 [details] [diff] [review]:
-----------------------------------------------------------------

r=me either way

::: content/html/content/crashtests/680922-binding.xml
@@ +3,5 @@
> +<content>
> +<html:form id="g" observes="b"/>
> +</content>
> +</binding>
> +</bindings>
\ No newline at end of file

Add newline to the end of this file.

::: content/html/content/src/nsGenericHTMLElement.h
@@ +553,5 @@
>    }
>    void RemoveFromNameTable() {
>      if (HasName()) {
>        nsIDocument* doc = GetCurrentDoc();
> +      if (doc && !IsInAnonymousSubtree()) {

Hmm.. I *think* that my idea was that it should be ok to remove an element that wasn't in the name map. I guess I'm fine with this solution too, but I wonder if it wouldn't be faster and safer to add a nullcheck.

Comment 6

[Boris Zbarsky [:bzbarsky]]

It _might_ be safer; in the common case of no anonymous content shenanigans it's probably a tad faster.  I can do the null-check instead; I assume your review applies to that?

Comment 7

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

Yup

Comment 8

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1cb463c1c7b7 with the null-check and the added newline.

Comment 9

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/1cb463c1c7b7