Last Comment Bug 680922 - Crash [@ nsIdentifierMapEntry::RemoveNameElement] on reload with form in binding and name, id, observes
: Crash [@ nsIdentifierMapEntry::RemoveNameElement] on reload with form in bind...
Status: RESOLVED FIXED
: crash, testcase
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: x86 Windows 7
: P1 critical (vote)
: mozilla9
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
:
Mentors:
Depends on:
Blocks: 588990
  Show dependency treegraph
 
Reported: 2011-08-22 08:59 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2011-09-20 07:49 PDT (History)
4 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
zipped up testcase (555 bytes, application/octet-stream)
2011-08-22 09:01 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Don't try to remove anonymous elements from the name map. (2.40 KB, patch)
2011-08-24 12:35 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
jonas: review+
Details | Diff | Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2011-08-22 08:59:28 PDT
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 [:mwargers] (not working for Mozilla) 2011-08-22 09:01:27 PDT
Created attachment 554876 [details]
zipped up testcase
Comment 2 Martijn Wargers [:mwargers] (not working for Mozilla) 2011-08-24 05:53:30 PDT
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 [:bz] (Out June 25-July 6) 2011-08-24 12:18:15 PDT
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 [:bz] (Out June 25-July 6) 2011-08-24 12:35:35 PDT
Created attachment 555493 [details] [diff] [review]
Don't try to remove anonymous elements from the name map.
Comment 5 Jonas Sicking (:sicking) PTO Until July 5th 2011-09-19 15:19:49 PDT
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 [:bz] (Out June 25-July 6) 2011-09-19 19:11:54 PDT
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) PTO Until July 5th 2011-09-19 19:13:14 PDT
Yup
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-09-19 21:34:23 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cb463c1c7b7 with the null-check and the added newline.
Comment 9 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-20 07:49:34 PDT
https://hg.mozilla.org/mozilla-central/rev/1cb463c1c7b7

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