242368 - Setting dynamically a table column's visibility in 1.8a will crash

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Gérard Talbot]

While working on a global testcase for bug 77019, bug 76497 and bug 242253 with
Mozilla 1.8a build 2004050108, I stumbled across a reproducible crash.
It seems that 

var ColumnCollection =
document.getElementById("idTable").getElementsByTagName("col");
ColumnCollection[2].style.visibility = "hidden"; // or collapse

cannot be executed.

Testcase coming up.

XP Pro here.

Comment 1

[Gérard Talbot]

Attachment: Reproducible CRASH testcase

Please make sure you're using a recent Mozilla 1.8a build.
Note that Opera 7.50 beta1 does not crash on this testcase.

Comment 2

[Gérard Talbot]

I must say that while trying attachment 31308 [details] of bugfile 76497, I do not crash
and I get the expected results (correct column visibility). Somehow, accessing
the node via nodeList from 
getElementsByTagName("col") 
instead of 
getElementById("idTargetedColumn")
causes the crash.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Happens on Linux too.  Stack:

#6  0x4117f35f in nsStyleBorder::GetBorder(nsMargin&) const (this=0xbfffdd10, 
    aBorder=@0x8832844) at nsStyleStruct.h:350
#7  0x412d6830 in TableBackgroundPainter::PaintTable(nsTableFrame*, nsMargin*) (
    this=0xbfffdd10, aTableFrame=0x8832844, aDeflate=0x0)
    at
/home/bzbarsky/mozilla/xlib/mozilla/layout/html/table/src/nsTablePainter.cpp:435
#8  0x412b438b in nsTableFrame::Paint(nsIPresContext*, nsIRenderingContext&,
nsRect const&, nsFramePaintLayer, unsigned) (this=0x8832844,
aPresContext=0x87659c0, 
    aRenderingContext=@0x8677860, aDirtyRect=@0xbfffde60, 
    aWhichLayer=eFramePaintLayer_Underlay, aFlags=0)
    at
/home/bzbarsky/mozilla/xlib/mozilla/layout/html/table/src/nsTableFrame.cpp:1427
#9  0x4119db6e in nsContainerFrame::PaintChild(nsIPresContext*,
nsIRenderingContext&, nsRect const&, nsIFrame*, nsFramePaintLayer, unsigned)
(this=0x88326e4, 
    aPresContext=0x87659c0, aRenderingContext=@0x8677860, aDirtyRect=@0xbfffdf00, 
    aFrame=0x8832844, aWhichLayer=eFramePaintLayer_Underlay, aFlags=0)
    at
/home/bzbarsky/mozilla/xlib/mozilla/layout/html/base/src/nsContainerFrame.cpp:303

And data:

(gdb) frame 7
#7  0x412d6830 in TableBackgroundPainter::PaintTable(nsTableFrame*, nsMargin*) (
    this=0xbfffdd10, aTableFrame=0x8832844, aDeflate=0x0)
    at
/home/bzbarsky/mozilla/xlib/mozilla/layout/html/table/src/nsTablePainter.cpp:435
435                 mCols[colIndex].mCol.mBorder->GetBorder(border);
(gdb) p mCols[colIndex].mCol.mBorder
$2 = (const nsStyleBorder *) 0x0
(gdb) p colIndex
$3 = 2
(gdb) p mCols[colIndex].mCol
$4 = {mFrame = 0x883a514, mRect = {x = 1162, y = 0, width = 546, height = 1512}, 
  mBackground = 0x0, mBorder = 0x0, mSynthBorder = 0x0}

mBorder is not set by TableBackgroundData::SetFull if
mFrame->IsVisibleForPainting() is false (as here).  But the loop over cols in
PaintTable() doesn't check visibility of the col....  It probable should.

Comment 4

[Gérard Talbot]

Attachment: reproducible CRASH testcase (better testcase)

This is strange: attachment 31308 [details] of bug 76497 works perfectly for me while
this testcase crashes (while using getElementById or getElementsByTagName) and
I don't know why.

Comment 5

[Bernd]

drunclear does it crash the 1.7rc1, the visibilty:hidden should be handled
identical for strict mode rendering.

Comment 6

[Bernd]

it crashes 1.7rc1 TB37208W

Comment 7

[Bernd]

this should be fixable before 1.7

Comment 8

[Gérard Talbot]

Yes, I crash with 1.7RC1 while trying attachment 147514 [details] (both buttons will crash
the application).

Comment 9

[fantasai]

Removing 
              mCols[colIndex].mCol.mBorder->GetBorder(border);

seems to fix it. At least, it's not crashing. It's not hiding the columns, either...

(That line doesn't seem to be doing anything, and the relevant background tests
 work fine without it.)

Bernd, does that seem right to you? Do you want a patch?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Er... wouldn't that break the border when cols have a border?  Or am I missing
something?

I'm still confused why the change I proposed (to check whether the col is
visible before doing anything with it) is wrong...

Comment 11

[fantasai]

> Er... wouldn't that break the border when cols have a border?  Or am I missing
> something?

It gets the border and then... does what?

http://lxr.mozilla.org/seamonkey/source/layout/html/table/src/nsTablePainter.cpp#414

> I'm still confused why the change I proposed (to check whether the col is
> visible before doing anything with it) is wrong...

Because if that line is not necessary, checking to see whether it's a valid
operation is not necessary. :)

Comment 12

[Boris Zbarsky [:bzbarsky]]

> It gets the border and then... does what?

Doesn't it affect the border that gets set on the next col?

Comment 13

[fantasai]

That's stashed in lastLeftBorder...
which really should be called nextLeftBorder. :/

Comment 14

[Boris Zbarsky [:bzbarsky]]

Er, yeah.  Then removing the line sounds fine to me.  ;)

Comment 15

[Bernd]

fantasai: could you please explain what this code inside the bordercollapse
switch should in theory do.

Comment 16

[fantasai]

Attachment: patch

Comment 17

[Boris Zbarsky [:bzbarsky]]

fantasai, I'm not really doing srs at the moment... poke roc or rbs?

Comment 18

[fantasai]

Comment on attachment 147637 [details] [diff] [review]
patch

Oh, sorry bz! *didn't notice that*

Comment 19

[Asa Dotzler [:asa]]

Comment on attachment 147637 [details] [diff] [review]
patch

a=asa (on behalf of drivers) for checkin to 1.7

Comment 20

[Bernd]

fix checked in trunk & branch

Comment 21

[Gérard Talbot]

I tried the testcase with Mozilla 1.8a build 2004050609 and I did not crash: so
this is FIXED.