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@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev