1151169 - AsyncCompositionManager doesn't compare layer tree IDs

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

With e10s, a layer underneath a RefLayer can have the same scroll ID as a layer above the RefLayer. This apparently confuses something in AsyncCompositionManager. Enabling APZ+E10S in a debug build of Firefox shows one problem: the scrollbar thumb doesn't have an async transform since it has the same scrollid as the top-level layer. As a result it doesn't move until the next content repaint, which in a debug build is much slower.

I'm not sure whether this is a layout bug and a Scroll ID should really be a (pid, viewid) tuple, or whether that's already implied and AsyncCompositionManager just has a bug.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

It's a composition manager bug. The scrollid is only meamt to be unique within a given layers id, so the compositor should check for the (layersid, scrollid) tuple.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

This forwards the scrollbar layer's tree id throughout TransformShadowTree(), then if we have to find the target of a scrollbar, we only search the portion of the tree beginning at that ID and ending at any RefLayer.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: v2

Slightly cleaner patch.

I considered following the big comment's advice on using the APZC tree instead, but it wasn't immediately clear how to do it... and the number of layers in general should be very small so I doubt it's worth it.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8588379 [details] [diff] [review]
v2

Review of attachment 8588379 [details] [diff] [review]:
-----------------------------------------------------------------

See comment. I didn't do a full review but can do one if my comment doesn't work for whatever reason.

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +883,5 @@
> +    return LayerMetricsWrapper();
> +  }
> +
> +  LayerMetricsWrapper root(state->mRoot);
> +  for (LayerMetricsWrapper ancestor(aScrollbar); ancestor != root; ancestor = ancestor.GetParent()) {

Hm.. can we instead do this?

{
  // Search ancestors first, identify root of subtree
  // with the same layers id
  LayerMetricsWrapper root;
  LayerMetricsWrapper scrollbar(aScrollbar);
  for (LayerMetricsWrapper ancestor(aScrollbar; ancestor; ancestor = ancestor.GetParent()) {
    if (ancestor.AsRefLayer()) {
      break;
    }
    root = ancestor;
    if (LayerIsScrollbarTarget(ancestor, aScrollbar) {
      ...
    }
  }
  // "root" now points to the root of the subtree with the same layers id as the scrollbar

That way you don't need to be passing around the layers id all over the place. After all, the exact layers id isn't really important, just that we never cross a RefLayer either going upwards or downwards when looking around the tree. You are already checking for RefLayers (as opposed to a specific layers id) when going downwards in FindScrolledLayerRecursive, so if you do something similar here while walking upwards that should be all you need.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: v3

w/ comments

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7af97d940d51

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7af97d940d51