Closed Bug 384663 Opened 17 years ago Closed 17 years ago

Crash [@ nsXBLPrototypeBinding::LocateInstance] with splitter, tree stuff and DOMAttrModified


(Core :: XBL, defect)

Windows XP
Not set





(Reporter: martijn.martijn, Assigned: smaug)


(5 keywords, Whiteboard: [sg:critical])

Crash Data


(3 files)

Attached file testcase
Not really sure in which component this belongs, just guessing.
The testcase crashes current trunk build and branch builds after 200ms, so I'm marking it security sensitive for now.
0  	@0x2578534
1 	nsXBLPrototypeBinding::LocateInstance(nsIContent *,nsIContent *,nsIContent *,nsIContent *)
2 	nsXBLPrototypeBinding::AttributeChanged(nsIAtom *,int,int,nsIContent *,nsIContent *,int)
3 	nsXBLBinding::AttributeChanged(nsIAtom *,int,int,int)
4 	nsGenericElement::SetAttrAndNotify(int,nsIAtom *,nsIAtom *,nsAString_internal const &,nsAttrValue &,int,int,int)
5 	nsGenericElement::SetAttr(int,nsIAtom *,nsIAtom *,nsAString_internal const &,int)
6 	nsGenericElement::SetAttribute(nsAString_internal const &,nsAString_internal const &)
7 	NS_InvokeByIndex_P
8 	AutoJSSuspendRequest::SuspendRequest()
9 	js_CheckRedeclaration
Hmm...  This loop (in nsXBLPrototypeBinding::AttributeChanged) can run arbitrary script via mutation events.  It should at least be holding a strong ref to |content|.  And it should probably put all the relevant nodes from the xblAttr entries into an nsCOMArray or something, because the entries can go away...  Or at least keep the entries from dying somehow.  Perhaps by holding a strong ref to |this|?  Either here or in nsGenericElement::SetAttrAndNotify?
Flags: blocking1.9?
Whiteboard: [sg:critical]
I can take this for now. Will do a patch during next week.
Assignee: nobody → Olli.Pettay
For this crash it is enough to keep the binding alive 
(using nsRefPtr in nsGenericElement::SetAttrAndNotify). That keeps 
also nsXBLAttributeEntry objects alive.
Only the nsRefPtr is needed to fix the crash, but I think
without nsCOMPtrs, there might be some other crashes.
I'll file a new bug to handle raw nsIContent* i
Attachment #268773 - Flags: superreview?(bzbarsky)
Attachment #268773 - Flags: review?(bzbarsky)
(In reply to comment #4)
> I'll file a new bug to handle raw nsIContent* in
> nsXBLAttributeEntry.
Bug 384870
Comment on attachment 268773 [details] [diff] [review]
nsRefPtr and nsCOMPtrs

Looks good.
Attachment #268773 - Flags: superreview?(bzbarsky)
Attachment #268773 - Flags: superreview+
Attachment #268773 - Flags: review?(bzbarsky)
Attachment #268773 - Flags: review+
Closed: 17 years ago
Resolution: --- → FIXED
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre) Gecko/20070626 Minefield/3.0a6pre
crashs on release build - TB33557049Q
Whiteboard: [sg:critical] → [sg:critical] post 1.8-branch
sorry for the bugspam - wrong keyword :) 
Flags: blocking1.8.1.5?
Whiteboard: [sg:critical] post 1.8-branch → [sg:critical]
Flags: blocking1.9?
The 1.8.0 branch is unaffected (tested FF1.5.0.12)
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x-
Keywords: regression
Flags: blocking1.8.1.5? → blocking1.8.1.5+
I can't reproduce the crash on branches atm, but I'll post a 
patch anyway.
Attached patch for 1.8Splinter Review
If someone can reproduce the crash on 1.8, it would be great
to ensure that the patch fixes it.
Attachment #270633 - Flags: approval1.8.1.5?
Attachment #270633 - Flags: approval1.8.0.13?
I crash in, but not in a debug current build. Was the branch problem a different one? Carsten's crash and mine (TB33796811) were both somewhere else altogether:

