Closed
Bug 472212
Opened 14 years ago
Closed 14 years ago
Crash [@ nsContentUtils::ComparePosition] with binding, part 2
Categories
(Core :: XBL, defect)
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)
341 bytes,
text/xml
|
Details | |
505 bytes,
application/vnd.mozilla.xul+xml
|
Details | |
9.14 KB,
text/plain
|
Details | |
9.59 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
12.01 KB,
patch
|
dveditz
:
approval1.9.0.15-
|
Details | Diff | Splinter Review |
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...
Reporter | ||
Comment 1•14 years ago
|
||
Reporter | ||
Comment 2•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → Olli.Pettay
Assignee | ||
Comment 3•14 years ago
|
||
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)
![]() |
||
Updated•14 years ago
|
Attachment #355595 -
Flags: superreview?(bzbarsky)
Attachment #355595 -
Flags: superreview+
Attachment #355595 -
Flags: review?(bzbarsky)
Attachment #355595 -
Flags: review+
![]() |
||
Comment 5•14 years ago
|
||
Comment on attachment 355595 [details] [diff] [review] patch Let's please make sure we add all these yummy crashtests!
Assignee | ||
Comment 6•14 years ago
|
||
I landed the patch yesterday, will add tests soon.
Flags: in-testsuite?
Assignee | ||
Comment 7•14 years ago
|
||
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: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•14 years ago
|
Attachment #355595 -
Flags: approval1.9.1?
Assignee | ||
Comment 8•14 years ago
|
||
Comment on attachment 355595 [details] [diff] [review] patch Or at least this should land 1.9.1.
Reporter | ||
Comment 9•14 years ago
|
||
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 10•14 years ago
|
||
Comment on attachment 355595 [details] [diff] [review] patch a191=beltzner
Attachment #355595 -
Flags: approval1.9.1? → approval1.9.1+
Assignee | ||
Updated•14 years ago
|
Keywords: fixed1.9.1
Assignee | ||
Comment 11•14 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f2c5086fceb2
Updated•14 years ago
|
Target Milestone: --- → mozilla1.9.2a1
Comment 13•13 years ago
|
||
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
Keywords: fixed1.9.1 → verified1.9.1
Comment 14•13 years ago
|
||
(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?]
Updated•13 years ago
|
Flags: blocking1.9.0.14?
Whiteboard: [sg:critical?] → [needs answer to comment 14 from Olli][sg:critical?]
Updated•13 years ago
|
Flags: blocking1.9.0.15?
Comment 15•13 years ago
|
||
Olli: Ping on comment 14?
Assignee | ||
Comment 16•13 years ago
|
||
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•13 years ago
|
||
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+
Assignee | ||
Comment 18•13 years ago
|
||
Attachment #396428 -
Flags: approval1.9.0.15?
Comment 19•13 years ago
|
||
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+
Assignee | ||
Comment 20•13 years ago
|
||
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
Assignee | ||
Comment 21•13 years ago
|
||
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
Assignee | ||
Comment 22•13 years ago
|
||
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?
Assignee | ||
Comment 23•13 years ago
|
||
Note, on 1.9.1/1.9.2/trunk we don't have such assertion because ID handling works in a bit different way.
Assignee | ||
Comment 24•13 years ago
|
||
Actually, I could check in the UninstallAnonymousContent if child's parent is aAnonParent, then there shouldn't be need to call RemoveSubtreeFromDocument.
Assignee | ||
Comment 25•13 years ago
|
||
(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•13 years ago
|
||
Olli: Making sure I understand, we shouldn't block on this bug at all?
Assignee | ||
Comment 27•13 years ago
|
||
Right. I don't think we should block on this.
![]() |
||
Comment 28•13 years ago
|
||
> 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•13 years ago
|
||
(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 30•13 years ago
|
||
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-
Updated•13 years ago
|
Flags: wanted1.9.0.x-
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.16?
Updated•12 years ago
|
Group: core-security
Updated•11 years ago
|
Crash Signature: [@ nsContentUtils::ComparePosition]
Assignee | ||
Updated•10 years ago
|
Flags: in-testsuite? → in-testsuite-
You need to log in
before you can comment on or make changes to this bug.
Description
•