Closed Bug 245295 Opened 20 years ago Closed 20 years ago

Crash [@ nsRuleNode::ComputeBackgroundData]


(Core :: CSS Parsing and Computation, defect)

Windows XP
Not set





(Reporter: timeless, Assigned: dewildt)



(Keywords: crash, helpwanted, Whiteboard: [good first bug] oom)

Crash Data


(1 file, 1 obsolete file)

PRUint8 parentFlags = parentBG->mBackgroundFlags;

+	parentBG	0x00000000 {mBackgroundFlags=??? mBackgroundAttachment=???
mBackgroundClip=??? ...}	const nsStyleBackground *

>	gklayout.dll!nsRuleNode::ComputeBackgroundData(nsStyleStruct *
aStartStruct=0x00000000, const nsCSSStruct & aData={...}, nsStyleContext *
aContext=0x0284fe48, nsRuleNode * aHighestNode=0x027b6f60, const
nsRuleNode::RuleDetail & aRuleDetail=eRulePartialReset, int aInherited=0) 
Line 2911	C++
aSID=eStyleStruct_Background, nsStyleContext * aContext=0x0284fe48, nsRuleData *
aRuleData=0x0012eb24, nsCSSStruct * aSpecificData=0x0012eb64)  Line 75	C++
 	gklayout.dll!nsRuleNode::GetBackgroundData(nsStyleContext *
aContext=0x00000000)  Line 1072 + 0x14	C++
aSID=eStyleStruct_Background, nsStyleContext * aContext=0x0284fe48, int
aComputeData=1)  Line 75	C++
aSID=eStyleStruct_Background)  Line 250 + 0xf	C++
 	gklayout.dll!nsStyleContext::CalcStyleDifference(nsStyleContext *
aOther=0x00000001)  Line 490 + 0x19	C++
 	gklayout.dll!CaptureChange(nsStyleContext * aOldContext=0x022a7338,
nsStyleContext * aNewContext=0x0284fe48, nsIFrame * aFrame=0x02d44e30,
nsIContent * aContent=0x02e5ba10, nsStyleChangeList * aChangeList=0x0012ecc0,
nsChangeHint aMinChange=0)  Line 1320	C++
 	gklayout.dll!nsFrameManager::ReResolveStyleContext(nsIPresContext *
aPresContext=0x022a7338, nsIFrame * aFrame=0x02d44e30, nsIContent *
aParentContent=0x0284fe48, nsStyleChangeList * aChangeList=0x0012ecc0,
nsChangeHint aMinChange=0)  Line 1430 + 0x14	C++
 	gklayout.dll!nsFrameManager::ComputeStyleChangeFor(nsIFrame *
aFrame=0x02d44e30, nsStyleChangeList * aChangeList=0x0012ecc0, nsChangeHint
aMinChange=47468080)  Line 1688	C++
 	gklayout.dll!nsCSSFrameConstructor::AttributeChanged(nsIPresContext *
aPresContext=0x00000000, nsIContent * aContent=0x00000000, int aNameSpaceID=0,
nsIAtom * aAttribute=0x00000000, int aModType=0)  Line 10092 + 0x11	C++
 	gklayout.dll!PresShell::AttributeChanged(nsIDocument * aDocument=0x024c6a50,
nsIContent * aContent=0x02e5ba10, int aNameSpaceID=0, nsIAtom *
aAttribute=0x009aacc8, int aModType=2)  Line 5263	C++
 	gklayout.dll!nsXULDocument::AttributeChanged(nsIContent * aElement=0x00000000,
int aNameSpaceID=0, nsIAtom * aAttribute=0x00000000, int aModType=0)  Line
1137 + 0x14	C++
 	gklayout.dll!nsXULElement::SetAttrAndNotify(int aNamespaceID=0, nsIAtom *
aAttribute=0x009aacc8, nsIAtom * aPrefix=0x00000000, const nsAString &
aOldValue={...}, nsAttrValue & aParsedValue={...}, int aModification=33554432,
int aFireMutation=0, int aNotify=1)  Line 2172	C++
 	gklayout.dll!nsXULElement::SetAttr(int aNamespaceID=0, nsIAtom *
aName=0x00000000, nsIAtom * aPrefix=0x00000000, const nsAString & aValue={...},
int aNotify=0)  Line 2095 + 0x1f	C++
 	gklayout.dll!nsSplitterFrameInner::MouseMove(nsIDOMEvent *
aMouseEvent=0x0366d468)  Line 928 + 0x39	C++
 	gklayout.dll!nsEventListenerManager::HandleEvent(nsIPresContext *
aPresContext=0x02e9a334, nsEvent * aEvent=0x0012f984, nsIDOMEvent * *
aDOMEvent=0x0012f754, nsIDOMEventTarget * aCurrentTarget=0x0366d4e8, unsigned
int aFlags=2, nsEventStatus * aEventStatus=0x0012f8d8)  Line 1574 + 0x29	C++
 	gklayout.dll!nsXULElement::HandleDOMEvent(nsIPresContext *
aPresContext=0x022a7338, nsEvent * aEvent=0x0366d4e8, nsIDOMEvent * *
aDOMEvent=0x0012f754, unsigned int aFlags=2, nsEventStatus *
aEventStatus=0x0012f8d8)  Line 2788	C++
 	gklayout.dll!nsXULElement::HandleDOMEvent(nsIPresContext *
aPresContext=0x022a7338, nsEvent * aEvent=0x0012f984, nsIDOMEvent * *
aDOMEvent=0x0012f754, unsigned int aFlags=7, nsEventStatus *
aEventStatus=0x0012f8d8)  Line 2807	C++
 	gklayout.dll!PresShell::HandleEventInternal(nsEvent * aEvent=0x024e3688,
nsIView * aView=0x02e9a3c8, unsigned int aFlags=1, nsEventStatus *
aStatus=0x0012f8d8)  Line 6073 + 0x11	C++
 	gklayout.dll!PresShell::HandleEvent(nsIView * aView=0x02e9a3c8, nsGUIEvent *
aEvent=0x0012f984, nsEventStatus * aEventStatus=0x0012f8d8, int aForceHandle=1,
int & aHandled=15110296)  Line 5966 + 0x11	C++
 	gklayout.dll!nsViewManager::HandleEvent(nsView * aView=0x00000000, nsGUIEvent
* aEvent=0x00000000, int aCaptured=0)  Line 2199	C++
 	gklayout.dll!nsViewManager::DispatchEvent(nsGUIEvent * aEvent=0x3d888889,
nsEventStatus * aStatus=0x0012f940)  Line 1939 + 0x14	C++
 	gklayout.dll!HandleEvent(nsGUIEvent * aEvent=0x0012f984)  Line 79	C++
 	gkwidget.dll!nsWindow::DispatchEvent(nsGUIEvent * event=0x0012f984,
nsEventStatus & aStatus=nsEventStatus_eIgnore)  Line 1067 + 0x3	C++
 	gkwidget.dll!nsWindow::DispatchWindowEvent(nsGUIEvent * event=0x00000000) 
