Speculative fix for crash accessing a supercluster root.

Speculative fix for crash accessing a supercluster root.

BUG=369485
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173584

[skobes, 2014-05-07 00:24:55]

I'm not very happy with this but without a repro I can't think of anything
better.

RenderObject is not ref counted so we can't just fix it by using RefPtr
everywhere.

If we only delete the supercluster from m_superclusters when a BlockSet is
removed from m_blocksForFingerprint, there may still be clusters on the cluster
stack pointing to the deleted supercluster.

We could iterate over the cluster stack and try to make really surgical
adjustments but I'm not sure that's the best road to go down.  In theory we can
just blow away all our per-layout state and the next beginLayout will
reconstruct everything.

Ideally we would just not mutate the render tree during layout. :(

[pdr., 2014-05-07 05:44:07]

On 2014/05/07 00:24:55, skobes wrote:
> I'm not very happy with this but without a repro I can't think of anything
> better.
> 
> RenderObject is not ref counted so we can't just fix it by using RefPtr
> everywhere.
> 
> If we only delete the supercluster from m_superclusters when a BlockSet is
> removed from m_blocksForFingerprint, there may still be clusters on the
cluster
> stack pointing to the deleted supercluster.
> 
> We could iterate over the cluster stack and try to make really surgical
> adjustments but I'm not sure that's the best road to go down.  In theory we
can
> just blow away all our per-layout state and the next beginLayout will
> reconstruct everything.
> 
> Ideally we would just not mutate the render tree during layout. :(

I think this is rare enough that the approach you took is reasonable. We need a
testcase though or this will come back to bite us.

I made some progress on a testcase. It's not all the way there but I think it's
the pattern we were looking for:
https://docs.google.com/a/google.com/document/d/1AcY9RhkAc2lYb_dNYX2vttwReaQB...

[skobes, 2014-05-07 21:15:56]

On 2014/05/07 05:44:07, pdr wrote:
> I made some progress on a testcase. It's not all the way there but I think
it's
> the pattern we were looking for:
>
https://docs.google.com/a/google.com/document/d/1AcY9RhkAc2lYb_dNYX2vttwReaQB...

Looks cool!  But unfortunately, in your testcase, the blocks that get
destroy()'ed during layout are all of type RenderScrollbarPart.  But a
RenderScrollbarPart will never be a cluster root.  So I don't see how we can
modify the test case to exercise the code in this patch.

prepareForLayout of first block
supercluster root 0x24fdfee18220:RenderBlock DIV
supercluster root 0x24fdfee40010:RenderTable TABLE id='c1'
supercluster root 0x24fdfee6c010:RenderTableCell TD
supercluster root 0x24fdfee6c010:RenderTableCell TD
supercluster root 0x24fdfee40188:RenderTable TABLE id='c2'
supercluster root 0x24fdfee6c128:RenderTableCell TD
supercluster root 0x24fdfee6c128:RenderTableCell TD
destroy(0x24fdfee2c110:RenderScrollbarPart)
destroy(0x24fdfee2c210:RenderScrollbarPart)
destroy(0x24fdfee2c410:RenderScrollbarPart)
destroy(0x24fdfee2c510:RenderScrollbarPart)
supercluster root 0x24fdfee18538:RenderBlock DIV
supercluster root 0x24fdfee40300:RenderTable TABLE id='c3'
supercluster root 0x24fdfee6c240:RenderTableCell TD
endLayout of first block

[pdr., 2014-05-07 23:11:48]

Reluctant LGTM. I am also not very happy with this, but I think it's worth a
shot.

[skobes, 2014-05-07 23:13:49]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2014-05-07 23:15:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skobes@chromium.org/274463002/1

[commit-bot: I haz the power, 2014-05-08 00:31:15]

Change committed as 173584