Bug 26342 – Absolutely positioned HTML element within foreignObject of absolutely positioned SVG crashes Safari

• Component: SVG
• Status: RESOLVED FIXED

[Tim Barham]

If an XHTML page includes an absolutely position SVG element, and that SVG element contains a foreignObject element that contains XHTML with an absolutely positioned element, Safari will crash.

If either the SVG element or the HTML element is not absolutely positioned, it will not crash.

Repro steps:

See the referenced URL (http://www.barhams.info/webkit/crash.xml) for a repro example.

Repro'd under Windows Vista and Windows 7, Safari 3.2.3 and latest Safari 4 beta.

[Mark Rowe (bdash)]

<rdar://problem/6966731>

[Charles Wei]

Created attachment 39535 [details]
patch that fixes the crash problem with SVG foreignobject with absolute positioning

Fixes the crash of SVG

[George Staikos]

I think Dave should review this since it is his code that is being changed.  Also the commented out asserts should be removed altogether, and there is a spelling mistake in the first comment.

[Darin Adler]

Comment on attachment 39535 [details]
patch that fixes the crash problem with SVG foreignobject with absolute positioning

Thanks for tackling this!

> +        This is to fix the crash reported with
> +        https://bugs.webkit.org/show_bug.cgi?id=26342
> +
> +        Test case at : http://www.barhams.info/webkit/crash.xml

In the WebKit project we require regression tests for any bug fix. So this fix needs to include a test for the LayoutTests directory that demonstrates the crash and that it is fixed.

>      if (!o || !o->isRenderBlock())
> -        return 0; // Probably doesn't happen any more, but leave just in case. -dwh
> +        return document()->renderView(); // return the render view of the document, which is the top level containner

This comment doesn't indicate why it is correct and acceptable to return container.

Typo here: "containner"

WebKit style uses sentence style comments, with capital letters and periods.

> -    ASSERT(useTransforms); // mapping a point through SVG w/o respecting trasnforms is useless.
> +//    ASSERT(useTransforms); // mapping a point through SVG w/o respecting trasnforms is useless.

We don't include commented-out code in WebKit. If the assertion is wrong it should be removed, not commented out. Further, you don't explain why you are removing this assertion.

> -    ASSERT(useTransforms); // mapping a point through SVG w/o respecting trasnforms is useless.
> +//    ASSERT(useTransforms); // mapping a point through SVG w/o respecting trasnforms is useless.

Ditto.

[Nikolas Zimmermann]

Charles will come up with a new DRT-compatible testcase. Though I think the patch is not correct. I'm running out of time, I may elaborate later. CC'Ing Eric, as he's for sure the one most familiar with this, after all he rewrote this code.

But I still think the ASSERTIONS are existant for a reason. Maybe Eric can say a few words?

[Eric Seidel (no email)]

Yeah, this looks like the wrong fix.  If you're hitting those useTransforms ASSERTS then some other part of the code is wrong.  We need a test case.  Attaching a crashing stack trace to the bug would make it easier for me to comment w/o reproducing locally. :)

Does this crash in release builds to?  In which case, then it's a P1.  Reproducible crashers are P1.

CCing simon (who is the transforms guy) just so he can see this go by.

[Eric Seidel (no email)]

I can take this on if necessary.  I'm just a little busy today, but I'm happy to add this to my todo list if Charles or Niko aren't able to easily find a solution.

[Simon Fraser (smfr)]

Crash point:

#0  0x00000001014a5ef3 in WebCore::RenderBlock::insertPositionedObject (this=0x0, o=0x11c241328) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/rendering/RenderBlock.cpp:2217
#1  0x00000001014c2368 in WebCore::RenderBlock::layoutInlineChildren (this=0x11c240c38, relayoutChildren=true, repaintTop=@0x7fff5fbfca1c, repaintBottom=@0x7fff5fbfca18) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/rendering/RenderBlockLineLayout.cpp:858
#2  0x00000001014a8d69 in WebCore::RenderBlock::layoutBlock (this=0x11c240c38, relayoutChildren=true) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/rendering/RenderBlock.cpp:712
#3  0x00000001014a230f in WebCore::RenderBlock::layout (this=0x11c240c38) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/rendering/RenderBlock.cpp:638
#4  0x00000001014a7f4f in WebCore::RenderBlock::layoutBlockChild (this=0x11c240698, child=0x11c240c38, marginInfo=@0x7fff5fbfcc00, previousFloatBottom=@0x7fff5fbfcc84, maxFloatBottom=@0x7fff5fbfcda4) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/rendering/RenderBlock.cpp:1327
#5  0x00000001014a88ae in WebCore::RenderBlock::layoutBlockChildren (this=0x11c240698, relayoutChildren=true, maxFloatBottom=@0x7fff5fbfcda4) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/rendering/RenderBlock.cpp:1270
#6  0x00000001014a8d82 in WebCore::RenderBlock::layoutBlock (this=0x11c240698, relayoutChildren=true) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/rendering/RenderBlock.cpp:714
#7  0x00000001014a230f in WebCore::RenderBlock::layout (this=0x11c240698) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/rendering/RenderBlock.cpp:638
#8  0x00000001014ed65d in WebCore::RenderForeignObject::layout (this=0x11c240698) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/rendering/RenderForeignObject.cpp:103
#9  0x00000001014c406f in WebCore::RenderObject::layoutIfNeeded (this=0x11c240698) at RenderObject.h:487
#10 0x0000000101552034 in WebCore::RenderSVGRoot::layout (this=0x11c23e4f8) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/rendering/RenderSVGRoot.cpp:102
#11 0x00000001014c406f in WebCore::RenderObject::layoutIfNeeded (this=0x11c23e4f8) at RenderObject.h:487
#12 0x00000001014993c0 in WebCore::RenderBlock::layoutPositionedObjects (this=0x10612c088, relayoutChildren=true) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/rendering/

o->containingBlock() is null at:

                    o->containingBlock()->insertPositionedObject(box);

[Charles Wei]

Created attachment 39636 [details]
resubmit patch with test cases, and reverse un-related changes for ASSERT

 Resubmit the patch after addressing the comments from the reviewer:

1.  A test case added. 
2.  Typo in the comments corrected.
3.  Rolled back the change about ASSERT.   I found the ASSERT problem with some other pages, not related to this bug. I was thinking to fix it with this patch . Now I would rather submit the patch that only fixes this bug, and would like to submit another seperate bug to track the ASSERT problem .

[Charles Wei]

(In reply to comment #8)
> Crash point:
> 
> #0  0x00000001014a5ef3 in WebCore::RenderBlock::insertPositionedObject
> (this=0x0, o=0x11c241328) at
> /Volumes/InternalData/Development/webkit/OpenSource/WebCore/rendering/RenderBlock.cpp:2217
> #1  0x00000001014c2368 in WebCore::RenderBlock::layoutInlineChildren
> (this=0x11c240c38, relayoutChildren=true, repaintTop=@0x7fff5fbfca1c,
> repaintBottom=@0x7fff5fbfca18) at
> /Volumes/InternalData/Development/webkit/OpenSource/WebCore/rendering/RenderBlockLineLayout.cpp:858
.......
> 
> o->containingBlock() is null at:
> 
>                     o->containingBlock()->insertPositionedObject(box);


o->containingBlock() returns 0  is the root cause of the bug, and my patch fixes this. 

about crash problem caused by ASSERT(),  it's irrelevant to this bug, I discovered the ASSERT problem while verifying this bug, and was trying to fix it together with a patch.  Now I would rather to have another separate bug to track the ASSERT problem, and whould like to have the submitted patch to tackle this bug only. 

FYI -- The ASSERT problem can be reproduced with the following page, after this patch has been applied.

http://starkravingfinkle.org/blog/wp-content/uploads/2007/07/foreignobject-transform.svg

[Darin Adler]

Comment on attachment 39636 [details]
resubmit patch with test cases, and reverse un-related changes for ASSERT

I don't agree that returning the top level container instead of 0 when a render object is not in any containing block is the correct way to fix this bug. Changing the behavior of a fundamental low level function like containingBlock seems wrong unless the further repercussions of this for all other callers are considered.

I'd like to hear Hyatt's comments. A check for 0 at appropriate call sites seems like a better approach.

[Eric Seidel (no email)]

Comment on attachment 39636 [details]
resubmit patch with test cases, and reverse un-related changes for ASSERT

Yes, this is the wrong fix. :)

ForeignObjects are a RenderBlock, but they should know how to find their containing SVGRenderRoot, which is also a RenderBlock.  For SVG this is slightly tricky becuase SVG doesn't use the concept of "containingBlock" like the rest of the rendering tree does.  SVG cares about "nearestViewportElement" instead.   There is some missing logic in SVG here.  Whatever it is that thinks it wants the foreignObject's containing block might not actually.  Or if it does, it wants the containing SVGRenderRoot (which it should be able to find...).  The "return 0" that you're returning should really be:
ASSERT_NOT_REACHED();
return 0;

[Simon Fraser (smfr)]

What should an abs. positioned block inside an SVG <foreignobject> be positioned relative to?

[Eric Seidel (no email)]

(In reply to comment #13)
> What should an abs. positioned block inside an SVG <foreignobject> be
> positioned relative to?

I would suspect the <foreignObject>, since fO is effectively like an iframe.

[Charles Wei]

Created attachment 39760 [details]
make the foreignobject the containing block for the abs positioned child block if no appropriate containing block found

Following Eric's comment,  making the foreignobject the containing block for the abs. positioned children block .

[Eric Seidel (no email)]

Comment on attachment 39760 [details]
make the foreignobject the containing block for the abs positioned child block if no appropriate containing block found

I think that's still not quite right.  I think you want to change the while condition instead:

        while (o && (o->style()->position() == StaticPosition || (o->isInline() && !o->isReplaced())) && !o->isRenderView() && !(o->hasTransform() && o->isRenderBlock())) {

Admittedly, I think that condition would be much clearer written as a bunch of if statements in the while body, each calling "break" when true.

while (o)) {
  if (o->isRenderView())
      break;
  if (o->hasTransform() && o->isRenderBlock())
      break;
  if (o->style()->position() != StaticPosition && (o->isReplaced() && !o->isInline())
      break;
}

Not something you'd have to change, but looking at it, that loop is pretty ugly/hard-to-read.

o->node()->hasTagName(SVGNames::foreignObjectTag)
is probably a bad check too.  We should probably add a isForeignObject() to RenderObject instead.

Anyway, another way to add what you want would be to just add:
if (o->isForeignObject())
   break;
instead of needing to edit the existing if or while condition.

If this test is just trying to test the crash it can just be a dumpAsText() test.  If you need to test the positioning of the text, then you might need to make it render tree test as you have.

[Darin Adler]

(In reply to comment #16)
>         while (o && (o->style()->position() == StaticPosition || (o->isInline()
> && !o->isReplaced())) && !o->isRenderView() && !(o->hasTransform() &&
> o->isRenderBlock())) {
> 
> Admittedly, I think that condition would be much clearer written as a bunch of
> if statements in the while body

Another way to make it more readable would be to make it an inline helper function. The name of the function gives us a chance to document what's being checked too.

[Charles Wei]

Created attachment 39844 [details]
re-submission

re-submission of the patch .

1.  This patch focuses on the crash of the webkit,  simpifying the original "while" statement raised by Eric is not in the scope of this bug. I would rather leave that alone as is,  you can have a separate bug to track that change .

2.  o->node()->hasTagName(SVGNames::foreignObjectTag) replaced by RenderObject::isSVGForeignObject(), as suggested by Eric

3.  Test case as a dumpAsText() , following Eric's suggestion .

[Darin Adler]

Comment on attachment 39844 [details]
re-submission

> +    virtual bool isSVGForeignObject() const { return true; }

A function like this should be made private. If someone already has a RenderForeignObject* it's a programming mistake for them to call isSVGForeignObject and making it private will solve that problem.

Otherwise seems OK to make this change. I'll say r=me

[Eric Seidel (no email)]

Comment on attachment 39844 [details]
re-submission

Yes, this also looks good to me.  I would probably have made a comment next to the if (o->isSVGForeignObject()) call, noting that <foreignObject> acts as the containing block for its content.

I also probably would have made this message simpler:
+This test case is designed for bug 26342 at: https://bugs.webkit.org/show_bug.cgi?id=26342, which complains about webkit crash with this test case. If you see this text renders with webkit, the test case SUCCEEDS !

PASS - This did not crash.  https://bugs.webkit.org/show_bug.cgi?id=26342

Says the same thing your message says in half the words. :)

[Eric Seidel (no email)]

Comment on attachment 39844 [details]
re-submission

Given that Darin asked for a change (and I mentioned some nits as well) this can't be auto-committed w/o posting a new patch. Marking cq-.

[Charles Wei]

Created attachment 39976 [details]
resubmit after adding comments and test case description change

1. Add change new-added member function isSVGForeignObject() from virtual public to virtual private,  I don't in favor of that.  Please look at RenderObject , it has similiar functions , like isSVGRoot(), isSVGContainer(), isSVGText(), isSVGImage(), ... , all public virtual member functions.  I think we should be consistent with all other similiar functions .

2. Test case descriptions changed as suggested by Eric

3. Comments added at o->isForeignObject()   to indicate foreignobject is the containing block for its contents.

[Eric Seidel (no email)]

Darin's comment was that RenderForeignObject::isForeignObject() should be private. :)  But this still looks fine.

[WebKit Commit Bot]

Comment on attachment 39976 [details]
resubmit after adding comments and test case description change

Clearing flags on attachment: 39976

Committed r48668: <http://trac.webkit.org/changeset/48668>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.