The test that was supposed to be committed alongside the change in bug 205217 somehow wasn't in the final committed patch.
<rdar://problem/57975185>
Created attachment 385780 [details] Patch
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?
Looks like adding the document.open() to write the success message makes this stop reproing. Let me take another look at this.
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>
Created attachment 385791 [details] Patch
(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.
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 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.
Created attachment 385805 [details] Patch
Let's wait for EWS.
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.
(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.
Created attachment 385907 [details] Patch
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.
Created attachment 385920 [details] Patch
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)
(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.
(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 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();
Created attachment 386028 [details] Patch
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.
Created attachment 386867 [details] Patch
Comment on attachment 386867 [details] Patch Clearing flags on attachment: 386867 Committed r254458: <https://trac.webkit.org/changeset/254458>
All reviewed patches have been landed. Closing bug.