Closed Bug 472212 Opened 11 years ago Closed 11 years ago

Crash [@ nsContentUtils::ComparePosition] with binding, part 2

Categories

(Core :: XBL, defect, critical)

x86
Windows XP
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: martijn.martijn, Assigned: smaug)

References

Details

(4 keywords, Whiteboard: [sg:critical?] no crash on 1.9.0, still worth fixing?)

Crash Data

Attachments

(5 files)

Bug 461027 was a case that had a similar testcase.

See upcoming testcase, which crashes current trunk build on load. It doesn't crash in Firefox 3.0, but I'm marking this security sensitive, because bug 461027 is.


Stacktrace from debug build:
>	necko.dll!nsStandardURL::SetHostPort(const nsACString_internal & value={...})  Line 1349 + 0x3 bytes	C++
 	gklayout.dll!nsContentUtils::ComparePosition(nsINode * aNode1=0x04a8c920, nsINode * aNode2=0x049192c0)  Line 1539 + 0xf bytes	C++
 	gklayout.dll!nsContentUtils::PositionIsBefore(nsINode * aNode1=0x04a8c920, nsINode * aNode2=0x049192c0)  Line 283 + 0xd bytes	C++
 	gklayout.dll!nsIdentifierMapEntry::AddIdContent(nsIContent * aContent=0x04a8c920)  Line 457 + 0xd bytes	C++
 	gklayout.dll!nsDocument::UpdateIdTableEntry(nsIContent * aContent=0x04a8c920)  Line 2302	C++
 	gklayout.dll!nsXULDocument::AddElementToDocumentPre(nsIContent * aElement=0x04a8c920)  Line 1674	C++
 	gklayout.dll!nsXULDocument::AddSubtreeToDocument(nsIContent * aElement=0x04a8c920)  Line 1754 + 0x12 bytes	C++
 	gklayout.dll!nsXBLBinding::InstallAnonymousContent(nsIContent * aAnonParent=0x05f88758, nsIContent * aElement=0x0628ef18)  Line 372	C++
 	gklayout.dll!RealizeDefaultContent(nsISupports * aKey=0x0628ef18, nsAutoPtr<nsTArray<nsRefPtr<nsXBLInsertionPoint> > > & aData={...}, void * aClosure=0x0012e278)  Line 548	C++
 	gklayout.dll!nsBaseHashtable<nsISupportsHashKey,nsAutoPtr<nsTArray<nsRefPtr<nsXBLInsertionPoint> > >,nsTArray<nsRefPtr<nsXBLInsertionPoint> > *>::s_EnumStub(PLDHashTable * table=0x00dd86e8, PLDHashEntryHdr * hdr=0x05d2f2ac, unsigned int number=0, void * arg=0x0012e118)  Line 346 + 0x1e bytes	C++
 	xpcom_core.dll!PL_DHashTableEnumerate(PLDHashTable * table=0x00dd86e8, PLDHashOperator (PLDHashTable *, PLDHashEntryHdr *, unsigned int, void *)* etor=0x01d8fac0, void * arg=0x0012e118)  Line 735 + 0x19 bytes	C
 	gklayout.dll!nsBaseHashtable<nsISupportsHashKey,nsAutoPtr<nsTArray<nsRefPtr<nsXBLInsertionPoint> > >,nsTArray<nsRefPtr<nsXBLInsertionPoint> > *>::Enumerate(PLDHashOperator (nsISupports *, nsAutoPtr<nsTArray<nsRefPtr<nsXBLInsertionPoint> > > &, void *)* enumFunc=0x01d8d100, void * userArg=0x0012e278)  Line 221 + 0x13 bytes	C++
 	gklayout.dll!nsXBLBinding::GenerateAnonymousContent()  Line 776	C++
 	gklayout.dll!nsXBLService::LoadBindings(nsIContent * aContent=0x0628ef18, nsIURI * aURL=0x05f85150, nsIPrincipal * aOriginPrincipal=0x04630430, int aAugmentFlag=0, nsXBLBinding * * aBinding=0x0012e5a0, int * aResolveStyle=0x0012e574)  Line 636	C++
 	gklayout.dll!nsCSSFrameConstructor::ConstructFrameInternal(nsFrameConstructorState & aState={...}, nsIContent * aContent=0x0628ef18, nsIFrame * aParentFrame=0x060f995c, nsIAtom * aTag=0x00d4327c, int aNameSpaceID=9, nsStyleContext * aStyleContext=0x0614f158, nsFrameItems & aFrameItems={...}, int aXBLBaseTag=0)  Line 7442 + 0x53 bytes	C++
