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

Status: VERIFIED FIXED

(Core :: XBL, defect)

Description

[Martijn Wargers (dead)]

Attachment: binding needed for testcase

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...

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Martijn Wargers (dead)]

Attachment: Complete stack of debug build

Comment 3

[Olli Pettay [:smaug]]

Attachment: patch

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Let's please make sure we add all these yummy crashtests!

Comment 6

[Olli Pettay [:smaug]]

I landed the patch yesterday, will add tests soon.

Comment 7

[Olli Pettay [:smaug]]

Actually, I wouldn't want to commit the testcases before this lands to 1.9.0,
although 1.9.0 doesn't crash.

Comment 8

[Olli Pettay [:smaug]]

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

Or at least this should land 1.9.1.

Comment 9

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090108 Minefield/3.2a1pre

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

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

a191=beltzner

Comment 11

[Olli Pettay [:smaug]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f2c5086fceb2

Comment 13

[Aakash Desai [:aakashd]]

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

Comment 14

[Daniel Veditz [:dveditz]]

(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?

Comment 15

[Samuel Sidler (old account; do not CC)]

Olli: Ping on comment 14?

Comment 16

[Olli Pettay [:smaug]]

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.

Comment 17

[Daniel Veditz [:dveditz]]

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.

Comment 18

[Olli Pettay [:smaug]]

Attachment: 190

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 396428 [details] [diff] [review]
190

Approved for 1.9.0.15, a=dveditz for release-drivers

Comment 20

[Olli Pettay [:smaug]]

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

Comment 21

[Olli Pettay [:smaug]]

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.

Comment 22

[Olli Pettay [:smaug]]

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?

Comment 23

[Olli Pettay [:smaug]]

Note, on 1.9.1/1.9.2/trunk we don't have such assertion because ID handling
works in a bit different way.

Comment 24

[Olli Pettay [:smaug]]

Actually, I could check in the UninstallAnonymousContent if child's parent
is aAnonParent, then there shouldn't be need to call RemoveSubtreeFromDocument.

Comment 25

[Olli Pettay [:smaug]]

(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.

Comment 26

[Samuel Sidler (old account; do not CC)]

Olli: Making sure I understand, we shouldn't block on this bug at all?

Comment 27

[Olli Pettay [:smaug]]

Right. I don't think we should block on this.

Comment 28

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 29

[Daniel Veditz [:dveditz]]

(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

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 396428 [details] [diff] [review]
190

backed out due to assertions, removing 1.9.0.15 approval from this patch.