Closed Bug 365151 Opened 15 years ago Closed 15 years ago

crash [@ nsStyleContext::GetRuleNode] or [@ nsTreeColumn::GetContent] due to stale nsTreeColumn::mFrame

Categories

(Core :: XUL, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: asqueella, Assigned: asqueella)

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(2 files, 8 obsolete files)

See bug 363791, in particular bug 363791 comment 11 for details.
For best results, load the testcase in a debug build. An opt build will end up accessing freed memory.
Attached patch like so? (obsolete) — Splinter Review
Yes, it's pretty error-prone. I'm not sure if it would be better to also add the necessary null-checks to protected nsTreeColumn methods.
Assignee: Jan.Varga → asqueella
Status: NEW → ASSIGNED
Attachment #249854 - Flags: superreview?(roc)
Attachment #249854 - Flags: review?(neil)
How about we just don't store mFrame in the object, and re-get mFrame whenever we need it?
Comment on attachment 249854 [details] [diff] [review]
like so?

> nsTreeBodyFrame::GetCellWidth(PRInt32 aRow, nsTreeColumn* aCol,
>                               nsIRenderingContext* aRenderingContext,
>                               nscoord& aDesiredSize, nscoord& aCurrentSize)
A -w diff would have been nice...

>+  NS_PRECONDITION(!aCol || aCol->mFrame, "invalid column passed");
>+  NS_PRECONDITION(aColumn && aColumn->mFrame, "invalid column passed");
>+  NS_PRECONDITION(aColumn && aColumn->mFrame, "invalid column passed");
>+  NS_PRECONDITION(aColumn && aColumn->mFrame, "invalid column passed");
>+  NS_PRECONDITION(aColumn && aColumn->mFrame, "invalid column passed");
>+  NS_PRECONDITION(aColumn && aColumn->mFrame, "invalid column passed");
>+  NS_PRECONDITION(aColumn && aColumn->mFrame, "invalid column passed");
>+  NS_PRECONDITION(aColumn && aColumn->mFrame, "invalid column passed");
>+  NS_ASSERTION(!primaryCol || primaryCol->mFrame, "primary column is invalid?");
Those first and last conditions look wrong.

