Bug 208397 – ASSERT(m_column != unsetColumnIndex) in RenderTable::cellBefore

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Doug Kelly]

When calling RenderTable::cellBefore as part of inserting a new cell in JavaScript on a table row that is not visible, cell->col() is coming back as unsetColumnIndex, which leads to an assert (and eventually, a crash).

<rdar://problem/59355313>

[Doug Kelly]

Created attachment 392025 [details]
Patch

[zalan]

Comment on attachment 392025 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392025&action=review

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=208397

radar# please.

> Source/WebCore/ChangeLog:10
> +        When inserting a cell into a table row which is not visible, this can lead to part of the rendering being triggered
> +        before layout is complete.  Instead, mark the layer as dirty using dirtyVisibleContentStatus(), and the visibility
> +        will be recomputed at a later time.

I think it's more about trying to compute the repaint rect while in the middle of tree building (I looked at the stacktrace in rdar://problem/59355313 and did not see any layout related code)

> Source/WebCore/rendering/RenderElement.cpp:888
> +            layer->dirtyVisibleContentStatus();

I think this setHasVisibleContent() should be completely eliminated since the other callsite (RenderElement::styleWillChange) does not make too much sense either. Simon should be able to confirm this.

[Alexey Proskuryakov]

Does this fully fix bug 78695?

[zalan]

(In reply to Alexey Proskuryakov from comment #3)
> Does this fully fix bug 78695?
I'd be very surprised if it did. While this and bug 78695 trigger the same assertion, we get there in 2 completely different codepaths (they both totally wrong though, -this one is because of computing repaint rect while inserting renderers into the tree, the other one is because running geometry computation while receiving image data)

[Doug Kelly]

(In reply to zalan from comment #2)
> Comment on attachment 392025 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=392025&action=review
> 
> > Source/WebCore/ChangeLog:4
> > +        https://bugs.webkit.org/show_bug.cgi?id=208397
> 
> radar# please.
> 
> > Source/WebCore/ChangeLog:10
> > +        When inserting a cell into a table row which is not visible, this can lead to part of the rendering being triggered
> > +        before layout is complete.  Instead, mark the layer as dirty using dirtyVisibleContentStatus(), and the visibility
> > +        will be recomputed at a later time.
> 
> I think it's more about trying to compute the repaint rect while in the
> middle of tree building (I looked at the stacktrace in
> rdar://problem/59355313 and did not see any layout related code)
>

Perhaps my phrasing is poor; when Simon, Ryosuke and I were looking at this, you're right, we weren't into  calling from layout code, but calling setHasVisibleContent() before layout has occurred was the problem.  I'm happy to rephrase this, though. :)
 
> > Source/WebCore/rendering/RenderElement.cpp:888
> > +            layer->dirtyVisibleContentStatus();
> 
> I think this setHasVisibleContent() should be completely eliminated since
> the other callsite (RenderElement::styleWillChange) does not make too much
> sense either. Simon should be able to confirm this.

I'm all for removing code if not needed (and if you are correct, this would be an entire function which would not be needed?) -- but seems outside the scope of this change?

[Doug Kelly]

Created attachment 392148 [details]
Patch

[zalan]

Comment on attachment 392148 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392148&action=review

> LayoutTests/fast/table/insert-cell-invisible-parent.html:18
> +<table rules="none" codebase="a">
> +<tr id="row" codebase="a">

Are those attributes really needed here to repro this issue?

[Doug Kelly]

(In reply to zalan from comment #7)
> Comment on attachment 392148 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=392148&action=review
> 
> > LayoutTests/fast/table/insert-cell-invisible-parent.html:18
> > +<table rules="none" codebase="a">
> > +<tr id="row" codebase="a">
> 
> Are those attributes really needed here to repro this issue?

At least in my testing, I believe they were.  I do try to remove any extraneous attributes.

[zalan]

(In reply to Doug Kelly from comment #8)
> (In reply to zalan from comment #7)
> > Comment on attachment 392148 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=392148&action=review
> > 
> > > LayoutTests/fast/table/insert-cell-invisible-parent.html:18
> > > +<table rules="none" codebase="a">
> > > +<tr id="row" codebase="a">
> > 
> > Are those attributes really needed here to repro this issue?
> 
> At least in my testing, I believe they were.  I do try to remove any
> extraneous attributes.
I can make ASan r257361 crash without the codebase attributes.

[Doug Kelly]

Created attachment 392150 [details]
Patch

[Doug Kelly]

(In reply to zalan from comment #9)
> (In reply to Doug Kelly from comment #8)
> > (In reply to zalan from comment #7)
> > > Comment on attachment 392148 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=392148&action=review
> > > 
> > > > LayoutTests/fast/table/insert-cell-invisible-parent.html:18
> > > > +<table rules="none" codebase="a">
> > > > +<tr id="row" codebase="a">
> > > 
> > > Are those attributes really needed here to repro this issue?
> > 
> > At least in my testing, I believe they were.  I do try to remove any
> > extraneous attributes.
> I can make ASan r257361 crash without the codebase attributes.

Yep, I stand corrected.  I just tested it again myself and confirmed these weren't actually needed.

[Doug Kelly]

Created attachment 392151 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 392151 [details]
Patch

Clearing flags on attachment: 392151

Committed r257720: <https://trac.webkit.org/changeset/257720>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.