Line 1088	C++
 	gkwidget.dll!nsWindow::DispatchMouseEvent(unsigned int aEventType=300,
unsigned int wParam=1, nsPoint * aPoint=0x00000000)  Line 5203	C++
 	gkwidget.dll!ChildWindow::DispatchMouseEvent(unsigned int aEventType=300,
unsigned int wParam=1, nsPoint * aPoint=0x00000000)  Line 5455 + 0x13	C++
 	gkwidget.dll!nsWindow::ProcessMessage(unsigned int msg=512, unsigned int
wParam=1, long lParam=19071129, long * aRetValue=0x0012fc88)  Line 3937	C++
 	gkwidget.dll!nsWindow::WindowProc(HWND__ * hWnd=0x003a0de6, unsigned int
msg=512, unsigned int wParam=1, long lParam=38627148)  Line 1349 + 0x10	C++
 	user32.dll!_InternalCallWinProc@20()  + 0x1b	
 	user32.dll!_UserCallWinProcCheckWow@32()  + 0xb7	
 	user32.dll!_DispatchMessageWorker@8()  + 0xd8	
 	user32.dll!_DispatchMessageW@4()  + 0xb	
 	gkwidget.dll!nsAppShell::Run()  Line 159	C++
 	appshell.dll!nsAppShellService::Run()  Line 524	C++
 	mozilla.exe!main1(int argc=0, char * * argv=0x00000000, nsISupports *
nativeApp=0x00000000)  Line 1302 + 0x9	C++
 	mozilla.exe!main(int argc=1, char * * argv=0x002a41f0)  Line 1779 + 0x16	C++
 	mozilla.exe!WinMain(HINSTANCE__ * __formal=0x00400000, HINSTANCE__ *
__formal=0x00400000, char * args=0x00152317, HINSTANCE__ * __formal=0x00400000)
 Line 1807 + 0x17	C++
 	mozilla.exe!WinMainCRTStartup()  Line 392 + 0xf	C
 	kernel32.dll!_BaseProcessStart@4()  + 0x23
