[webkit-dev] Crash in RenderLayer related to setStyle() - Questions

[Dan Bernstein]

On Jul 23, 2010, at 3:48 PM, Alex Milowski wrote:

> I've identified a crash with the MathML implementation related to use of
> CSS style rules that cause a RenderLayer instance to be created.  In the
> MathML code's various createRenderer() methods, they always call
> RenderObject::setStyle() on the object they've just created.
> 
> When the setStyle() method is called, a style difference is calculated
> and propagated.  If you call setStyle() inside your createRenderer()
> method, you're calling it on an unattached RenderObject instance.
> 
> Further, if there happens to be a rule like:
> 
> math {
>   overflow: auto;
> }
> 
> you'll get a layer created by RenderBoxModelObject.  That layer
> will get the style change.
> 
> Right now, as the code stands, you'll get an exception and crash
> due to the fact that the RenderLayer code makes some assumptions
> about the RenderObject instance being attached to the tree.
> 
> The fix is trivial but my question is basically: what's the right thing
> to do here?  Should the setStyle() be called in createRenderer() ?  It
> seems like the answer is no because the style gets associated
> somewhere else.  If I remove the call, the crash is also fixed (without
> any change to RenderLayer) and the tests still all pass for MathML.
> 
> Further, should RenderLayer be made more safe?  Should it check
> for zero pointer values in a couple places and avoid this?
> 
> If we shouldn't be calling setStyle() in createRenderer(), then fixing
> RenderLayer would just hide that fact.  While an ASSERT wouldn't
> hide things, it would still only arise when a layer is created.
> 
> I know how to fix the MathML code and I just want to make
> sure I understand why calling setStyle() is "bad".
> 
> I'm not sure what should be done about RenderLayer or otherwise.
> 
> Suggestions?

Do not call setStyle() from createRenderer(). Node::createRendererIfNeeded() calls setAnimatableStyle() on the renderer after setting it as the node’s renderer, so there is no need for createRenderer() to call setStyle(); and as you saw, calling it before setting the node’s renderer violates the assumption that the pointers between node and renderer are in a consistent state (which is not to say that renderer->node->renderer is always the original renderer).