[webkit-dev] Crash in RenderLayer related to setStyle() - Questions
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? -- --Alex Milowski "The excellence of grammar as a guide is proportional to the paucity of the inflexions, i.e. to the degree of analysis effected by the language considered." Bertrand Russell in a footnote of Principles of Mathematics ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Crash in RenderLayer related to setStyle() - Questions
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
Re: [webkit-dev] Crash in RenderLayer related to setStyle() - Questions
Coincidentally, http://webkit.org/coding/dom-element-attach.html :DG< On Sat, Jul 24, 2010 at 9:53 AM, Dan Bernstein 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@lists.webkit.org > http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev > ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev