Question on line box tree, relevant to a crash on gmail (#162564)

Hi I've looked a bit at #162564, and I can see why it crashes, but I am
a bit at loss about how it's intended to work/how to fix it. The
redundancy between the two trees is quite confusing.

The crash scenario here is this:
(1) is called with fullLayout set.
(2) It calls dirtyInlineBoxes(fullLayout) on a RenderText kid, which in turn
calls deleteInlineBoxes (since fullLayout is true)
(3) that deletes all the box kids of the object, and sets the
m_firstTextBox/m_lastTextBox of the RenderText to 0. However, the parent
inline box is not touched at all.
(4) Later on, layoutInlineChildren calls determineStartPosition. Since
fullLayout is true, it walks over all the child rootboxes, and ask them to
cleanup. of these has the kids of the RenderText, which are already
deleted. Boom.

Now, it sure seems to me like (3) should update the parent InlineFlowBox
in this context. However, deleteInlineBoxes() should not do it when called
from detach, and it seems like the RenderLine::remove() ->
InlineFlowBox::removeFromLine() could be problematic, since it dirties the
parent as well.

Suggestions?

Thanks,
Maks


P.S. VG log, for more context (thanks to dfaure):
==10501== Invalid read of size 8
==10501== at 0x17F51340: khtml::InlineBox::parent() const
(render_line.h:96)
==10501== by 0x17FE2F75:
(khtml::RenderArena*)
(render_line.cpp:204)
==10501== by 0x17F4E8B0:
(bool, khtml::BidiIterator&,
khtml::BidiState&) (bidi.cpp:1733)
==10501== by 0x17F50052:
(bool, int) (bidi.cpp:1467)
==10501== by 0x17F5F130: (bool)
(render_block.cpp:782)
==10501== by 0x17F5FA09: khtml::RenderBlock::layout()
(render_block.cpp:685)
==10501== by 0x17FDBAF4: khtml::RenderBody::layout() (render_body.cpp:96)
==10501== by 0x17E137D6: khtml::R::layoutIfNeeded()
(render_object.h:455)
==10501== by 0x17F5E5C8: (bool)
(render_block.cpp:1505)
==10501== by 0x17F5F145: (bool)
(render_block.cpp:784)
==10501== by 0x17F5FA09: khtml::RenderBlock::layout()
(render_block.cpp:685)
==10501== by 0x17E137D6: khtml::R::layoutIfNeeded()
(render_object.h:455)
==10501== by 0x17F5E5C8: (bool)
(render_block.cpp:1505)
==10501== by 0x17F5F145: (bool)
(render_block.cpp:784)
==10501== by 0x17FD5C53: khtml::RenderCanvas::layout()
(render_canvas.cpp:187)
==10501== Address 0x1C705458 is 72 bytes inside a block of size 112 free'd
==10501== at 0x4C2182B: free (vg_replace_malloc.c:233)
==10501== by 0x17F98642: khtml::RenderArena::free(unsigned long,
void*) (render_arena.cpp:122)
==10501== by 0x17F9802D:
khtml::InlineTextBox::detach(khtml::RenderArena*, bool)
(render_text.cpp:82)
==10501== by 0x17F939D6:
(khtml::RenderArena*)
(render_text.cpp:780)
==10501== by 0x17F958D7: (bool,
bool) (render_text.cpp:789)
==10501== by 0x17F4FF14:
(bool, int) (bidi.cpp:1443)
==10501== by 0x17F5F130: (bool)
(render_block.cpp:782)
==10501== by 0x17F5FA09: khtml::RenderBlock::layout()
(render_block.cpp:685)
==10501== by 0x17FDBAF4: khtml::RenderBody::layout() (render_body.cpp:96)
==10501== by 0x17E137D6: khtml::R::layoutIfNeeded()
(render_object.h:455)
==10501== by 0x17F5E5C8: (bool)
(render_block.cpp:1505)
==10501== by 0x17F5F145: (bool)
(render_block.cpp:784)
==10501== by 0x17F5FA09: khtml::RenderBlock::layout()
(render_block.cpp:685)
==10501== by 0x17E137D6: khtml::R::layoutIfNeeded()
(render_object.h:455)
==10501== by 0x17F5E5C8: (bool)
(render_block.cpp:1505)
==10501== by 0x17F5F145: (bool)
(render_block.cpp:784)
==10501== by 0x17FD5C53: khtml::RenderCanvas::layout()
(render_canvas.cpp:187)
==10501== by 0x17E10022: KHTMLView::layout() (khtmlview.cpp:966)

Le samedi 28 juin 2008, Maksim a :
Hi I've looked a bit at #162564, and I can see why it crashes, but I am
a bit at loss about how it's intended to work/how to fix it. The
redundancy between the two trees is quite confusing.

I have problem reproducing the gmail testcase is that with the full
interface/ua spoofing?

>
The crash scenario here is this:
(1) is called with fullLayout set.
(2) It calls dirtyInlineBoxes(fullLayout) on a RenderText kid, which in
turn calls deleteInlineBoxes (since fullLayout is true)
(3) that deletes all the box kids of the object, and sets the
m_firstTextBox/m_lastTextBox of the RenderText to 0. However, the parent
inline box is not touched at all.
(4) Later on, layoutInlineChildren calls determineStartPosition. Since
fullLayout is true, it walks over all the child rootboxes, and ask them to
cleanup. of these has the kids of the RenderText, which are already
deleted. Boom.

's cleanup is a fast cleanup : it doesn't try
to carefully remove the line boxes, or to climb to the parents, because it is
happening from the block level, so the inline boxes are supposed to be
parent-less (root). This should even be asserted somewhere.

Careful removal with consistency checks, is for dom manipulations.

So, AFAICS, this crash indicates some inline rendering elements have been
moved directly to the root of a block without using the dom, and forgoting to
cleanup the lineboxes before. e.g. anonymous block cleanups, or other
pseudo-elements surgery.

I'll have a look as soon as I can reproduce this,
Germain

Le samedi 28 juin 2008, Maksim a :
>Hi I've looked a bit at #162564, and I can see why it crashes, but I
>am
>a bit at loss about how it's intended to work/how to fix it. The
>redundancy between the two trees is quite confusing.
>
I have problem reproducing the gmail testcase is that with the full
interface/ua spoofing?

Yeah, fancy UI, version 1, I think. Anyway, thanks for the explanation,
will need some thought to digest it, though.