etc...
Attached file testcase
Assignee: nobody → Olli.Pettay
Attached patch patchSplinter Review
Fixes this bug and doesn't regress Bug 472212 nor Bug 468210.
This bug is about nsXBLInsertionPoint::UnbindDefaultContent.

As far as I see there is no need to unbind mContent nor mDefaultContent
because those are never bound to anything. Just unbind all of their child nodes.
Attachment #355595 - Flags: superreview?(bzbarsky)
Attachment #355595 - Flags: review?(bzbarsky)
Duplicate of this bug: 470491
Attachment #355595 - Flags: superreview?(bzbarsky)
Attachment #355595 - Flags: superreview+
Attachment #355595 - Flags: review?(bzbarsky)
Attachment #355595 - Flags: review+
Comment on attachment 355595 [details] [diff] [review]
patch

Let's please make sure we add all these yummy crashtests!
I landed the patch yesterday, will add tests soon.
Flags: in-testsuite?
Actually, I wouldn't want to commit the testcases before this lands to 1.9.0,
although 1.9.0 doesn't crash.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Attachment #355595 - Flags: approval1.9.1?
Comment on attachment 355595 [details] [diff] [review]
patch

Or at least this should land 1.9.1.
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090108 Minefield/3.2a1pre
Status: RESOLVED → VERIFIED
Comment on attachment 355595 [details] [diff] [review]
patch

a191=beltzner
Attachment #355595 - Flags: approval1.9.1? → approval1.9.1+
Keywords: fixed1.9.1
Duplicate of this bug: 468511
Target Milestone: --- → mozilla1.9.2a1
verified FIXED using the attached testcase on builds:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090721
Minefield/3.6a1pre ID:20090721044139

