Focus Crash Relating to MathML
If anyone has any ideas of this bug: https://bugs.webkit.org/show_bug.cgi?id=47745 I'd appreciate the help. Basically, when the MathML is wrapped in an anchor parent, the focus event causes a crash at: ASSERTION FAILED: !renderer()->needsLayout() (/Users/alex/workspace/WebKit/WebCore/dom/Node.cpp:793 virtual bool WebCore::Node::isFocusable() const) The node in question is the anchor itself. It is marked as needed layout, which I presume is coming from the MathML rendering objects, but all the children rendering objects are marked as not needing layout. If you try out the example attached to the bug, just click on the tiny "x" in the example. That will cause the crash. My guess is that this is related to a larger issue of layout not propagating correctly with MathML. In certain situations, I've seen MathML layout overflow containers and then a simple window resize or similar user action will cause everything, including the MathML, to reflow correctly. I haven't narrowed down specific cases of this problem quite yet. -- --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
On Sat, Oct 16, 2010 at 3:38 PM, Alex Milowski <alex@milowski.org> wrote:
If anyone has any ideas of this bug:
Even more curious is that I just noticed the crash only happens with a debug build. -- --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
18.10.2010, в 12:33, Alex Milowski написал(а):
Even more curious is that I just noticed the crash only happens with a debug build.
The crash is an assertion failure, so it's not surprising that it only occurs in debug builds. Assertions aren't compiled into release builds. Sometimes, looking at svn blame to see when the assertion was added help one figure out what it is about. In general, asking a renderer-related question like isFocusable() needs finished layout indeed - e.g. display:none makes a node unfocusable, but before layout (recalcStyle?), the renderer doesn't know that. - WBR, Alexey Proskuryakov
On Mon, Oct 18, 2010 at 12:48 PM, Alexey Proskuryakov <ap@webkit.org> wrote:
18.10.2010, в 12:33, Alex Milowski написал(а):
Even more curious is that I just noticed the crash only happens with a debug build.
The crash is an assertion failure, so it's not surprising that it only occurs in debug builds. Assertions aren't compiled into release builds.
OK. Less curious now! :)
Sometimes, looking at svn blame to see when the assertion was added help one figure out what it is about. In general, asking a renderer-related question like isFocusable() needs finished layout indeed - e.g. display:none makes a node unfocusable, but before layout (recalcStyle?), the renderer doesn't know that.
Yes, that makes sense. This has something to do with the inline flow. The whole problem goes away if you change the display property of the anchor to block. All the children are marked as not needing layout but the parent (the anchor) has m_normalChildNeedsLayout set to true. If you remove the MathML math element, everything works as expected. -- --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
Most of the MathML rendering objects have a display style property value of inline-block. Whenever these rendering objects are used, somehow the parent "container" gets marked as having children in need of layout. The MathML math rendering object completes its layout and marks itself as not needing layout. In the end, the container (e.g. the anchor element) render object has itself in a state where m_normalChildNeedsLayout is true but no child is marked as needing layout. I've gone through the MathML rendering objects and remove all uses of markContaingBlocksForLayout() and setNeedsLayoutPrefWidthsRecalc() which generally cause the container to be marked with a child needing layout. These calls were unnecessary and the resulting code should be more efficient. In situations where the MathML does not contain a rendering object that is an inline-block, everything works fine. For example: <a href='#'> <math xmlns='http://www.w3.org/1998/Math/MathML'><mi>x</mi></math> </a> Keep in mind, in the above, the 'mi' element just uses RenderInline as it has no special semantics as of yet. In cases where specialized render objects (typically with display inline-block) are used (e.g. an operator), the assert fires: <a href='#'> <math xmlns='http://www.w3.org/1998/Math/MathML'><mo>x</mo></math> </a> At this point, I don't think my code is directly causing the anchor to get marked with a child needing layout. I do rely on RenderBlock::layout() within most of the rendering objects to handle the actual layout after adjustments. I've tried making sure that the parent or container schedule a re-layout but that hasn't really helped. You can see all these adjustments and optimizations in the patch for: https://bugs.webkit.org/show_bug.cgi?id=43462 Any ideas of what to look at next would be appreciated. -- --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
On Mon, Oct 18, 2010 at 8:22 PM, Alex Milowski <alex@milowski.org> wrote:
In cases where specialized render objects (typically with display inline-block) are used (e.g. an operator), the assert fires:
<a href='#'> <math xmlns='http://www.w3.org/1998/Math/MathML'><mo>x</mo></math> </a>
The culprit in this particular case turns out to be a call to destroyLeftoverChildren() during the layout. The RenderMathMLOperator class potentially re-stacks the glyphs and that causes the children to be destroyed. During that process, the container ancestors are marked as having a child needing layout. At the end of the ancestor's layout() method, the MathML rendering objects have all sorted themselves out and no longer need layout. The result is that the tree is inconsistent. If the ancestors can easily be marked as having a child needing layout during the descendant's layout() process, shouldn't each ancestor check the consistency between if m_normalChildNeedsLayout and their actual children before leaving layout()? -- --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
(1) Make sure any layout methods you call do setNeedsLayout(false) at the end of them. (2) Look for any early returns in any of your layout methods, since maybe you did an early return causing the setNeedsLayout(false) to be missed. (3) Make sure you aren't dirtying a child for a re-layout without immediately doing that re-layout, e.g., don't call setChildNeedsLayout(true, false) on some child and then bail without doing a layout. dave On Oct 18, 2010, at 10:22 PM, Alex Milowski wrote:
Most of the MathML rendering objects have a display style property value of inline-block. Whenever these rendering objects are used, somehow the parent "container" gets marked as having children in need of layout. The MathML math rendering object completes its layout and marks itself as not needing layout. In the end, the container (e.g. the anchor element) render object has itself in a state where m_normalChildNeedsLayout is true but no child is marked as needing layout.
I've gone through the MathML rendering objects and remove all uses of markContaingBlocksForLayout() and setNeedsLayoutPrefWidthsRecalc() which generally cause the container to be marked with a child needing layout. These calls were unnecessary and the resulting code should be more efficient.
In situations where the MathML does not contain a rendering object that is an inline-block, everything works fine. For example:
<a href='#'> <math xmlns='http://www.w3.org/1998/Math/MathML'><mi>x</mi></math> </a>
Keep in mind, in the above, the 'mi' element just uses RenderInline as it has no special semantics as of yet.
In cases where specialized render objects (typically with display inline-block) are used (e.g. an operator), the assert fires:
<a href='#'> <math xmlns='http://www.w3.org/1998/Math/MathML'><mo>x</mo></math> </a>
At this point, I don't think my code is directly causing the anchor to get marked with a child needing layout. I do rely on RenderBlock::layout() within most of the rendering objects to handle the actual layout after adjustments.
I've tried making sure that the parent or container schedule a re-layout but that hasn't really helped.
You can see all these adjustments and optimizations in the patch for:
https://bugs.webkit.org/show_bug.cgi?id=43462
Any ideas of what to look at next would be appreciated.
-- --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
On Tue, Oct 19, 2010 at 11:29 AM, David Hyatt <hyatt@apple.com> wrote:
(1) Make sure any layout methods you call do setNeedsLayout(false) at the end of them. (2) Look for any early returns in any of your layout methods, since maybe you did an early return causing the setNeedsLayout(false) to be missed. (3) Make sure you aren't dirtying a child for a re-layout without immediately doing that re-layout, e.g., don't call setChildNeedsLayout(true, false) on some child and then bail without doing a layout.
While this is helpful, the current code (in the patch) follows these principles (except when RenderBlock::layout() is called last and so setNeedsLayout(false) is already done). The problem I have is an *ancestor* is marked as having a child needing layout during the layout process. When then MathML layout finishes, the MathML rendering objects do not need layout but the parent is marked with m_normalChildNeedsLayout set to true. This only becomes a problem when the parent of the RenderMathMLMath rendering object is a RenderInline instance as the a RenderBlock will call setNeedsLayout(false) on itself at the very end of layout. To me, although I have yet to confirm this, it seems like setNeedsLayout(false) is called during the layout of the inline flow from RenderBlock::layoutInlineChildren() on the RenderInline instance and then the RenderInline is marked with a child needing layout. Unfortunately, none of the above suggestions are going to fix that. I think the call to destroyLeftoverChildren() is also something we should reconsider. In my very simple example, this is what is causing the RenderInline instance to be marked with a child needing layout as it causes a traversal through the ancestors. I know why it is there but it doesn't necessarily seem like the right way (or place) to reorganize the operator stacking. -- --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
On Oct 19, 2010, at 2:07 PM, Alex Milowski wrote:
On Tue, Oct 19, 2010 at 11:29 AM, David Hyatt <hyatt@apple.com> wrote:
(1) Make sure any layout methods you call do setNeedsLayout(false) at the end of them. (2) Look for any early returns in any of your layout methods, since maybe you did an early return causing the setNeedsLayout(false) to be missed. (3) Make sure you aren't dirtying a child for a re-layout without immediately doing that re-layout, e.g., don't call setChildNeedsLayout(true, false) on some child and then bail without doing a layout.
While this is helpful, the current code (in the patch) follows these principles (except when RenderBlock::layout() is called last and so setNeedsLayout(false) is already done). The problem I have is an *ancestor* is marked as having a child needing layout during the layout process. When then MathML layout finishes, the MathML rendering objects do not need layout but the parent is marked with m_normalChildNeedsLayout set to true.
Ok, just speculating from eyeballing the code.... I think layoutInlineChildren should do setNeedsLayout(false) on inlines when the end of the inline is encountered rather than the start of it. The iteration order is start of inline -> contents of inline -> end of inline, and we do setNeedsLayout(false) at the start of the inline. If the contents of the inline end up causing a dirtying up the chain, then this will not be caught and cleared. else if (o->isText() || (o->isRenderInline() && !endOfInline)) { if (fullLayout || o->selfNeedsLayout()) dirtyLineBoxesForRenderer(o, fullLayout); o->setNeedsLayout(false); if (!o->isText()) toRenderInline(o)->invalidateVerticalPosition(); // FIXME: Should do better here and not always invalidate everything. } In that code above I think !endOfInline should maybe just become endOfInline instead. dave (hyatt@apple.com)
Also, if your pattern of code in a layout method is (1) Call base class RenderBlock::layout (2) Do other stuff that might cause dirtying Then you should really bulletproof that code by adding (1) Call base class RenderBlock::layout (2) Do a setChildNeedsLayout(true, false) on yourself just to make yourself already dirty. (3) Do other stuff that might cause dirtying (4) Do a setNeedsLayout(false) We don't really have a good setup for calling base class layout methods... technically you should stay dirty throughout the lifetime of your own layout method, but the base class method will mark you as "clean." We should come up with something better at some point, but for now I think if you just dirty for the rest of the code you want to run and then mark yourself clean at the end, you'd stop the problem as well. dave On Oct 19, 2010, at 2:07 PM, Alex Milowski wrote:
On Tue, Oct 19, 2010 at 11:29 AM, David Hyatt <hyatt@apple.com> wrote:
(1) Make sure any layout methods you call do setNeedsLayout(false) at the end of them. (2) Look for any early returns in any of your layout methods, since maybe you did an early return causing the setNeedsLayout(false) to be missed. (3) Make sure you aren't dirtying a child for a re-layout without immediately doing that re-layout, e.g., don't call setChildNeedsLayout(true, false) on some child and then bail without doing a layout.
While this is helpful, the current code (in the patch) follows these principles (except when RenderBlock::layout() is called last and so setNeedsLayout(false) is already done). The problem I have is an *ancestor* is marked as having a child needing layout during the layout process. When then MathML layout finishes, the MathML rendering objects do not need layout but the parent is marked with m_normalChildNeedsLayout set to true.
This only becomes a problem when the parent of the RenderMathMLMath rendering object is a RenderInline instance as the a RenderBlock will call setNeedsLayout(false) on itself at the very end of layout. To me, although I have yet to confirm this, it seems like setNeedsLayout(false) is called during the layout of the inline flow from RenderBlock::layoutInlineChildren() on the RenderInline instance and then the RenderInline is marked with a child needing layout. Unfortunately, none of the above suggestions are going to fix that.
I think the call to destroyLeftoverChildren() is also something we should reconsider. In my very simple example, this is what is causing the RenderInline instance to be marked with a child needing layout as it causes a traversal through the ancestors. I know why it is there but it doesn't necessarily seem like the right way (or place) to reorganize the operator stacking.
-- --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
On Tue, Oct 19, 2010 at 1:44 PM, David Hyatt <hyatt@apple.com> wrote:
Also, if your pattern of code in a layout method is
(1) Call base class RenderBlock::layout (2) Do other stuff that might cause dirtying
Then you should really bulletproof that code by adding
(1) Call base class RenderBlock::layout (2) Do a setChildNeedsLayout(true, false) on yourself just to make yourself already dirty. (3) Do other stuff that might cause dirtying (4) Do a setNeedsLayout(false)
We don't really have a good setup for calling base class layout methods... technically you should stay dirty throughout the lifetime of your own layout method, but the base class method will mark you as "clean." We should come up with something better at some point, but for now I think if you just dirty for the rest of the code you want to run and then mark yourself clean at the end, you'd stop the problem as well.
This all sounds good. I'm going to experiment a bit and see if there is a better solution than using destroyLeftoverChildren() in RenderMathMLOperator. That will probably solve my immediate problem. I'll also look into changing when setNeedsLayout(false) is called as you have described. I think that change would be good to make sure that inline contents can't leave the tree in a strange state as it is quite easy to cause ancestors to get marked with descendants needing layout when, at the end of the layout for the subtree, that is no longer true. -- --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
participants (3)
-
Alex Milowski
-
Alexey Proskuryakov
-
David Hyatt