Bug 1548247 - Correct and limit scroll update renderroot usage r=kats
authorDoug Thayer <dothayer@mozilla.com>
Tue, 18 Jun 2019 22:02:57 +0000
changeset 479150 c86eb07fe740f960e42a4b429de0a4e8b3634da9
parent 479149 7bc777500d4018efe7486f20a54bba09a2981195
child 479151 bf1be740ee4ad47ef0d378c3222bf6c16eaf4a5c
push id88110
push userdothayer@mozilla.com
push dateTue, 18 Jun 2019 22:04:23 +0000
treeherderautoland@e249cd75138b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskats
bugs1548247
milestone69.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1548247 - Correct and limit scroll update renderroot usage r=kats There's two things going on here. 1) nsGfxScrollFrame is getting the wrong renderroot, because it's not correctly recursing up the frame tree. 2) Hiding behind that problem is that if we do correctly assign the renderroot, we end up blocking on both render roots updating if we don't, say, have a horizontal scroll option, because that leaves us with a wr::RenderRoot::Default. 2.1) We then still end up blocking on the default renderroot because we initialize the selector with WebRenderBridgeParent's mRenderRoot. Differential Revision: https://phabricator.services.mozilla.com/D31858
gfx/layers/apz/src/APZUpdater.cpp
gfx/layers/apz/src/FocusTarget.cpp
gfx/layers/apz/src/FocusTarget.h
gfx/thebes/gfxUtils.cpp
layout/generic/nsGfxScrollFrame.cpp
--- a/gfx/layers/apz/src/APZUpdater.cpp
+++ b/gfx/layers/apz/src/APZUpdater.cpp
@@ -152,22 +152,28 @@ void APZUpdater::ClearTree(LayersId aRoo
         }
       }));
 }
 
 void APZUpdater::UpdateFocusState(LayersId aRootLayerTreeId,
                                   WRRootId aOriginatingWrRootId,
                                   const FocusTarget& aFocusTarget) {
   MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread());
-  UpdaterQueueSelector selector(aOriginatingWrRootId);
+  UpdaterQueueSelector selector(aOriginatingWrRootId.mLayersId);
   if (aFocusTarget.mData.is<FocusTarget::ScrollTargets>()) {
     const FocusTarget::ScrollTargets& targets =
         aFocusTarget.mData.as<FocusTarget::ScrollTargets>();
-    selector.mRenderRoots += targets.mHorizontalRenderRoot;
-    selector.mRenderRoots += targets.mVerticalRenderRoot;
+    if (targets.mHorizontalRenderRoot) {
+      selector.mRenderRoots += *targets.mHorizontalRenderRoot;
+    }
+    if (targets.mVerticalRenderRoot) {
+      selector.mRenderRoots += *targets.mVerticalRenderRoot;
+    }
+  } else {
+    selector.mRenderRoots += aOriginatingWrRootId.mRenderRoot;
   }
   RunOnUpdaterThread(selector,
                      NewRunnableMethod<LayersId, LayersId, FocusTarget>(
                          "APZUpdater::UpdateFocusState", mApz,
                          &APZCTreeManager::UpdateFocusState, aRootLayerTreeId,
                          aOriginatingWrRootId.mLayersId, aFocusTarget));
 }
 
--- a/gfx/layers/apz/src/FocusTarget.cpp
+++ b/gfx/layers/apz/src/FocusTarget.cpp
@@ -202,27 +202,29 @@ FocusTarget::FocusTarget(PresShell* aRoo
           selectedContent.get(), ScrollableDirection::Vertical);
 
   // We might have the globally focused element for scrolling. Gather a ViewID
   // for the horizontal and vertical scroll targets of this element.
   ScrollTargets target;
   target.mHorizontal = nsLayoutUtils::FindIDForScrollableFrame(horizontal);
   target.mVertical = nsLayoutUtils::FindIDForScrollableFrame(vertical);
   if (XRE_IsContentProcess()) {
-    target.mHorizontalRenderRoot = gfxUtils::GetContentRenderRoot();
-    target.mVerticalRenderRoot = gfxUtils::GetContentRenderRoot();
+    target.mHorizontalRenderRoot = Some(gfxUtils::GetContentRenderRoot());
+    target.mVerticalRenderRoot = Some(gfxUtils::GetContentRenderRoot());
   } else {
-    target.mHorizontalRenderRoot =
-        horizontal ? gfxUtils::RecursivelyGetRenderRootForFrame(
-                         horizontal->GetScrolledFrame())
-                   : wr::RenderRoot::Default;
-    target.mVerticalRenderRoot =
-        vertical ? gfxUtils::RecursivelyGetRenderRootForFrame(
-                       vertical->GetScrolledFrame())
-                 : wr::RenderRoot::Default;
+    if (horizontal) {
+      auto renderRoot = gfxUtils::RecursivelyGetRenderRootForFrame(
+          horizontal->GetScrolledFrame());
+      target.mHorizontalRenderRoot = Some(renderRoot);
+    }
+    if (vertical) {
+      auto renderRoot = gfxUtils::RecursivelyGetRenderRootForFrame(
+          vertical->GetScrolledFrame());
+      target.mVerticalRenderRoot = Some(renderRoot);
+    }
   }
   mData = AsVariant(target);
 
   FT_LOG("Creating scroll target with seq=%" PRIu64 ", kl=%d, h=%" PRIu64
          ", v=%" PRIu64 "\n",
          aFocusSequenceNumber, mFocusHasKeyEventListeners, target.mHorizontal,
          target.mVertical);
 }
--- a/gfx/layers/apz/src/FocusTarget.h
+++ b/gfx/layers/apz/src/FocusTarget.h
@@ -8,16 +8,17 @@
 #define mozilla_layers_FocusTarget_h
 
 #include <stdint.h>  // for int32_t, uint32_t
 
 #include "mozilla/DefineEnum.h"                  // for MOZ_DEFINE_ENUM
 #include "mozilla/layers/ScrollableLayerGuid.h"  // for ViewID
 #include "mozilla/webrender/WebRenderTypes.h"    // for RenderRoot
 #include "mozilla/Variant.h"                     // for Variant
+#include "mozilla/Maybe.h"                       // for Maybe
 
 namespace mozilla {
 
 class PresShell;
 
 namespace layers {
 
 /**
@@ -25,19 +26,19 @@ namespace layers {
  * element of a document and the scrollable frames to use when keyboard
  * scrolling it. It is created on the main thread at paint-time, but is then
  * passed over IPC to the compositor/APZ code.
  */
 class FocusTarget final {
  public:
   struct ScrollTargets {
     ScrollableLayerGuid::ViewID mHorizontal;
-    wr::RenderRoot mHorizontalRenderRoot;
+    Maybe<wr::RenderRoot> mHorizontalRenderRoot;
     ScrollableLayerGuid::ViewID mVertical;
-    wr::RenderRoot mVerticalRenderRoot;
+    Maybe<wr::RenderRoot> mVerticalRenderRoot;
 
     bool operator==(const ScrollTargets& aRhs) const {
       bool ret =
           (mHorizontal == aRhs.mHorizontal && mVertical == aRhs.mVertical);
       if (ret) {
         // The render root is a function of where the scrollable frame is in
         // the DOM/layout tree, so if the ViewIDs match then the render roots
         // should also match.
--- a/gfx/thebes/gfxUtils.cpp
+++ b/gfx/thebes/gfxUtils.cpp
@@ -1506,17 +1506,17 @@ Maybe<wr::RenderRoot> gfxUtils::GetRende
 
 wr::RenderRoot gfxUtils::RecursivelyGetRenderRootForFrame(
     const nsIFrame* aFrame) {
   if (!gfxVars::UseWebRender() || !StaticPrefs::WebRenderSplitRenderRoots()) {
     return wr::RenderRoot::Default;
   }
 
   for (const nsIFrame* current = aFrame; current;
-       current = current->GetParent()) {
+       current = nsLayoutUtils::GetCrossDocParentFrame(current)) {
     auto renderRoot = gfxUtils::GetRenderRootForFrame(current);
     if (renderRoot) {
       return *renderRoot;
     }
   }
 
   return wr::RenderRoot::Default;
 }
--- a/layout/generic/nsGfxScrollFrame.cpp
+++ b/layout/generic/nsGfxScrollFrame.cpp
@@ -2851,19 +2851,17 @@ void ScrollFrameHelper::ScrollToImpl(nsP
             bool success = nsLayoutUtils::FindIDFor(content, &id);
             MOZ_ASSERT(success);  // we have a displayport, we better have an ID
 
             // Schedule an empty transaction to carry over the scroll offset
             // update, instead of a full transaction. This empty transaction
             // might still get squashed into a full transaction if something
             // happens to trigger one.
             wr::RenderRoot renderRoot = wr::RenderRoot::Default;
-            if (XRE_IsContentProcess()) {
-              renderRoot = gfxUtils::GetContentRenderRoot();
-            } else {
+            if (XRE_IsParentProcess()) {
               renderRoot = gfxUtils::RecursivelyGetRenderRootForFrame(mOuter);
             }
             success = manager->SetPendingScrollUpdateForNextTransaction(
                 id,
                 {mScrollGeneration, CSSPoint::FromAppUnits(GetScrollPosition()),
                  CSSPoint::FromAppUnits(GetApzScrollPosition()),
                  mLastScrollOrigin == nsGkAtoms::relative},
                 renderRoot);