Last Comment Bug 384663 - Crash [@ nsXBLPrototypeBinding::LocateInstance] with splitter, tree stuff and DOMAttrModified
: Crash [@ nsXBLPrototypeBinding::LocateInstance] with splitter, tree stuff and...
Status: VERIFIED FIXED
[sg:critical]
: crash, regression, testcase, verified1.8.0.13, verified1.8.1.5
Product: Core
Classification: Components
Component: XBL (show other bugs)
: Trunk
: x86 Windows XP
: -- critical (vote)
: ---
Assigned To: Olli Pettay [:smaug] (TPAC)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-06-15 20:00 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2011-06-13 10:01 PDT (History)
9 users (show)
dveditz: blocking1.8.1.5+
dveditz: wanted1.8.1.x+
dveditz: wanted1.8.0.x-
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (586 bytes, application/vnd.mozilla.xul+xml)
2007-06-15 20:00 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
nsRefPtr and nsCOMPtrs (2.53 KB, patch)
2007-06-18 03:40 PDT, Olli Pettay [:smaug] (TPAC)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
for 1.8 (3.33 KB, patch)
2007-07-02 15:37 PDT, Olli Pettay [:smaug] (TPAC)
dveditz: approval1.8.1.5+
dveditz: approval1.8.0.13+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2007-06-15 20:00:41 PDT
Created attachment 268571 [details]
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 [:bz] (TPAC) 2007-06-15 22:28:27 PDT
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] (TPAC) 2007-06-16 00:32:30 PDT
I can take this for now. Will do a patch during next week.
Comment 3 Olli Pettay [:smaug] (TPAC) 2007-06-18 03:31:18 PDT
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] (TPAC) 2007-06-18 03:40:14 PDT
Created attachment 268773 [details] [diff] [review]
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] (TPAC) 2007-06-18 03:46:30 PDT
(In reply to comment #4)
> I'll file a new bug to handle raw nsIContent* in
> nsXBLAttributeEntry.
> 
Bug 384870
Comment 6 Boris Zbarsky [:bz] (TPAC) 2007-06-18 07:38:09 PDT
Comment on attachment 268773 [details] [diff] [review]
nsRefPtr and nsCOMPtrs

Looks good.
Comment 7 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-06-27 22:58:48 PDT
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] 2007-06-28 13:26:24 PDT
crashs on 2.0.0.4 release build - TB33557049Q
Comment 9 Carsten Book [:Tomcat] 2007-06-28 13:32:20 PDT
sorry for the bugspam - wrong keyword :) 
Comment 10 Daniel Veditz [:dveditz] 2007-06-28 15:12:06 PDT
The 1.8.0 branch is unaffected (tested FF1.5.0.12)
Comment 11 Olli Pettay [:smaug] (TPAC) 2007-07-02 15:30:34 PDT
I can't reproduce the crash on branches atm, but I'll post a 
patch anyway.
Comment 12 Olli Pettay [:smaug] (TPAC) 2007-07-02 15:37:52 PDT
Created attachment 270633 [details] [diff] [review]
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] 2007-07-06 14:34:04 PDT
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] 2007-07-06 14:39:59 PDT
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] 2007-07-06 14:46:55 PDT
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] (TPAC) 2007-07-07 04:36:06 PDT
I think we should take the patch.
Comment 17 Daniel Veditz [:dveditz] 2007-07-09 10:32:05 PDT
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] 2007-07-10 18:09:16 PDT
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] 2007-08-20 14:35:56 PDT
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:] 2009-04-24 10:42:22 PDT
crash test landed
http://hg.mozilla.org/mozilla-central/rev/e799259f7192

Note You need to log in before you can comment on or make changes to this bug.