Also, the testcase crashed my patched build.
Attachment #249854 - Flags: review?(neil) → review-
(In reply to comment #4)
>Also, the testcase crashed my patched build.
Sorry, that might have been my fault, the patch hadn't applied properly.
(In reply to comment #5)
>(In reply to comment #4)
>>Also, the testcase crashed my patched build.
>Sorry, that might have been my fault, the patch hadn't applied properly.
Yeah, the testcase doesn't crash with a correctly patch build ;-)

r=me with those preconditions fixed
Neil: Why are those conditions wrong? !aCol || aCol->mFrame means that either aCol is nsnull or it has a valid frame.

roc: As for not keeping mFrame at all, that does sound better than trying to null it out whenever the frame is destroyed. (I assume you meant keeping an owning ref to nsIContent for the <treecol> instead?)

I am not sure about performance implications of doing so, how slow is getting an nsIFrame given an element? (I'm not familiar with that code yet.) The columns' frames appears to be accessed pretty often during painting.
OK, I misunderstood the intent of those preconditions.

Do internal methods actually need preconditions?
(In reply to comment #8)
> Do internal methods actually need preconditions?
> 
Without those, it's much harder to convince yourself nothing in nsTreeBodyFrame calls mFrame-dependent methods on nsTreeColumn without ensuring the column has a frame.

One problem is that there's no clean way to indicate that there's no frame for the column from non-COM variant of GetX/GetWidth methods. Perhaps it makes sense to switch the callers to use the XPCOM variants of those methods or return some special value (-1?) to indicate failure. (It would be less error-prone to just propagate the failure than to ensure nothing calls those methods on a "dead" column).
(In reply to comment #7)
> roc: As for not keeping mFrame at all, that does sound better than trying to
> null it out whenever the frame is destroyed. (I assume you meant keeping an
> owning ref to nsIContent for the <treecol> instead?)

Yes.

> I am not sure about performance implications of doing so, how slow is getting
> an nsIFrame given an element? (I'm not familiar with that code yet.) The
> columns' frames appears to be accessed pretty often during painting.

Should be OK. If necessary, restructure the code so that the column frames are only acquired once per body paint.
Attached patch patch (obsolete) — Splinter Review
Attachment #249854 - Attachment is obsolete: true
Attachment #250363 - Flags: superreview?(roc)
Attachment #250363 - Flags: review?(neil)
Attachment #249854 - Flags: superreview?(roc)
Attached patch same as above as -w (obsolete) — Splinter Review
I would prefer GetRect, GetX and GetWidth to return rv and take the rect or nscoord as an out parameter.
OK, I'll fix that. Any other comments?
Attached patch patch (obsolete) — Splinter Review
Attachment #250363 - Attachment is obsolete: true
Attachment #250364 - Attachment is obsolete: true
Attachment #250371 - Flags: superreview?(roc)
Attachment #250371 - Flags: review?(neil)
Attachment #250363 - Flags: superreview?(roc)
Attachment #250363 - Flags: review?(neil)
Attached patch diff -w (obsolete) — Splinter Review
+   * The first column in the list of columns. All of the columns are supposed
+   * to be "alive", i.e. have a frame. This is achieved by clearing the columns
+   * list each time an nsTreeColFrame is destroyed.

Is this still true? I think it should say that some columns in the list may not have a frame. Isn't that so?

There is one more thing. I'd like your new methods to take nsIFrame* aBodyFrame as a parameter, and get the presshell via aBodyFrame->GetPresContext()->PresShell(). (Probably want a new helper GetFrame(nsIFrame* aBodyFrame).) This is probably slightly more efficient but more importantly it's correct in case we ever have multiple presentations.
(In reply to comment #17)
> +   * The first column in the list of columns. All of the columns are supposed
> +   * to be "alive", i.e. have a frame. This is achieved by clearing the
> columns
> +   * list each time an nsTreeColFrame is destroyed.
> 
> Is this still true? I think it should say that some columns in the list may not
> have a frame. Isn't that so?
> 
I think it's true for the reason listed here and some assertions in the patch assume this is the case as well.
Attached patch patch (obsolete) — Splinter Review
Attachment #250371 - Attachment is obsolete: true
Attachment #250372 - Attachment is obsolete: true
Attachment #250383 - Flags: superreview?(roc)
Attachment #250383 - Flags: review?(roc)
Attachment #250371 - Flags: superreview?(roc)
Attachment #250371 - Flags: review?(neil)
Attached patch diff -w (obsolete) — Splinter Review
Attachment #250383 - Flags: superreview?(roc)
Attachment #250383 - Flags: superreview+
Attachment #250383 - Flags: review?(roc)
Attachment #250383 - Flags: review+
Attached patch patch for checkin (obsolete) — Splinter Review
fixed merge conflicts.
Attachment #250383 - Attachment is obsolete: true
Attachment #250384 - Attachment is obsolete: true
Whiteboard: [checkin needed]
Attached patch for checkinSplinter Review
Updated to the tip again.
Attachment #250481 - Attachment is obsolete: true
mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp 	1.300
mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.h 	1.111
mozilla/layout/xul/base/src/tree/src/nsTreeColumns.cpp 	1.19
mozilla/layout/xul/base/src/tree/src/nsTreeColumns.h 	1.7
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Target Milestone: --- → mozilla1.9alpha
Flags: in-testsuite?
Component: XP Toolkit/Widgets: Trees → XUL
QA Contact: xptoolkit.trees → xptoolkit.widgets
Crash Signature: [@ nsStyleContext::GetRuleNode] [@ nsTreeColumn::GetContent]
Crash test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe6ec7fdc171
Crash Signature: [@ nsStyleContext::GetRuleNode] [@ nsTreeColumn::GetContent] → [@ nsStyleContext::GetRuleNode] [@ nsTreeColumn::GetContent]
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.