nsGfxScrollFrameInner::GetIntegerAttribute  [mozilla/layout/generic/nsGfxScrollFrame.cpp, line 2602]
nsGfxScrollFrameInner::SetAttribute  [mozilla/layout/generic/nsGfxScrollFrame.cpp, line 2547]
nsGfxScrollFrameInner::LayoutScrollbars  [mozilla/layout/generic/nsGfxScrollFrame.cpp, line 2437]
nsHTMLScrollFrame::Reflow  [mozilla/layout/generic/nsGfxScrollFrame.cpp, line 818]
nsFrame::BoxReflow  [mozilla/layout/generic/nsFrame.cpp, line 5493]
nsFrame::RefreshSizeCache  [mozilla/layout/generic/nsFrame.cpp, line 4997]
nsFrame::GetAscent  [mozilla/layout/generic/nsFrame.cpp, line 5205]
nsSprocketLayout::GetAscent  [mozilla/layout/xul/base/src/nsSprocketLayout.cpp, line 1591]
nsBoxFrame::GetAscent  [mozilla/layout/xul/base/src/nsBoxFrame.cpp, line 987]
nsSprocketLayout::GetAscent  [mozilla/layout/xul/base/src/nsSprocketLayout.cpp, line 1591]
nsBoxFrame::GetAscent  [mozilla/layout/xul/base/src/nsBoxFrame.cpp, line 987]
nsSprocketLayout::GetAscent  [mozilla/layout/xul/base/src/nsSprocketLayout.cpp, line 1591]
nsBoxFrame::GetAscent  [mozilla/layout/xul/base/src/nsBoxFrame.cpp, line 987]
nsSprocketLayout::GetAscent  [mozilla/layout/xul/base/src/nsSprocketLayout.cpp, line 1591]
nsBoxFrame::GetAscent  [mozilla/layout/xul/base/src/nsBoxFrame.cpp, line 987]
nsSprocketLayout::GetAscent  [mozilla/layout/xul/base/src/nsSprocketLayout.cpp, line 1591]
nsBoxFrame::GetAscent  [mozilla/layout/xul/base/src/nsBoxFrame.cpp, line 987]
nsSprocketLayout::Layout  [mozilla/layout/xul/base/src/nsSprocketLayout.cpp, line 260]
nsBoxFrame::DoLayout  [mozilla/layout/xul/base/src/nsBoxFrame.cpp, line 1106]
nsBoxFrame::DoLayout  [mozilla/layout/xul/base/src/nsBoxFrame.cpp, line 1106]
nsRootBoxFrame::Reflow  [mozilla/layout/xul/base/src/nsRootBoxFrame.cpp, line 227]
nsContainerFrame::ReflowChild  [mozilla/layout/generic/nsContainerFrame.cpp, line 905]
ViewportFrame::Reflow  [mozilla/layout/generic/nsViewportFrame.cpp, line 240]
IncrementalReflow::Dispatch  [mozilla/layout/base/nsPresShell.cpp, line 915]
PresShell::ProcessReflowCommands  [mozilla/layout/base/nsPresShell.cpp, line 6947]
ReflowEvent::HandleEvent  [mozilla/layout/base/nsPresShell.cpp, line 6773]
PL_HandleEvent  [mozilla/xpcom/threads/plevent.c, line 689]
So the 1.8 branch crash was in layout, vs this fix and original crash in content.

qawanted: did the 1.8 branch crash really go away in or is it simply masked when I test in a debug build? If it still happens we probably need a new bug rather than this one.

This patch isn't likely to "fix" a layout crash, but I'll try it. It's not going to hurt anything, either.
Keywords: qawanted
Jay does not crash in a release nightly of

Should I be thankful that the branch crash went away, or worry?

Given that this patch doesn't currently "fix" anything should we land it anyway in case some _other_ testcase can make the binding go away prematurely? Or should we leave things alone? I'm tempted to go with the latter.
Flags: blocking1.8.1.5+ → blocking1.8.1.5?
Keywords: qawanted
I think we should take the patch.
Flags: blocking1.8.1.5? → blocking1.8.1.5+
Comment on attachment 270633 [details] [diff] [review]
for 1.8

approved for and, a=dveditz for release-drivers
Attachment #270633 - Flags: approval1.8.1.5?
Attachment #270633 - Flags: approval1.8.1.5+
Attachment #270633 - Flags: approval1.8.0.13?
Attachment #270633 - Flags: approval1.8.0.13+
retested this bug with the testcase and Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv: Gecko/2007071004 BonEcho/ and this time i did not crash on re-testing like in comment #9, so adding verified keyword
I ran a local server (PWS) on Mac OS X, then pointed Thunderbird's start page to, and chose "Go | Mail Start Page", and didn't have any issues (no crashes).

Verified FIXED on version (20070809) of Thunderbird; replacing fixed1.8.0.13 with verified
Group: security
Flags: in-testsuite?
crash test landed
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsXBLPrototypeBinding::LocateInstance]
You need to log in before you can comment on or make changes to this bug.