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

[Dimitri Glazkov]

Coincidentally, http://webkit.org/coding/dom-element-attach.html

:DG<

On Sat, Jul 24, 2010 at 9:53 AM, Dan Bernstein <mitz at apple.com> wrote:
>
> 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).
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
>