875175 - Stop continuous reflow and invalidation of entire SVG when an element that is partially/wholly outside the viewport is subject to a transform animation

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

In bug 852588 I've been seeing some apparently random invalidation. It turn out that at least one of the things I'm seeing is continuous reflow and invalidation of the _entire_ SVG tree when any element that is subject to a transform change (in the case of that bug, an animation) and that element is partially or entirely outside the viewport.

What seems to be happening is this:

When the SVG gets its initial reflow, nsSVGOuterSVGFrame::Reflow sets its overflow rects (via FinishAndStoreOverflow) without any regard to the overflow rects of its children. This makes sense for root-<svg> because it's not possible to scroll outside the bounds of the nsSVGOuterSVGFrame. It also currently makes sense for non-root-outer-<svg>, since we don't yet support |overflow:visible| on non-root-outer-<svg>.

Now, when the transform on one of its children changes, we end up calling UpdateOverflow on that child, and calling it up the frame tree until we get to an ancestor who's new overflow rects are the same as its old overflow rects. It's when we hit the nsSVGOuterSVGFrame that things go wrong. We would expect the UpdateOverflow calls to stop here, but instead of returning false, the UpdateOverflow call on nsSVGOuterSVGFrame returns true. This is because it (nsFrame::UpdateOverflow) passes the result from nsLayoutUtils::UnionChildOverflow to FinishAndStoreOverflow, not the content area of the nsSVGOuterSVGFrame, which results in different overflow rects to those produced by nsSVGOuterSVGFrame::Reflow. The UpdateOverflow calls then continues all the way up until we reach nsGfxScrollFrameInner::UpdateOverflow where FrameNeedsReflow is called passing NS_FRAME_IS_DIRTY.

The result of scheduling this dirty reflow is that we end up back inside nsSVGOuterSVGFrame::Reflow where, since this is a NS_FRAME_IS_DIRTY reflow, we end up calling ReflowSVG over all descendant frames and invalidating them. Worse, since nsSVGOuterSVGFrame::Reflow resets the overflow rects back to the values that don't take account of the child frames, the next time the transform changes on the partially/wholly out of viewport element we're going to do it all over again.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So this will cause ApplyOverflowClipping to do extra clipping work, and it might also break some 3-D stuff... at least based on a quick look at other callers of ShouldApplyOverflowClipping.

Have you checked what other callers of SetOverflowAreasToDesiredBounds use to make UpdateOverflow work right?

(sorry, interruption now, maybe more later)

Comment 3

[Jonathan Watt [:jwatt]]

Hmm, yeah. I think the correct thing to do here is to override UpdateOverflow then, and make sure that both Reflow and UpdateOverflow do the same thing.

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #2)
> Have you checked what other callers of SetOverflowAreasToDesiredBounds use
> to make UpdateOverflow work right?

Actually, a lot of the Reflow implementations that call SetOverflowAreasToDesiredBounds look wrong. It seems that for a good number of them Reflow and UpdateOverflow can produce different overflow rects. I'll file a bug on that shortly.

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: patch

this time with the required qrefresh...

Comment 6

[Jonathan Watt [:jwatt]]

Passed Try: https://tbpl.mozilla.org/?tree=Try&rev=e34d308a19ec

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 753523 [details] [diff] [review]
patch

I assume non-root SVG elements really do draw outside the frame bounds sometimes.  This review is based on the assumption that that's the case; if not, please ping me again.

>+    nsSVGOuterSVGAnonChildFrame *anonKid =
>+      static_cast<nsSVGOuterSVGAnonChildFrame*>(GetFirstPrincipalChild());
>+    overflowAreas.VisualOverflow().UnionRect(
>+      overflowAreas.VisualOverflow(),
>+      anonKid->GetVisualOverflowRect() + anonKid->GetPosition());

Probably no need to bother casting anonKid here; it could just be nsIFrame *anonKid = GetFirstPrincipalChild(), since you're not using any non-nsIFrame methods.

r=dbaron

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, is there something that ensures that root SVG elements don't draw outside their rect (i.e., by clipping them)?  Maybe add comments pointing to that code?  Or, if not... what prevents them from doing so such that you'd need to keep the visual overflow up-to-date?

Comment 9

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ba223470c83

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #7)
> I assume non-root SVG elements really do draw outside the frame bounds
> sometimes.

They should do, and just as soon as I get review for bug 378923, they will.

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #8)
> Also, is there something that ensures that root SVG elements don't draw
> outside their rect (i.e., by clipping them)?

Yes, the DisplayListClipState::AutoClipContainingBlockDescendantsToContentBox in DisplayBorderBackgroundOutline. I've added a comment to the patch pointing to that.

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9ba223470c83