+	this->mPresContext	0x00000000 {mShell=??? mDeviceContext=???
mEventManager=??? ...}	nsIPresContext *

    bg = new (mPresContext) nsStyleBackground(mPresContext);

  const nsStyleBackground* parentBG = bg;

So it /looks/ like the placement new decided not to try to crash.

and then parentBG was null, so it crashed later.

no idea how this happened. we are playing with content policies...
"new (mPresContext) ..." eventually winds up in PL_ArenaAllocate() as far as
I can see and it can return NULL.  nsRuleNode.cpp uses these allocations quite
a lot - maybe we need to start adding NULL-checks on those?
Hmm...  we _could_ just bail out of that stuff on OOM, and then fall back on the
code in nsRuleNode::GetStyleData() (at the end) to cover up to callers.  It
looks like just returning null from the various functions involved may work...
*** Bug 277575 has been marked as a duplicate of this bug. ***
Fixing this should be pretty straightforward -- a matter of verifying the
codepaths to make sure that the checks in nsRuleNode::GetStyleData work to
handle OOM if null is returned on allocation failures, as I suspect they do.
Keywords: helpwanted
Whiteboard: [good first bug]
Whiteboard: [good first bug] → [good first bug] oom
Two questions for my understanding

About "GetStyleData":
Maybe not directly related to this bug. Line 4803 contains the comment
> // To ensure that |GetStyleData| never returns null
but in line 4789 is nsnull returned. Is this wrong or is here a comment missing?

About "Compute<name>Data":
What is if the function fails to allocate memory for
"aHighestNode->mStyleData.mInheritedData" or "aHighestNode->mStyleData.mResetData"?
Should the previous allocated nsStyle<name> (Background etc.) released and
nsnull returned or should only be avoided to store the data in
> Is this wrong or is here a comment missing?

GetStyleData should never return null when aComputeData is true (i.e. when
someone really wants style data).  If aComputeData is false, GetStyleData really
means "check whether you have cached data and return it if you do"; in that
case, returning null is ok.

If allocations in ComputeXXXData fail, I think you want to delete the struct and
return null.  Otherwise we will leak the struct (the reset/inherited data takes
ownership).  Notice that nsStyleContext::SetStyle() can also have allocation
failures, though I think we have a separate bug on that.
I checked all "new (mPresContext) ...", not only those of the "ComputeXXXData"
(See duped bug 277575 comment 1)
Fixed also a small float-nscoord conversion warning.

The allocation failure in nsStyleContext::SetStyle is bug 281096
Assignee: dbaron → mozilla3q04
Attachment #174124 - Flags: review?(bzbarsky)
Attachment #174124 - Flags: superreview?(dbaron)
Comment on attachment 174124 [details] [diff] [review]
Add OOM checks to "new (mPresContext)"

