205282 – Debug assert in computeFloatVisibleRectInContainer on non-SVG object parent

RESOLVED FIXED 205282

Debug assert in computeFloatVisibleRectInContainer on non-SVG object parent

https://bugs.webkit.org/show_bug.cgi?id=205282

Summary Debug assert in computeFloatVisibleRectInContainer on non-SVG object parent

Sunny He

Reported 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.

Attachments
Patch (1.98 KB, patch) 2019-12-16 10:23 PST, Sunny He	no flags	Details Formatted Diff Diff
Patch (2.16 KB, patch) 2019-12-16 11:31 PST, Sunny He	no flags	Details Formatted Diff Diff
Patch (2.15 KB, patch) 2019-12-16 13:40 PST, Sunny He	no flags	Details Formatted Diff Diff
Patch (3.92 KB, patch) 2019-12-17 12:16 PST, Sunny He	no flags	Details Formatted Diff Diff
Patch (4.01 KB, patch) 2019-12-17 14:28 PST, Sunny He	no flags	Details Formatted Diff Diff
Patch (4.50 KB, patch) 2019-12-18 16:10 PST, Sunny He	no flags	Details Formatted Diff Diff
Patch (4.62 KB, patch) 2020-01-06 11:23 PST, Sunny He	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2019-12-16 10:20:17 PST

<rdar://problem/57975185>

Sunny He

Comment 2 2019-12-16 10:23:03 PST

Created attachment 385780 [details] Patch

Said Abou-Hallawa

Comment 3 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?

Sunny He

Comment 4 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.

Said Abou-Hallawa

Comment 5 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>

Sunny He

Comment 6 2019-12-16 11:31:09 PST

Created attachment 385791 [details] Patch

Sunny He

Comment 7 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.

Sunny He

Comment 8 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.

Ryosuke Niwa

Comment 9 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.

Sunny He

Comment 10 2019-12-16 13:40:40 PST

Created attachment 385805 [details] Patch

Ryosuke Niwa

Comment 11 2019-12-16 13:41:19 PST

Let's wait for EWS.

Sunny He

Comment 12 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.

Ryosuke Niwa

Comment 13 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.

Sunny He

Comment 14 2019-12-17 12:16:40 PST

Created attachment 385907 [details] Patch

alan

Comment 15 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.

Sunny He

Comment 16 2019-12-17 14:28:14 PST

Created attachment 385920 [details] Patch

alan

Comment 17 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)

Sunny He

Comment 18 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.

alan

Comment 19 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()).

Said Abou-Hallawa

Comment 20 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();

Sunny He

Comment 21 2019-12-18 16:10:34 PST

Created attachment 386028 [details] Patch

Darin Adler

Comment 22 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.

Sunny He

Comment 23 2020-01-06 11:23:01 PST

Created attachment 386867 [details] Patch

WebKit Commit Bot

Comment 24 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>

WebKit Commit Bot

Comment 25 2020-01-13 15:06:37 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component Layout and Rendering

Assignee

Nobody

Reported

2019-12-16 10:19 PST

Modified

2020-01-13 15:06 PST History

CC List

19 users Show

URL

Keywords InRadar

Depends on

Blocks