Closed
Bug 365151
Opened 18 years ago
Closed 18 years ago
crash [@ nsStyleContext::GetRuleNode] or [@ nsTreeColumn::GetContent] due to stale nsTreeColumn::mFrame
Categories
(Core :: XUL, defect)
Core
XUL
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: asqueella, Assigned: asqueella)
Details
(Keywords: crash, testcase)
Crash Data
Attachments
(2 files, 8 obsolete files)
18 years ago
718 bytes,
application/vnd.mozilla.xul+xml
|
Details | |
49.03 KB,
patch
|
Details | Diff | Splinter Review |
See bug 363791, in particular bug 363791 comment 11 for details.
Assignee | ||
Comment 1•18 years ago
|
||
For best results, load the testcase in a debug build. An opt build will end up accessing freed memory.
Assignee | ||
Comment 2•18 years ago
|
||
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
Assignee | ||
Updated•18 years ago
|
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 4•18 years ago
|
||
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-
Comment 5•18 years ago
|
||
(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.
Comment 6•18 years ago
|
||
(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
Assignee | ||
Comment 7•18 years ago
|
||
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.
Comment 8•18 years ago
|
||
OK, I misunderstood the intent of those preconditions.
Do internal methods actually need preconditions?
Assignee | ||
Comment 9•18 years ago
|
||
(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.
Assignee | ||
Comment 11•18 years ago
|
||
Attachment #249854 -
Attachment is obsolete: true
Attachment #250363 -
Flags: superreview?(roc)
Attachment #250363 -
Flags: review?(neil)
Attachment #249854 -
Flags: superreview?(roc)
Assignee | ||
Comment 12•18 years ago
|
||
I would prefer GetRect, GetX and GetWidth to return rv and take the rect or nscoord as an out parameter.
Assignee | ||
Comment 14•18 years ago
|
||
OK, I'll fix that. Any other comments?
Assignee | ||
Comment 15•18 years ago
|
||
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)
Assignee | ||
Comment 16•18 years ago
|
||
+ * 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.
Assignee | ||
Comment 18•18 years ago
|
||
(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.
Assignee | ||
Comment 19•18 years ago
|
||
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)
Assignee | ||
Comment 20•18 years ago
|
||
Attachment #250383 -
Flags: superreview?(roc)
Attachment #250383 -
Flags: superreview+
Attachment #250383 -
Flags: review?(roc)
Attachment #250383 -
Flags: review+
Assignee | ||
Comment 21•18 years ago
|
||
fixed merge conflicts.
Attachment #250383 -
Attachment is obsolete: true
Attachment #250384 -
Attachment is obsolete: true
Assignee | ||
Updated•18 years ago
|
Whiteboard: [checkin needed]
Assignee | ||
Comment 22•18 years ago
|
||
Updated to the tip again.
Attachment #250481 -
Attachment is obsolete: true
Comment 23•18 years ago
|
||
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: 18 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Target Milestone: --- → mozilla1.9alpha
Assignee | ||
Updated•18 years ago
|
Flags: in-testsuite?
Component: XP Toolkit/Widgets: Trees → XUL
QA Contact: xptoolkit.trees → xptoolkit.widgets
Updated•14 years ago
|
Crash Signature: [@ nsStyleContext::GetRuleNode]
[@ nsTreeColumn::GetContent]
Comment 24•12 years ago
|
||
Crash Signature: [@ nsStyleContext::GetRuleNode]
[@ nsTreeColumn::GetContent] → [@ nsStyleContext::GetRuleNode]
[@ nsTreeColumn::GetContent]
Flags: in-testsuite? → in-testsuite+
Comment 25•12 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•