Bug 33266 – WebCore::InlineFlowBox::determineSpacingForFlowBoxes ReadAV@NULL (43c64e8abbda6766e5f5edbd254c2d57)

• Component: DOM
• Status: RESOLVED FIXED

[Berend-Jan Wever]

Created attachment 45981 [details]
Fancy HTML version of cdb debugger output for Chromium

Attempt to read from NULL pointer (+0x34) in WebCore::InlineFlowBox::determineSpacingForFlowBoxes
Repro:
<ruby>><table><rt>

[Roland Steiner]

Created attachment 46654 [details]
patch - handle all combinations of inline and block flow when splitting and merging ruby bases

As described in the ChangeLog, the ruby code so far did not anticipate block flow in ruby bases. This caused an illegal type cast and subsequently the crash.

The new code covers all combinations when splitting and merging ruby bases.

[WebKit Review Bot]

Attachment 46654 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/rendering/RenderRubyBase.cpp:87:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/rendering/RenderRubyBase.cpp:174:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 2

[Roland Steiner]

Purple mac-ews reminds me that I still need to bother someone about my @chromium.org account access for BugZilla... :p

[Roland Steiner]

Created attachment 46665 [details]
patch - cleaned up version of previous patch, even more layout tests

Cleaned up version of the previous patch. Also adds a line that was missing in the previous patch and removes an incorrect ASSERT.
Adds 6 more layout tests (14 in total) in order to try to even better cover all possibilities.

[WebKit Review Bot]

Attachment 46665 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/rendering/RenderRubyBase.cpp:100:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/rendering/RenderRubyBase.cpp:174:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 2

[Eric Seidel (no email)]

A purple mac ews means that your bugzilla account is not listed in committers.py:
http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/committers.py

Just change that string to an array of strings, one of which is your bugzilla (@google) address, and it will work fine.

You can also change this account's email address to be @chromium in https://bugs.webkit.org/userprefs.cgi?tab=account

Comment on attachment 46665 [details]
patch - cleaned up version of previous patch, even more layout tests

This looks okay.

> +        Ruby did not handle mal-formed cases correctly when the ruby base was in

Typo. Should be “malformed”.

> +void RenderBlock::moveAllChildrenTo(RenderObject* to, RenderObjectChildList* toChildList)
> +{
> +    for (RenderObject* child = children()->firstChild(); child; child = children()->firstChild())

Existing loops that iterate over children while removing them store the pointer to the current child’s next sibling before moving it, then make the stored sibling the current child. I wonder if that’s more efficient that calling children()->firstChild().

> +    // RenderRubyBase needs to do some complex block manipulation.
> +    friend class RenderRubyBase;

I wish this could be avoided. And the comment isn’t helpful.

> +    if (childrenInline())
> +        moveChildrenFromInlineFlow(toBase, beforeChild);
> +    else
> +        moveChildrenFromBlockFlow(toBase, beforeChild);

Following the naming scheme used in RenderBlock, I would name these methods moveInlineChildren() and moveBlockChildren(). This is always a block flow.

> +        // If toBase has a suitable block, we re-use it, otherwise create a new one.
> +        RenderBlock* block;
> +        RenderObject* lastChild = toBase->lastChild();
> +        if (lastChild && lastChild->isAnonymousBlock() && lastChild->childrenInline()) {
> +            block = toRenderBlock(lastChild);
> +        } else {

Braces around a single-line clause.

Should you check that !lastChild->parent()->createsAnonymousWrapper()? Can you assert that?

> +        for (RenderObject* child = firstChild(); child != beforeChild; child = firstChild())
> +            moveChildTo(block, block->children(), child);

Perhaps you can unify this loop with the similar loop at the end of the “if” clause.

> +    if (beforeChild != firstChild()) {

You can rewrite this with an early return.

> +        if (firstChildHere && firstChildHere->isAnonymousBlock() && firstChildHere->childrenInline() && 
> +            lastChildThere && lastChildThere->isAnonymousBlock() && lastChildThere->childrenInline() ) {

Please move the && to the beginning of the second line, and remove the space before the last parenthesis.

> +            for (RenderObject* child = anonBlockHere->firstChild(); child; child = anonBlockHere->firstChild())
> +                anonBlockHere->moveChildTo(anonBlockThere, anonBlockThere->children(), child);

This looks like moveAllChildrenTo().

r- because of the createsAnonymousWrapper() question, function naming (using “block flow” and “inline flow” instead of “block children” and “inline children”) and the style issues.

[Roland Steiner]

Created attachment 46898 [details]
patch - addressing the review remarks

Changed the patch as requested, in particular:

-) Changed moveAllChildrenTo() to use original coding with 2 variables. I don't think there'd be any speed difference, but then again safe is safe... ;)

-) Renamed methods as suggested, refactored moveInlineChildren() as suggested

-) Changed method signatures to use RenderRubyBase* instead of the more generic RenderBlock*. This should also address the question about createsAnonymousWrapper (?).

-) about braces around a single line clause: ISTR there was a discussion on webkit-dev@ that that's ok if it's consistent between the if and the else clause (?). But I'm not religious about such things, so I removed them anyway.

[Roland Steiner]

Addendum: 

I'm also not too happy introducing a 'friend' clause into RenderBlock, but that was the smallest and least intrusive change I could think of to pacify the compiler about those "anonBlock->..." operations.

Making moveChildTo, moveAllChildrenTo and makeChildrenNonInline public is probably a no-no.

So the remaining option would be to make makeChildrenNonInline protected and add a clearAnonymousBlock() method in RenderBlock, or - perhaps better - add a RenderAnonymousBlock class that contains this and is used for anonymous block wrappers in general (which would be a good idea IMHO).

(Any other possibilities I'm missing?)

[Roland Steiner]

Patch landed as rev. 53525.

[Roland Steiner]

*** Bug 33760 has been marked as a duplicate of this bug. ***