302725 - [FIX]Border-collapse sorta-leaks BCCellData structs

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Boris Zbarsky [:bzbarsky]]

More precisely, we never call their constructor.  For now that may not be an
issue because they only have integer members, but not a good idea in general...

The problem is that we're deleting these structs through a pointer to a
superclass (CellData, to be precise).  And the superclass has a non-virtual
destructor.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

The other approach is to find all places where we delete CellData structs and
put in checks to decide whether to cast to BCCellData before calling delete...
this seemed saner.

Comment 2

[Bernd]

http://blogs.msdn.com/oldnewthing/archive/2004/05/07/127826.aspx

Comment 3

[Bernd]

>More precisely, we never call their constructor.
Boris, doesn't
http://lxr.mozilla.org/mozilla/source/layout/tables/nsCellMap.cpp#64 call the
constructor. I am just trying to understand the impact.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Er, I meant we never call their _destructor_.  Which means that any members that
have destructors never get those destructors called (which may be ok so far,
since there aren't any, but if we ever add some we're in trouble) and the
destructor code itself doesn't run, so the counter is never decremented so the
leak logs say we leak the object.  I _think_ we deallocate the right amount of
memory....  but I'm not sure.

Comment 5

[Bernd]

Comment on attachment 191017 [details] [diff] [review]
Proposed patch

The price way pay for this is we double the size of the cellmap for every
ordinary table (from 4 to 8 bytes per cellmap coordinate) and another 4 bytes
to the BCCellData (from 16 bytes to 20 bytes) for every cellmap coordinate.
This is due to the vtable.

Since nobody cried when the BCCellData went from 8 bytes to 16 bytes (see bug
286794) I guess its fine and its more correct anyway.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Like I said, the other option is to fix all the delete calls to cast to the
right type...

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

How many delete calls are there?  It seems worth doing if there aren't too many.

Comment 8

[Boris Zbarsky [:bzbarsky]]

I'm not sure.... We have delete calls for CellData* at least in:

nsCellMap::~nsCellMap
nsCellMap::ShrinkWithoutRows
nsCellMap::ShrinkWithoutCell
nsCellMap::RebuildConsideringRows
nsCellMap::RebuildConsideringCells
nsCellMap::SetDataAt

That might actually be all of them.  Not sure which of those could be BCCellData* or how one would tell.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Bernd, do you know which of the callsites in comment 8 we'd need to fix?

Comment 10

[Bernd]

I think all of them, please note that once you know the cell data type it does not change. A cellmap is either BC or not, there is no mix.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Could a cellmap have functions:

  PRBool IsBC();
  void DeleteCellData(CellData *aCellData);

where the latter would rely on being a friend, and the destructor could be private?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Unfortunately, it seems to be the nsTableCellMap that knows whether we're BC or not.  And not all the methods that delete cell data (eg ~nsCellMap) have one...  bernd, any ideas?

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Why not just add a member variable to the cell map?  (Probably a boolean for whether it's BC is better than a parent link.)

Comment 14

[Boris Zbarsky [:bzbarsky]]

That'd probably work if we don't have too many of them around.  Bernd, would that be ok with you?

Comment 15

[Bernd]

Sure, one flag per cellmap (a cellmap corresponds to a rowgroup) is much better than the virtual overhaead per cell entry.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: So like this

Summary of changes:

1)  Make celldata constructor/destructor private and allocate/deallocate them in
    only one place (DestroyCellData/AllocCellData methods).
2)  Make use of the fact that |delete 0| is ok (eliminate some if statements).
3)  Assert as needed that mIsBC matches the mBCInfo state of the nsTableCellMap
4)  Remove a few tabs
5)  Fix some compile warnings due to printf with %p as the format not being
    passed void*.

Comment 17

[Bernd]

Comment on attachment 203745 [details] [diff] [review]
So like this

+ nsCellMap* newMap = new nsCellMap(aNewGroup, mBCInfo != nsnull);
I think table code does not do nsnull comparisons, at least I remove them whenever I see them.

+  NS_ASSERTION(!!aMap.mBCInfo == mIsBC, "BC state mismatch");

this thing looks a little bit wired, lxr does not give a search for such a construct, but yes its valid.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> I think table code does not do nsnull comparisons

The options are "!!mBCInfo" and "mBCInfo != nsnull".  The latter seems less ugly to me.  Note that I need to convert a pointer to a PRBool here, so just passing mBCInfo is not an option.

> this thing looks a little bit wired

Similar issue.  I could have written this as:

+  NS_ASSERTION((aMap.mBCInfo != nsnull) == mIsBC, "BC state mismatch");

but that would have been more confusing, I think.

Comment 19

[Bernd]

yeah, it looked only alien to *me*.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 203745 [details] [diff] [review]
So like this

sr=dbaron.  Sorry for the delay.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to tip

Comment 22

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.