822378 - Update the overflow rects of the child frames when an nsSVGOuterSVGFrame's children-only transform changes

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

While working on bug 813531, I discovered that we have a bug in our code for updating the overflow rects of the child frames when an nsSVGOuterSVGFrame's children-only transform changes. Currently GetFrameForChildrenOnlyTransformHint returns the nsSVGOuterSVGFrame, and as a result we update the overflow of its nsSVGOuterSVGAnonChildFrame instead of the frames of its element's children.

This bug is currently hidden (most of the time at least) because our current code often causes us to dispatch an nsChangeHint_ReconstructFrame when we reflow the nsSVGOuterSVGFrame for the first time, causing us to recreate the entire frame tree. In bug 813531 I'm going to be changing the code to stop sending out that wasteful reconstruct-frame hint though.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Robert Longson [:longsonr]]

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

># HG changeset patch
># Date 1354213454 0
># Parent 9d6e95e7785580d4b3756ecbfa4c18725e9791cb
># User Jonathan Watt <[e-mail]>
>Bug 822378 - Update the overflow rects of the child frames when an nsSVGOuterSVGFrame's children-only transform changes. r=longsonr.
>
>diff --git a/layout/base/nsCSSFrameConstructor.cpp b/layout/base/nsCSSFrameConstructor.cpp
>--- a/layout/base/nsCSSFrameConstructor.cpp
>+++ b/layout/base/nsCSSFrameConstructor.cpp
>@@ -7700,16 +7700,21 @@ GetFrameForChildrenOnlyTransformHint(nsI
>     // This happens if the root-<svg> is fixed positioned, in which case we
>     // can't use aFrame->GetContent() to find the primary frame, since
>     // GetContent() returns nullptr for ViewportFrame.
>     aFrame = aFrame->GetFirstPrincipalChild();
>   }
>   // For an nsHTMLScrollFrame, this will get the SVG frame that has the
>   // children-only transforms:
>   aFrame = aFrame->GetContent()->GetPrimaryFrame();
>+  if (aFrame->GetType() == nsGkAtoms::svgOuterSVGFrame) {
>+    aFrame = aFrame->GetFirstPrincipalChild();
>+    NS_ABORT_IF_FALSE(aFrame->GetType() == nsGkAtoms::svgOuterSVGAnonChildFrame,
>+                      "Where is the nsSVGOuterSVGFrame's anon child??");
>+  }
>   NS_ABORT_IF_FALSE(aFrame->IsFrameOfType(nsIFrame::eSVG |
>                                           nsIFrame::eSVGContainer),
>                     "Children-only transforms only expected on SVG frames");
>   return aFrame;
> }
> 
> static void
> DoApplyRenderingChangeToTree(nsIFrame* aFrame,
>@@ -8189,29 +8194,31 @@ nsCSSFrameConstructor::ProcessRestyledFr
>           didReflowThisFrame = true;
>         }
>       }
>       NS_ASSERTION(!(hint & nsChangeHint_ChildrenOnlyTransform) ||
>                    (hint & nsChangeHint_UpdateOverflow),
>                    "nsChangeHint_UpdateOverflow should be passed too");
>       if ((hint & nsChangeHint_UpdateOverflow) && !didReflowThisFrame) {
>         if (hint & nsChangeHint_ChildrenOnlyTransform) {
>-          // Update overflow areas of children first:
>-          nsIFrame* childFrame =
>-            GetFrameForChildrenOnlyTransformHint(frame)->GetFirstPrincipalChild();
>+          // The overflow areas of the child frames need to be updated:
>+          nsIFrame* hintFrame = GetFrameForChildrenOnlyTransformHint(frame);
>+          nsIFrame* childFrame = hintFrame->GetFirstPrincipalChild();
>           for ( ; childFrame; childFrame = childFrame->GetNextSibling()) {
>             NS_ABORT_IF_FALSE(childFrame->IsFrameOfType(nsIFrame::eSVG),
>                               "Not expecting non-SVG children");
>+            // If |frame| is dirty or has dirty children, we don't bother updating
>+            // overflows since that will happen when it's reflowed.
>             if (!(childFrame->GetStateBits() &
>                   (NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN))) {
>-              childFrame->UpdateOverflow();
>+              aTracker.AddFrame(childFrame);
>             }
>             NS_ASSERTION(!nsLayoutUtils::GetNextContinuationOrSpecialSibling(childFrame),
>                          "SVG frames should not have continuations or special siblings");
>-            NS_ASSERTION(childFrame->GetParent() == frame,
>+            NS_ASSERTION(childFrame->GetParent() == hintFrame,
>                          "SVG child frame not expected to have different parent");

I really don't think this assert buys us anything. It would make the code simpler to remove it as we wouldn't need hintFrame.

Comment 3

[Robert Longson [:longsonr]]

Sorry about that I had planned to remove more of the preceding code ;-)

Comment 4

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #2)
> I really don't think this assert buys us anything. It would make the code
> simpler to remove it as we wouldn't need hintFrame.

I added that assertion because at some point I ended up screwing things up in a way that would have been caught by it. I can't remember the details right now, but I left it in and pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/249e7db380b0

Comment 5

[Jonathan Watt [:jwatt]]

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

Requesting approval since this blocks bug 813531, which is tracking for 18. I'm requesting approval before its been merged to m-c, but on the understanding that I'll only land it on branches if it does get merged to m-c.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown
User impact if declined: printing of SVG broken
Testing completed (on m-c, etc.): seems to have landed well on m-i
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

Comment 6

[Jonathan Watt [:jwatt]]

I got an automated Talos Regression email (Dromaeo (CSS) decrease 1.44% on Linux x64 Mozilla-Inbound-Non-PGO) that may be related to this bug. Graph is:

http://mzl.la/YdxtI7

_If_ the regression is real, and _if_ this bug is to blame, we don't have much option other than to take the slowdown since the patch here makes us do what we should have been doing all along.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/249e7db380b0

Comment 8

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8d0c225354b1

Comment 9

[bhavana bajaj [:bajaj]]

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

Approving on beta as the patch is low risk & is needed for Bug 813531 (needed on FF18)

Comment 10

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/releases/mozilla-beta/rev/809b98789ee3

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/809b98789ee3