potential crash in nsCSSFrameConstructor::ContentInserted(), releasing null bodyBoxObject

NEW
Assigned to

Status

()

Core
Layout
P3
normal
16 years ago
9 years ago

People

(Reporter: (not reading, please use seth@sspitzer.org instead), Assigned: timeless)

Tracking

Trunk
x86
Windows 2000
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [needs patch that addresses review comments - timeless])

Attachments

(2 attachments, 2 obsolete attachments)

When you change the pref for forwarding from inline to attachment (Edit |
Preferences | Mail & Newsgroups | Composition | Forward Message), and then
immediately forward a message, we crash.

may not be reproducable everywhere.  but we've seen this on a branch based off 
1.0.2

we're doing NS_RELEASE(bodyBoxObject);

but it's null.

a stack trace from my debug build:

nsCSSFrameConstructor::ContentInserted(nsCSSFrameConstructor * const 0x03f880b0,
nsIPresContext * 0x03b2fa68, nsIContent * 0x0507f080, nsIContent * 0x0509a4c0,
int 1, nsILayoutHistoryState * 0x00000000, int 0) line 8828 + 6 bytes
StyleSetImpl::ContentInserted(StyleSetImpl * const 0x04087eb0, nsIPresContext *
0x03b2fa68, nsIContent * 0x0507f080, nsIContent * 0x0509a4c0, int 1) line 1537
PresShell::ContentInserted(PresShell * const 0x03f881e0, nsIDocument *
0x03a6cb18, nsIContent * 0x0507f080, nsIContent * 0x0509a4c0, int 1) line 5259 +
53 bytes
nsXBLResourceLoader::NotifyBoundElements() line 279
nsXBLResourceLoader::StyleSheetLoaded(nsXBLResourceLoader * const 0x04f667c8,
nsICSSStyleSheet * 0x03f4f5b8, int 0) line 209
CSSLoaderImpl::InsertSheetInDoc(nsICSSStyleSheet * 0x03f4f5b8, int 2, nsIContent
* 0x00000000, int 0, nsICSSLoaderObserver * 0x04f667c8) line 1196
InsertPendingSheet(void * 0x050f1df0, void * 0x03e56c70) line 757
nsVoidArray::EnumerateForwards(int (void *, void *)* 0x027b6320
InsertPendingSheet(void *, void *), void * 0x03e56c70) line 660 + 21 bytes
CSSLoaderImpl::Cleanup(URLKey & {...}, SheetLoadData * 0x05060c70) line 821
CSSLoaderImpl::SheetComplete(nsICSSStyleSheet * 0x00000000, SheetLoadData *
0x05060c70) line 914
CSSLoaderImpl::ParseSheet(nsIUnicharInputStream * 0x05004990, SheetLoadData *
0x05060c70, int & 1, nsICSSStyleSheet * & 0x03f4f5b8) line 949
CSSLoaderImpl::DidLoadStyle(nsIStreamLoader * 0x041f5350, nsString * 0x04f18468,
SheetLoadData * 0x05060c70, unsigned int 0) line 984 + 27 bytes
SheetLoadData::OnStreamComplete(SheetLoadData * const 0x05060c70,
nsIStreamLoader * 0x041f5350, nsISupports * 0x00000000, unsigned int 0, unsigned
int 3799, const char * 0x04f55088) line 741
nsStreamLoader::OnStopRequest(nsStreamLoader * const 0x041f5354, nsIRequest *
0x04f84740, nsISupports * 0x00000000, unsigned int 0) line 163
nsJARChannel::OnStopRequest(nsJARChannel * const 0x04f84744, nsIRequest *
0x04f848fc, nsISupports * 0x00000000, unsigned int 0) line 606 + 49 bytes
nsOnStopRequestEvent::HandleEvent() line 213
nsARequestObserverEvent::HandlePLEvent(PLEvent * 0x04f96e9c) line 116
PL_HandleEvent(PLEvent * 0x04f96e9c) line 643 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x013f4fa0) line 573 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x000f0124, unsigned int 49430, unsigned int 0,
long 20926368) line 1308 + 9 bytes
USER32! 77e11b60()
USER32! 77e11cca()
USER32! 77e183f1()
nsAppShellService::Run(nsAppShellService * const 0x01440028) line 458
main1(int 1, char * * 0x00336ff0, nsISupports * 0x00000000, const nsXREAppData &
{...}) line 1521 + 32 bytes
xre_main(int 1, char * * 0x00336ff0, const nsXREAppData & {...}) line 1885 + 41
bytes
main(int 1, char * * 0x00336ff0) line 51 + 17 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e8d326()

Updated

16 years ago
Summary: potential crash in nsCSSFrameConstructor::ContentInserted(), releasing null bodyBoxObject → potential crash in nsCSSFrameConstructor::ContentInserted(), releasing null bodyBoxObject
jpatel might recognize this stack, or know if this is a top crashser.

Comment 3

16 years ago
Not being familiar with XUL list boxes, one of the the questions that pops
immediately to mind is if the pointer you are checking should ever really be
null, or is something unexpectedly freeing it out from underneath us? I tend to
not wallpaper over problems like this until I can answer that question.

I can help you take a look if you had a testcase of some sort.

Comment 4

16 years ago
Created attachment 103125 [details]
Breakdown of crash frequency of this signature in TB data.

This signature is currently bug #138 on the Trunk list. Not in the top40. (see
also bug 77716)
I'll meet with kin so we can debug this on my build at work.

taking it from marc, until someone in layout takes it from me.
Assignee: attinasi → sspitzer

Updated

16 years ago
QA Contact: petersen → moied

