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

Status: VERIFIED FIXED

(Core :: XBL, defect)

Description

[Martijn Wargers (dead)]

Attachment: 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

Comment 1

[Boris Zbarsky [:bzbarsky]]

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?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

I can take this for now. Will do a patch during next week.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

For this crash it is enough to keep the binding alive 
(using nsRefPtr in nsGenericElement::SetAttrAndNotify). That keeps 
also nsXBLAttributeEntry objects alive.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: nsRefPtr and nsCOMPtrs

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.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #4)
> I'll file a new bug to handle raw nsIContent* in
> nsXBLAttributeEntry.
> 
Bug 384870

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 268773 [details] [diff] [review]
nsRefPtr and nsCOMPtrs

Looks good.

Comment 7

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre) Gecko/20070626 Minefield/3.0a6pre

Comment 8

[Carsten Book [:Tomcat]]

crashs on 2.0.0.4 release build - TB33557049Q

Comment 9

[Carsten Book [:Tomcat]]

sorry for the bugspam - wrong keyword :) 

Comment 10

[Daniel Veditz [:dveditz]]

The 1.8.0 branch is unaffected (tested FF1.5.0.12)

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

I can't reproduce the crash on branches atm, but I'll post a 
patch anyway.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for 1.8

If someone can reproduce the crash on 1.8, it would be great
to ensure that the patch fixes it.

Comment 13

[Daniel Veditz [:dveditz]]

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

Comment 14

[Daniel Veditz [:dveditz]]

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.

Comment 15

[Daniel Veditz [:dveditz]]

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.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

I think we should take the patch.

Comment 17

[Daniel Veditz [:dveditz]]

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

Comment 18

[Carsten Book [:Tomcat]]

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

Comment 19

[Stephen Donner [:stephend] Not actively reading bugmail]

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.

Comment 20

[Bob Clary [:bc] (inactive)]

crash test landed
http://hg.mozilla.org/mozilla-central/rev/e799259f7192