>Index: mozilla/layout/style/nsRuleNode.cpp
>+    if (!newChildrenList) {

Want to make this NS_UNLIKELY(!newChildrenList) ?  And similar for the other
OOM checks you add?

> +      delete next;

That seems wrong.  |next| is a rulenode; those don't have an operator delete
the right way to destroy them is to call Destroy() on them.  Perhaps we should
have an operator delete that just calls Destroy?

> nsRuleNode::SetDefaultOnRoot(const nsStyleStructID aSID, 
>+      if (!fontData)
>+        return nsnull;

I think it'd make more sense to do:

if (NS_LIKELY(fontData)) {
  // stuff we do now with fontData

And just leave the |return fontData;|, since it'll return the right thing
(null) as needed?

Similar for the rest of SetDefaultOnRoot.

>@@ -2011,19 +2063,20 @@ nsRuleNode::ComputeFontData(nsStyleStruc
>-  if (!font) {
>+  if (!font)
>     font = new (mPresContext) nsStyleFont(mPresContext);
>-  }

Leave the curly braces as they were, please.

>+  if (!font)
>+    return nsnull; // Out Of Memory

And for new code, put the curlies in, since that seems to be the preferred
style for this file.

Don't you need to skip calling the post-resolve callback in WalkRuleTree if
Compute##name##Data returns null?

Also, I'd say you want a check up in the place where we do

  font = new (mPresContext) nsStyleFont(*parentFont);

(otherwise, if that allocation fails and the new one succeeds, we'll get
somewhat bizarre results...).  And in both cases, I think you want to put the
OOM check inside the body of the if where we actually allocate the font...

>@@ -2084,16 +2137,20 @@ nsRuleNode::ComputeFontData(nsStyleStruc
>+    if (!aHighestNode->mStyleData.mInheritedData) {
>+      delete font;

Again, style structs should be destroyed by calling Destroy() on them.

Also, do that test within the existing if body so it's not done twice all the

Fix that here and for the other structs, and I'll take another look?
Attachment #174124 - Flags: superreview?(dbaron)
Attachment #174124 - Flags: superreview-
Attachment #174124 - Flags: review?(bzbarsky)
Attachment #174124 - Flags: review-
Attached patch Patch v1.2Splinter Review
> Leave the curly braces as they were, please.

> And for new code, put the curlies in, since that seems to be the preferred
style for this file.

The style seems to be a mixture between braces and no braces. It seems that
only ComputeFontData uses the braces for the single line if-body. (That's the
reason why I changed that part.)
I didn't add the braces, because I think that is the most used here. (Sorry if
I'm wrong, I will add them if it's still desired.) I only add braces for
SetDefaultOnRoot because that part used them.

> Also, I'd say you want a check up in the place where we do

I moved the parts like
   if (!text)
     text = new (mPresContext) nsStyleText();
into the previous if construct, so we have after that if-construct a valid
style element. I think this is faster, uses less code and clearer as an OOM
check after each new.

> Don't you need to skip calling the post-resolve callback in WalkRuleTree if
Compute##name##Data returns null?

Yes, not all of the callback function have a parameter null check.
Attachment #174124 - Attachment is obsolete: true
Attachment #175475 - Flags: review?(bzbarsky)
Comment on attachment 175475 [details] [diff] [review]
Patch v1.2

r+sr=bzbarsky; I'll try to check this in tomorrow.
Attachment #175475 - Flags: superreview+
Attachment #175475 - Flags: review?(bzbarsky)
Attachment #175475 - Flags: review+
Fix checked in for 1.8b2.  Daniel, thanks for the patch!
Closed: 20 years ago
Resolution: --- → FIXED
Bug 277575 has the in-testsuite+ flag set. This bug is a dupe of 277575. Should there not be a test case associated with this bug then? How should the in-testsuite value be set for this bug?
Depends on what was actually added to a testsuite in bug 277575.  Ideally, we would have a test that covers the various codepaths touched in this bug in an automated suite; bug 277575 sure isn't there.
Crash Signature: [@ nsRuleNode::ComputeBackgroundData]
You need to log in before you can comment on or make changes to this bug.