364318 - [FIX]shrinkwithoutcells should verify cells before deleting them

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect, P1)

Description

[Bernd]

this is a regression from bug 356335

Comment 1

[Bernd]

Attachment: testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

So... I don't crash with the testcase.  nsCellMap::DestroyCellData is null-safe, so is the problem that endColIndex or aColIndex might be out of range?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Er, I was using the build that has bug 357729 fixed (and therefore a lot of this stuff is safer)...

I'll try to look into this today, but I'm not sure I'll have time.  :(

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hmm... So yeah, this is a problem with that patch too.  It's just that nsTArray::RemoveElementsAt doesn't assert.  It should.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: This fixes the bug for me

Comment 6

[Bernd]

+    PRUint32 endIndexForRow = PR_MIN(endColIndex + 1, row.Length());

I believe that if this coerce, while being correct from a point of view "you shall not go past the array", kicks in, it signals a serious cellmap problem.

How can a cell with a rowspan and colspan not warrant the existence of the corresponding entries in the rows. In short, it should martially assert.

I suspect there is a second issue with 0-spans, that I am going to debug.

Comment 7

[Bernd]

The zero rowspan issue comes from http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsCellMap.cpp&rev=3.116&mark=2007-2009#1983
which is now just wrong.


It makes us trying to remove the rowspan  from row 2. The patch will lead to a removal of the rowspan belonging to C1,0

cellmap dump:

 thead mapRowCount=3 tableRowCount=2
 row 0 : C0,0  C0,1
 row 1 : C1,0  R
 row 2 : R

I would prefer to only assert and to bail.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Assert and bail in which case?  If we get to a row for which row.Length() <= endColIndex?

Comment 9

[Bernd]

Comment on attachment 249117 [details] [diff] [review]
This fixes the bug for me

as usual "reading the patch helps"

Deep in my heart, I would prefer to put after NS_ASSERTION(count == 0 || start < Length(), "Invalid start index");
a 0 deref  that crashes hard because if this triggers we have a serious buffer overflow issue.

Comment 10

[Bernd]

Attachment: patch for the underlying problem

Comment 11

[Boris Zbarsky [:bzbarsky]]

Bernd, you convinced me that my patch should assert and bail, btw.  So how do I interpret your r+ on it?

Comment 12

[Bernd]

Your patch does already bail. Just assert that endColIndex + 1 <= row.Length(), so that we get alerted if we coerce.

Comment 13

[Boris Zbarsky [:bzbarsky]]

My patch doesn't bail out of the function by any means.  It just skips the DestroyCellData and RemoveElementsAt calls when they don't make sense.  So it doesn't sound to me like it does what you want it to do....

Comment 14

[Bernd]

I checked in attachment 249117 [details] [diff] [review]. The coerce of the endrowindex will cause exactly the bail that I asked for as endrowindex will become smaller than startrowindex and the for loop will do nothing.  I also added the assert.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 249214 [details] [diff] [review]
patch for the underlying problem

>Index: nsCellMap.cpp
> PRInt32 nsCellMap::GetRowSpan(PRInt32  aRowIndex,

So the idea here is that now that we do non-lazy zero-rowspan expansion the cellmap always reflects reality and we shouldn't mess with the return value due to zero-rowspans, right?

>Index: nsCellMap.h
>   PRInt32 GetRowSpan(PRInt32 aRowIndex,
>                      PRInt32 aColIndex,
>-                     PRBool  aGetEffective,
>-                     PRBool& aIsZeroRowSpan) const;
>+                     PRBool  aGetEffective) const;

Could you please add documentation explaining what this function does?  Especialy how aGetEffective affects the return value?

r+sr=bzbarsky assuming the answer to my question is "yes" and with the comments added.

Comment 16

[Bernd]

>So the idea here is that now that we do non-lazy zero-rowspan expansion the
>cellmap always reflects reality and we shouldn't mess with the return value due
>to zero-rowspans, right?
exactly

>Could you please add documentation explaining what this function does? 
>Especialy how aGetEffective affects the return value?

done see the commit, the language might be not perfect, I am open for a better wording

Comment 17

[Boris Zbarsky [:bzbarsky]]

Language looks fine to me.  Thanks!

Comment 18

[Boris Zbarsky [:bzbarsky]]

I missed the fact that this patch included the one for bug 363239 (which didn't have reviews yet at the time)...