Closed Bug 384663 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: XBL, defect)

x86
Windows XP
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: martijn.martijn, Assigned: smaug)

Details

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

Crash Data

Attachments

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

https://crash-reports.mozilla.com/reports/report/index/71f69073-1bb2-11dc-8217-001a4bd43ed6
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
nsXBLAttributeEntry.
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+
Status: NEW → RESOLVED
Closed: 13 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
Status: RESOLVED → VERIFIED
crashs on 2.0.0.4 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 2.0.0.4, but not in a debug current 1.8.1.5pre 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]
0x778b0c24
0x0020006c
0x880ff445
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 2.0.0.5 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 2.0.0.5pre

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 1.8.1.5 and 1.8.0.13, 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:1.8.1.5pre) Gecko/2007071004 BonEcho/2.0.0.5pre 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 1.5.0.13 start page to http://h-164.office.mozilla.org/~stephend/_crash9.xul, and chose "Go | Mail Start Page", and didn't have any issues (no crashes).

Verified FIXED on version 1.5.0.13 (20070809) of Thunderbird; replacing fixed1.8.0.13 with verified 1.8.0.13.
Group: security
Flags: in-testsuite?
crash test landed
http://hg.mozilla.org/mozilla-central/rev/e799259f7192
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.