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

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Nickolay_Ponomarev]

See bug 363791, in particular bug 363791 comment 11 for details.

Comment 1

[Nickolay_Ponomarev]

For best results, load the testcase in a debug build. An opt build will end up accessing freed memory.

Comment 2

[Nickolay_Ponomarev]

Attachment: like so?

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.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How about we just don't store mFrame in the object, and re-get mFrame whenever we need it?

Comment 4

[[e-mail]]

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.

Comment 5

[[e-mail]]

(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

[[e-mail]]

(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

Comment 7

[Nickolay_Ponomarev]

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

[[e-mail]]

OK, I misunderstood the intent of those preconditions.

Do internal methods actually need preconditions?

Comment 9

[Nickolay_Ponomarev]

(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).

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 11

[Nickolay_Ponomarev]

Attachment: patch

Comment 12

[Nickolay_Ponomarev]

Attachment: same as above as -w

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I would prefer GetRect, GetX and GetWidth to return rv and take the rect or nscoord as an out parameter.

Comment 14

[Nickolay_Ponomarev]

OK, I'll fix that. Any other comments?

Comment 15

[Nickolay_Ponomarev]

Attachment: patch

Comment 16

[Nickolay_Ponomarev]

Attachment: diff -w

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+   * 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.

Comment 18

[Nickolay_Ponomarev]

(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.

Comment 19

[Nickolay_Ponomarev]

Attachment: patch

Comment 20

[Nickolay_Ponomarev]

Attachment: diff -w

Comment 21

[Nickolay_Ponomarev]

Attachment: patch for checkin

fixed merge conflicts.

Comment 22

[Nickolay_Ponomarev]

Attachment: for checkin

Updated to the tip again.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

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

Comment 24

[Mats Palmgren (inactive)]

Crash test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe6ec7fdc171

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fe6ec7fdc171