and

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.1pre) Gecko/20090720
Shiretoko/3.5.1pre ID:20090720042942
(In reply to comment #7)
> Actually, I wouldn't want to commit the testcases before this lands to 1.9.0,
> although 1.9.0 doesn't crash.

Olli: do you still think we need this fix on the 1.9.0 branch?
Flags: wanted1.9.0.x?
Flags: blocking1.9.0.13?
Whiteboard: [sg:critical?]
Flags: blocking1.9.0.14?
Whiteboard: [sg:critical?] → [needs answer to comment 14 from Olli][sg:critical?]
Flags: blocking1.9.0.15?
Well, at least the testcase doesn't crash on 1.9.0. (Tried using a local copy of the test.)
This is probably a regression from bug 344258. Or that bug revealed the problem.
But since Bug 461027 was taken to 1.9.0, this should be taken too, I think.

The patch does apply to 1.9.0. I'm posting to it to tryserver.
Since the testcase doesn't actually crash 1.9.0 it's OK to check it in now. We'll block to get this fix into 1.9.0.15 too.
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.15?
Flags: blocking1.9.0.15+
Attached patch 190Splinter Review
Attachment #396428 - Flags: approval1.9.0.15?
Comment on attachment 396428 [details] [diff] [review]
190

Approved for 1.9.0.15, a=dveditz for release-drivers
Attachment #396428 - Flags: approval1.9.0.15? → approval1.9.0.15+
Checking in content/xbl/src/nsXBLBinding.cpp;
/cvsroot/mozilla/content/xbl/src/nsXBLBinding.cpp,v  <--  nsXBLBinding.cpp
new revision: 1.267; previous revision: 1.266
done
Checking in content/xbl/src/nsXBLBinding.h;
/cvsroot/mozilla/content/xbl/src/nsXBLBinding.h,v  <--  nsXBLBinding.h
new revision: 1.60; previous revision: 1.59
done
Checking in content/xbl/src/nsXBLInsertionPoint.cpp;
/cvsroot/mozilla/content/xbl/src/nsXBLInsertionPoint.cpp,v  <--  nsXBLInsertionPoint.cpp
new revision: 1.22; previous revision: 1.21
done
Keywords: fixed1.9.0.15
Backed out because of
###!!! ASSERTION: attempt to remove an element that was never added: 'hep != nsnull && *hep != nsnull', file /builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-53.1.21.el5_Depend/mozilla/content/xul/document/src/nsElementMap.cpp, line 281


Checking in content/xbl/src/nsXBLBinding.cpp;
/cvsroot/mozilla/content/xbl/src/nsXBLBinding.cpp,v  <--  nsXBLBinding.cpp
new revision: 1.268; previous revision: 1.267
done
Checking in content/xbl/src/nsXBLBinding.h;
/cvsroot/mozilla/content/xbl/src/nsXBLBinding.h,v  <--  nsXBLBinding.h
new revision: 1.61; previous revision: 1.60
done
Checking in content/xbl/src/nsXBLInsertionPoint.cpp;
/cvsroot/mozilla/content/xbl/src/nsXBLInsertionPoint.cpp,v  <--  nsXBLInsertionPoint.cpp
new revision: 1.23; previous revision: 1.22
done

Is this not needed in 1.9.0 after all. I need to re-check that.
Keywords: fixed1.9.0.15
The assertion is actually harmless in this case, but waterson added there
quite strong comment http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/xul/document/src/nsElementMap.cpp&rev=1.29&mark=278-281#249
With the patch we may do some extra work to ensure that all the elements
are removed from ID map(s).


Boris, do you think removing the assertion would be ok?
Note, on 1.9.1/1.9.2/trunk we don't have such assertion because ID handling
works in a bit different way.
Actually, I could check in the UninstallAnonymousContent if child's parent
is aAnonParent, then there shouldn't be need to call RemoveSubtreeFromDocument.
(In reply to comment #24)
> Actually, I could check in the UninstallAnonymousContent if child's parent
> is aAnonParent, then there shouldn't be need to call RemoveSubtreeFromDocument.
That approach didn't work.

Bug 461027 and this bugs shouldn't crash on 1.9.0 since nsElementMap has strong
reference to the nsIContent object.
Olli: Making sure I understand, we shouldn't block on this bug at all?
Right. I don't think we should block on this.
> Boris, do you think removing the assertion would be ok?

Do you still want me to try to figure out an answer to this question?
(In reply to comment #27)
> Right. I don't think we should block on this.

Removing the 1.9.0.15 blocking flag. Should we also remove the 1.9.0 "wanted" flag? comment 25 makes it sound like this bug shouldn't be a problem in 1.9.0
Flags: blocking1.9.0.15+ → blocking1.9.0.16?
Whiteboard: [needs answer to comment 14 from Olli][sg:critical?] → [sg:critical?] no crash on 1.9.0, still worth fixing?
Comment on attachment 396428 [details] [diff] [review]
190

backed out due to assertions, removing 1.9.0.15 approval from this patch.
Attachment #396428 - Flags: approval1.9.0.15+ → approval1.9.0.15-
Flags: wanted1.9.0.x-
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.16?
Group: core-security
Crash Signature: [@ nsContentUtils::ComparePosition]
Flags: in-testsuite? → in-testsuite-
You need to log in before you can comment on or make changes to this bug.