AsyncCompositionManager doesn't compare layer tree IDs

RESOLVED FIXED in Firefox 40

Status

()

Core
Panning and Zooming
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dvander, Assigned: dvander)

Tracking

unspecified
mozilla40
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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.
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.
(Assignee)

Updated

3 years ago
Summary: RefLayers can create duplicate scroll IDs → AsyncCompositionManager doesn't compare layer tree IDs
(Assignee)

Comment 2

3 years ago
Created attachment 8588378 [details] [diff] [review]
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.
Attachment #8588378 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 3

3 years ago
Created attachment 8588379 [details] [diff] [review]
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.
Attachment #8588378 - Attachment is obsolete: true
Attachment #8588378 - Flags: review?(bugmail.mozilla)
Attachment #8588379 - Flags: review?(bugmail.mozilla)
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.
Attachment #8588379 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 5

3 years ago
Created attachment 8588501 [details] [diff] [review]
v3

w/ comments
Attachment #8588379 - Attachment is obsolete: true
Attachment #8588501 - Flags: review?(bugmail.mozilla)
Attachment #8588501 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 6

3 years ago
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7af97d940d51
https://hg.mozilla.org/mozilla-central/rev/7af97d940d51
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.