Bug 205282

Summary: Debug assert in computeFloatVisibleRectInContainer on non-SVG object parent
Product: WebKit Reporter: Sunny He <sunny_he>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, darin, dino, esprehn+autocc, ews-watchlist, fmalita, glenn, gyuyoung.kim, kondapallykalyan, pdr, rniwa, sabouhallawa, schenney, sergio, simon.fraser, webkit-bug-importer, wenson_hsieh, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Sunny He 2019-12-16 10:19:33 PST 
The test that was supposed to be committed alongside the change in bug 205217 somehow wasn't in the final committed patch.
Comment 1 Radar WebKit Bug Importer 2019-12-16 10:20:17 PST 
<rdar://problem/57975185>
Comment 2 Sunny He 2019-12-16 10:23:03 PST 
Created attachment 385780 [details]
Patch
Comment 3 Said Abou-Hallawa 2019-12-16 11:07:15 PST 
Comment on attachment 385780 [details]
Patch

Are you sure this test case crashes without your change? It opens fine in MiniBrowser - debug without your change. run-webkit-tests also passes it if add it to LayoutTests/svg/dom/. How can I reproduce the crash without your change?
Comment 4 Sunny He 2019-12-16 11:18:52 PST 
Looks like adding the document.open() to write the success message makes this stop reproing. Let me take another look at this.
Comment 5 Said Abou-Hallawa 2019-12-16 11:26:37 PST 
You can add the confirming text before the <svg> element. Something like this:

<body>
    <p>Confirm that svg element with document as parent is handled without crashing</p>
    <svg onload="run()">
        <text id="svgvar"></text>
    </svg>
</body>
Comment 6 Sunny He 2019-12-16 11:31:09 PST 
Created attachment 385791 [details]
Patch
Comment 7 Sunny He 2019-12-16 11:54:16 PST 
(In reply to Said Abou-Hallawa from comment #5)
> You can add the confirming text before the <svg> element. Something like
> this:
> 
> <body>
>     <p>Confirm that svg element with document as parent is handled without
> crashing</p>
>     <svg onload="run()">
>         <text id="svgvar"></text>
>     </svg>
> </body>

Good idea, that's much better than messing with async stuff. Let me try that out.
Comment 8 Sunny He 2019-12-16 12:55:04 PST 
Unfortunately just inserting the message in the body doesn't produce output since document's child nodes are removed in the process of the test. So I guess we still have to do the document.open() business if we want there to be a non-blank test output.
Comment 9 Ryosuke Niwa 2019-12-16 13:33:20 PST 
Comment on attachment 385791 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385791&action=review

> LayoutTests/svg/dom/replaceChild-document-crash.html:21
> +        }, 100);

Does 0s timer work?

> LayoutTests/svg/dom/replaceChild-document-crash.html:22
> +        

Nit: whitespace.
Comment 10 Sunny He 2019-12-16 13:40:40 PST 
Created attachment 385805 [details]
Patch
Comment 11 Ryosuke Niwa 2019-12-16 13:41:19 PST 
Let's wait for EWS.
Comment 12 Sunny He 2019-12-16 17:43:16 PST 
The new test is hitting assert in debug build

Optional<FloatRect> RenderObject::computeFloatVisibleRectInContainer(const FloatRect&, const RenderLayerModelObject*, VisibleRectContext) const
{
    ASSERT_NOT_REACHED();
    return FloatRect();
}

