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

VERIFIED FIXED

Status

()

defect
--
critical
VERIFIED FIXED
12 years ago
8 years ago

People

(Reporter: martijn.martijn, Assigned: smaug)

Tracking

(5 keywords)

Trunk
x86
Windows XP
Points:
---
Bug Flags:
blocking1.8.1.5 +
wanted1.8.1.x +
wanted1.8.0.x -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical], crash signature)

Attachments

(3 attachments)

Reporter

Description

12 years ago
Posted 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?

Updated

12 years ago
Whiteboard: [sg:critical]
Assignee

Comment 2

12 years ago
I can take this for now. Will do a patch during next week.
Assignee: nobody → Olli.Pettay
Assignee

Comment 3

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

Comment 4

12 years ago
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)
Assignee

Comment 5

12 years ago
(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+
Assignee

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Reporter

Comment 7

12 years ago
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+
Assignee

Comment 11

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

Comment 12

12 years ago
Posted 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.
Assignee

Updated

12 years ago
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
Assignee

Comment 16

12 years ago
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+
Assignee

Updated

12 years ago
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?

Comment 20

10 years ago
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.