Updated

16 years ago
Priority: -- → P3
met with kin.  the code (hewitt's originally, adding listbox support to xul) 
does a null check, but doesn't null check the same address before calling 
release.

kin suggests (and after looking at cvsblame, and debugging, I agree) we change:

         NS_RELEASE(bodyBoxObject);
         if (listBoxBody)
           listBoxBody->OnContentInserted(aPresContext, aChild);

to:
         if (listBoxBody) {
           NS_RELEASE(bodyBoxObject);
           listBoxBody->OnContentInserted(aPresContext, aChild);
         }

and 
           listBoxBody = NS_STATIC_CAST(nsListBoxBodyFrame*, bodyBoxObject);
           NS_RELEASE(bodyBoxObject);
to:
           listBoxBody = NS_STATIC_CAST(nsListBoxBodyFrame*, bodyBoxObject);
           NS_IF_RELEASE(bodyBoxObject);

I'll create a new patch, and drive it in, unless a layout person on the cc list 
can take it from me.  
Timeless, would you be able to take care of this?
(Assignee)

Comment 8

16 years ago
Created attachment 104496 [details] [diff] [review]
stop playing refcounting games with things which don't refcount
Attachment #103099 - Attachment is obsolete: true
(Assignee)

Comment 9

16 years ago
.
Assignee: sspitzer → timeless
Keywords: mozilla1.2
Target Milestone: --- → mozilla1.2final
Comment on attachment 104496 [details] [diff] [review]
stop playing refcounting games with things which don't refcount

Have you verified that nsListBoxObject is unused?  That's the way it looks to
me from LXR, bit it should probably be removed from nsLayoutModule.cpp and the
build if that's the case, and if it's unneeded.

Also, if nsIListBoxObject is always for un-refcounted objects, there should be
a comment in nsIListBoxObject.idl and it should not be marked as scriptable,
which it currently is.
+    supp->QueryInterface(NS_GET_IID(nsIListBoxObject), (void**)&body);

CallQueryInterface, dude.
(Assignee)

Comment 12

16 years ago
Created attachment 104532 [details] [diff] [review]
remove nsListBoxObject to clarify that the object isn't refcounted. + don't play refcounting games

http://lxr.mozilla.org/seamonkey/search?string=nsListBoxObject
(build, definition, implementation, prototype, and) one available constructor:
http://lxr.mozilla.org/seamonkey/ident?i=NS_NewListBoxObject
(module, definition/prototype and) chains to
http://lxr.mozilla.org/seamonkey/search?string=CreateNewListBoxObject
chains to
http://lxr.mozilla.org/seamonkey/search?string=xul-boxobject-listbox;
so it's not constructed

so yes, no one in seamonkey, mozilla, mozilla1.0 or mozdev
(http://mozdev.org/lxr/search?string=boxobject) uses it (i checked each).

ok, so if we remove nsListBoxObject, then there's only one nsIListBoxObject and
it is *not* refcounted, but it is consumed by a script [the following will not
be news to the reporter because it's the how/why the reporter triggered the
problem :)], first as xbl:
/xpfe/global/resources/content/bindings/listbox.xml, line 63 -- onget="return
this.boxObject.QueryInterface(Components.interfaces.nsIListBoxObject);"
(boxObject is a <something>::GetBoxObject)

and then in js/xul
/mailnews/compose/resources/content/addressingWidgetOverlay.js, line 859 --
function awRecipientKeyPress(event, element)
/mailnews/compose/resources/content/addressingWidgetOverlay.xul, line 67 --
onkeypress="awRecipientKeyPress(event, this)"

/mailnews/compose/resources/content/addressingWidgetOverlay.js, line 717 --
function awTabFromRecipient(element, event)
/mailnews/compose/resources/content/addressingWidgetOverlay.js, line 863 --
awTabFromRecipient(element, event);

/mailnews/compose/resources/content/addressingWidgetOverlay.js, line 727 --
listBox.listBoxObject.ensureIndexIsVisible(listBoxRow + 1);
/mailnews/compose/resources/content/addressingWidgetOverlay.js, line 737 --
listBox.listBoxObject.ensureIndexIsVisible(listBoxRow - 1);
Attachment #104496 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #104532 - Flags: superreview?(bzbarsky)
Attachment #104532 - Flags: review?(dbaron)
Comment on attachment 104532 [details] [diff] [review]
remove nsListBoxObject to clarify that the object isn't refcounted. + don't play refcounting games

So how exactly are we having a scriptable non-refcounted object?  That does not
work.
Comment on attachment 104532 [details] [diff] [review]
remove nsListBoxObject to clarify that the object isn't refcounted. + don't play refcounting games

Oy.  That stuff was already there; you didn't really change it...

Make the comment in that idl an XXX comment and file a bug on that misbegotten
state of affairs, and sr=me...
Attachment #104532 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 104532 [details] [diff] [review]
remove nsListBoxObject to clarify that the object isn't refcounted. + don't play refcounting games

What about nsXULDocument::GetBoxObjectFor?  (Perhaps you should also discuss
this with a XUL module owner?)
Comment on attachment 104532 [details] [diff] [review]
remove nsListBoxObject to clarify that the object isn't refcounted. + don't play refcounting games

review- per previous comments (the code you're removing still seems to be used
by code that you're not removing, etc.)
Attachment #104532 - Flags: review?(dbaron) → review-

Updated

15 years ago
Keywords: mozilla1.2
Target Milestone: mozilla1.2final → ---
QA Contact: moied → layout
Whiteboard: [needs patch that addresses review comments - timeless]
You need to log in before you can comment on or make changes to this bug.