Looks like this isn't as unreachable as thought.
Comment 13 Ryosuke Niwa 2019-12-16 20:34:50 PST 
(In reply to Sunny He from comment #12)
> The new test is hitting assert in debug build
> 
> Optional<FloatRect> RenderObject::computeFloatVisibleRectInContainer(const
> FloatRect&, const RenderLayerModelObject*, VisibleRectContext) const
> {
>     ASSERT_NOT_REACHED();
>     return FloatRect();
> }
> 
> Looks like this isn't as unreachable as thought.

Hm... maybe we should either remove that assertion or have a fix for it.
Comment 14 Sunny He 2019-12-17 12:16:40 PST 
Created attachment 385907 [details]
Patch
Comment 15 zalan 2019-12-17 12:32:48 PST 
Comment on attachment 385907 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385907&action=review

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:77
> +    return (is<RenderSVGRoot>(parent)) ? parent.computeFloatVisibleRectInContainer(adjustedRect, container, context) : FloatRect();

I think you want to check if the parent is an svg object and not that it's the root.
Comment 16 Sunny He 2019-12-17 14:28:14 PST 
Created attachment 385920 [details]
Patch
Comment 17 zalan 2019-12-17 14:30:59 PST 
Comment on attachment 385920 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385920&action=review

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:79
> +        return FloatRect();

I think { } is more appropriate here (nullopt vs. empty rect)
Comment 18 Sunny He 2019-12-18 10:20:46 PST 
(In reply to zalan from comment #17)
> Comment on attachment 385920 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385920&action=review
> 
> > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:79
> > +        return FloatRect();
> 
> I think { } is more appropriate here (nullopt vs. empty rect)

Using nullopt results in crash when RenderObject::computeFloatRectForRepaint tries to deref the returned value.
Comment 19 zalan 2019-12-18 10:26:19 PST 
(In reply to Sunny He from comment #18)
> (In reply to zalan from comment #17)
> > Comment on attachment 385920 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=385920&action=review
> > 
> > > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:79
> > > +        return FloatRect();
> > 
> > I think { } is more appropriate here (nullopt vs. empty rect)
> 
> Using nullopt results in crash when RenderObject::computeFloatRectForRepaint
> tries to deref the returned value.
That's very sad (computeFloatRectForRepaint shouldn't just blindly deref that optional FloatRect). Let's just go with your original patch for now (return FloatRect()).
Comment 20 Said Abou-Hallawa 2019-12-18 13:22:27 PST 
Comment on attachment 385920 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385920&action=review

>>>> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:79
>>>> +    auto& parent = *renderer.parent();
>>>> +    if (!parent.element() || !parent.element()->isSVGElement())
>>>> +        return FloatRect();
>>> 
>>> I think { } is more appropriate here (nullopt vs. empty rect)
>> 
>> Using nullopt results in crash when RenderObject::computeFloatRectForRepaint tries to deref the returned value.
> 
> That's very sad (computeFloatRectForRepaint shouldn't just blindly deref that optional FloatRect). Let's just go with your original patch for now (return FloatRect()).

This check should be moved before calculating the 'adjustedRect'. Also you can add this assertion which is similar to what localToParentTransform() does:

ASSERT(renderer.parent());
auto& parent = *renderer.parent();
Comment 21 Sunny He 2019-12-18 16:10:34 PST 
Created attachment 386028 [details]
Patch
Comment 22 Darin Adler 2020-01-03 19:59:43 PST 
Comment on attachment 386028 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386028&action=review

> Source/WebCore/ChangeLog:3
> +        Fix missing tests from bug 205217

I don’t understand this bug title. It doesn’t "fix missing tests"; it seems to fix a bug and add a test.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:70
> +    if (!parent.element() || !parent.element()->isSVGElement())

Should write:

    if (!is<SVGElement>(parent.element()))

That combines the null check with the type check.
Comment 23 Sunny He 2020-01-06 11:23:01 PST 
Created attachment 386867 [details]
Patch
Comment 24 WebKit Commit Bot 2020-01-13 15:06:34 PST 
Comment on attachment 386867 [details]
Patch

Clearing flags on attachment: 386867

Committed r254458: <https://trac.webkit.org/changeset/254458>
Comment 25 WebKit Commit Bot 2020-01-13 15:06:37 PST 
All reviewed patches have been landed.  Closing bug.