1152049 - [e10s][apz] Windowed plugins aren't clipped by scrollbars

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P1)

Description

[Alice0775 White]

Attachment: screenshot

This does not happen without e10s.

Steps To Reproduce:
1. Clear cache and cookies if any
2. Open http://www.n2yo.com/?s=38348|25544
3. Wait for a while until the ads hide

Actual Results:
Flash advertising is drawn on top of the scrollbar

Expected Results:
Flash advertising should not be drawn on top of the scrollbar

Comment 1

[Alice0775 White]

Regression pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=741a15659b09&tochange=5969eb0fe8b5

Triggered by: Bug 1095754

Comment 2

[Jim Mathies [:jimm]]

Any time you run into this plugin tearing / chrome overdraw problem Alice, it's bug 1137944.

Comment 4

[Alice0775 White]

alternate STR
1. Open http://edition.cnn.com/2014/11/23/showbiz/music/katy-perry-super-bowl/index.html?hpt=en_bn1
2. Shrink browser window and/or height to 600-500px

Comment 5

[Alice0775 White]

I can also reproduce on ubuntu14.04.

Comment 6

[Alice0775 White]

I can still reproduce on Windows7


https://hg.mozilla.org/mozilla-central/rev/473aefe5bd85842eeb142e0cde8e2cd21edbf40b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0 ID:20151021065025

Comment 7

[Alice0775 White]

Attachment: screencast

Comment 8

[Jim Mathies [:jimm]]

Attachment: scrollbar clip.png

this I can reproduce.

Comment 9

[Jim Mathies [:jimm]]

hey roc, any ideas as to why this scrollbar isn't getting picked up by the clipping calculations we do in layers code?

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp#2118

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No. I would dump display lists and layers (MOZ_DUMP_PAINT=1); between them we should be able to figure out what the problem is. I'm guessing the clip isn't reaching the layer tree for some reason.

Comment 11

[Jim Mathies [:jimm]]

hey markus, I haven't had a chance to look at this yet. Since you were just messing around in this code I'm wondering you might have a guess as to what could be causing this scrollbar bug?

Comment 12

[Jim Mathies [:jimm]]

btw I tested this with bug 1217168, but unfortunately it didn't help.

Comment 13

[Markus Stange [:mstange]]

The clip for the scroll area is a frame metrics clip. Frame metrics clips get applied during AsyncCompositionManager::ApplyAsyncContentTransformToTree at [1][2], where they are folded into the layer's shadow clip rect. Does the plugin visibility code run before or after AsyncCompositionManager::TransformShadowTree? It needs to run after if it wants complete clip information.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp#846
[2] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp#887

Comment 14

[Jim Mathies [:jimm]]

It happens just before TransformShadowTree is called here - 

https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp#1059

This was the only point where the remote layer tree was connected to the chrome layer tree for clipping calculations. AutoResolveRefLayers calls into the manager here -

https://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp#129

and WalkTheTree does the actual work here - 

https://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp?case=true&from=WalkTheTree#95

I'm not sure we can move this since the destructor of that AutoResolveRefLayers appears to disconnect the remote layer tree here - 

https://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.h#250

any suggestions you might have are welcome.

Comment 15

[Markus Stange [:mstange]]

(In reply to Jim Mathies [:jimm] from comment #14)
> I'm not sure we can move this since the destructor of that
> AutoResolveRefLayers appears to disconnect the remote layer tree

The destructor doesn't run until the very end of CompositorParent::CompositeToTarget. TransformShadowTree runs while AutoResolveRefLayers is still on the stack. It has to, since it needs to operate on the merged tree.

Comment 16

[Jim Mathies [:jimm]]

(In reply to Markus Stange [:mstange] from comment #15)
> (In reply to Jim Mathies [:jimm] from comment #14)
> > I'm not sure we can move this since the destructor of that
> > AutoResolveRefLayers appears to disconnect the remote layer tree
> 
> The destructor doesn't run until the very end of
> CompositorParent::CompositeToTarget. TransformShadowTree runs while
> AutoResolveRefLayers is still on the stack. It has to, since it needs to
> operate on the merged tree.

ah yeah thanks, I was thinking that was in it's own block.

I'm still a little leery of moving this since right after AutoResolveRefLayers constructor returns we will bail out of composition fairly often -

https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp#1084

If I move plugin calculation below TransformShadowTree, lots of compositor set up code will have been called already, specifically - 

BeginTransaction related calls
https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp#1104

SetShadowLayerPropeerties
https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp#1110

ComputeRotation
https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp#1121

Do you see any issues here? I'm concerned this might break something or kill perf.

Comment 17

[Markus Stange [:mstange]]

(In reply to Jim Mathies [:jimm] from comment #16)
> (In reply to Markus Stange [:mstange] from comment #15)
> > (In reply to Jim Mathies [:jimm] from comment #14)
> > > I'm not sure we can move this since the destructor of that
> > > AutoResolveRefLayers appears to disconnect the remote layer tree
> > 
> > The destructor doesn't run until the very end of
> > CompositorParent::CompositeToTarget. TransformShadowTree runs while
> > AutoResolveRefLayers is still on the stack. It has to, since it needs to
> > operate on the merged tree.
> 
> ah yeah thanks, I was thinking that was in it's own block.
> 
> I'm still a little leery of moving this since right after
> AutoResolveRefLayers constructor returns we will bail out of composition
> fairly often -
> 
> https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/
> CompositorParent.cpp#1084

I think the plugin related bailouts need to move along with the plugin visibility computation.

> If I move plugin calculation below TransformShadowTree, lots of compositor
> set up code will have been called already, specifically - 
> 
> BeginTransaction related calls
> https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/
> CompositorParent.cpp#1104

This one worries me a little, because if we bail out we'll have called BeginTransaction without a matching EndTransaction. But looking at the code in BeginTransaction, that shouldn't be a problem, and there's already an early return after BeginTransaction (at line 1117).

> SetShadowLayerPropeerties
> https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/
> CompositorParent.cpp#1110

This one ensures that TransformShadowTree doesn't pile up async transforms / clips, but that it can start with a clean slate each composition.

Actually - does that mean that at the moment, plugin visibility computation runs while the shadow properties still have their values from the last composite? Then my hypothesis about the cause of the missing clip might be incorrect.

> ComputeRotation
> https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/
> CompositorParent.cpp#1121

Not a problem since we don't have rotated screens on platforms where we need plugin clipping. And even if we had, it probably wouldn't interfere with anything, but I haven't verified that.

> Do you see any issues here? I'm concerned this might break something or kill
> perf.

Nothing jumps out at me. But it is a fairly fundamental change to the way things work, so it needs some amount of testing. I don't see a way this could kill perf.

Comment 18

[Jim Mathies [:jimm]]

Attachment: wip

unfortunately this still doesn't fix the scrollbar clipping problem.

Comment 19

[Markus Stange [:mstange]]

That's unfortunate. I don't have any other ideas for the scrollbar clipping problem at the moment. You'll need to check whether the framemetrics clip makes it to the plugin layer and whether it gets picked up there at the right time.

Comment 20

[Jim Mathies [:jimm]]

I seem to be missing the layer that gets the scrollbar clip when calculating the plugin clip. not sure why yet.

layer=1B05A800 asyncClip: 0,0 x 629,808
GetVisibleRegionRelativeToRootLayer:
layer=1AB56000 noclip
layer=1B5BE400 1,113 x 646,808
layer=1A1BBC00 noclip

Comment 21

[Jim Mathies [:jimm]]

Appears to be on a child of the content root, which GetVisibleRegionRelativeToRootLayer doesn't look at.

Comment 22

[Markus Stange [:mstange]]

Can you attach a layers dump here? layers.dump-host-layers=true, or layerManagerComposite->Dump(), requires an --enable-dump-painting or debug build

Comment 23

[Jim Mathies [:jimm]]

Attachment: layers with a plugin.txt

Comment 24

[Jim Mathies [:jimm]]

Attachment: patch v.1

This solves the problem. I'm only applying the clip from the first set of child layers below the ContainerLayerComposite. I could also access the scrollbar clip directly by going to GetLastChild instead. I think walking over the list is right, although I'm not sure if I should be digging lower than that.

Comment 25

[Jim Mathies [:jimm]]

I can ask matt to review the AutoResolveRefLayers changes if you like, he reviewed the original work there.

Comment 26

[Markus Stange [:mstange]]

This doesn't seem right. Why are we checking the clip on the ContainerLayer? Don't you want the clip for the plugin layer?

Comment 27

[Jim Mathies [:jimm]]

(In reply to Markus Stange [:mstange] from comment #26)
> This doesn't seem right. Why are we checking the clip on the ContainerLayer?
> Don't you want the clip for the plugin layer?

There is no plugin layer, we have a LayerTransactionParent which is asked for its root layer [1], this is a 'ContainerLayerComposite', and is considered the root of the content contained in a tab that hosts a plugin. We call GetVisibleRegionRelativeToRootLayer on this, which walks up the layer tree looking at parents and siblings of parents accumulating a visible region which then gets applied to all plugins in the tab.

I've added code that applies the clip rect stored in the first set of child layers below this root layer because that's where the scrollbar clip is stored.

If you look at the text dump, we have:

root content layer: ContainerLayerComposite (0x1c204800)
 first child: PaintedLayerComposite (0x1bf9ec00) not visible, no clip
 second child: ContainerLayerComposite (0x1c20b000)opaqueContent, dims = content dims
 third child: PaintedLayerComposite (0x1c20ec00) (has scrollid, scroll clip

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp#2119

Comment 28

[Markus Stange [:mstange]]

(In reply to Jim Mathies [:jimm] from comment #27)
> accumulating a visible
> region which then gets applied to all plugins in the tab.

Oh, this is the part I didn't know about. Why are you using the same clip for all the plugins in the tab? What about plugins that are in a scrollable div, which need a different clip?
I was expecting us to compute one clip per plugin, from the layer for that plugin.

Comment 29

[Jim Mathies [:jimm]]

Maybe I could not alter GetVisibleRegionRelativeToRootLayer, but start with the PaintedLayerComposite instead and call GetVisibleRegionRelativeToRootLayer on that? I think this is what you consider the 'plugin layer'. I'm not sure how you find it accurately from the ContainerLayerComposite.(In reply to Markus Stange [:mstange] from comment #28)
> (In reply to Jim Mathies [:jimm] from comment #27)
> > accumulating a visible
> > region which then gets applied to all plugins in the tab.
> 
> Oh, this is the part I didn't know about. Why are you using the same clip
> for all the plugins in the tab? What about plugins that are in a scrollable
> div, which need a different clip?
> I was expecting us to compute one clip per plugin, from the layer for that
> plugin.

If content clips the plugin, that info should get calculated in the content process and handed over via the plugin data struct [1] that we send over with the chrome side clipping info [2] we calculate here.

An alternate fix here might be to not change GetVisibleRegionRelativeToRootLayer, but start with the PaintedLayerComposite instead and call GetVisibleRegionRelativeToRootLayer on that. (I think this is what you consider the 'plugin layer'.) Then the current logic in GetVisibleRegionRelativeToRootLayer would walk those siblings and pick up the scroll clip.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/LayersMessages.ipdlh#281
[2] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp#2136

Comment 30

[Jim Mathies [:jimm]]

oops, ignore that top paragraph.

Comment 31

[Jim Mathies [:jimm]]

This is an apz only bug. Discussing this with mstange also shows that we have a problem with subframe scrollbars too. This is trickier to fix.

Comment 32

[Jim Mathies [:jimm]]

Hey Milan, this should block apz rollout. I might be able to help work on it, although currently the e10s team is not blocked on it. Even if we get windowless flash running we're still sol with other types of plugins for the next year so I don't think this can get ignored. I think we want to get this and bug 1193055 tracked by one of the projects.

Comment 33

[Milan Sreckovic [:milan] (needinfo for best results)]

Sounds good.

Comment 34

[Botond Ballo [:botond]]

Marking as blocking all-aboard-apz as per comment 32.

Comment 35

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

Based on the discussion yesterday Markus is going to look at the clipping in layout (AIUI the display item for plugins needs to have the clip intersection from scrollframes, even if there is a displayport).

Comment 36

[Jim Mathies [:jimm]]

mstange, isn't this a dupe now?

Comment 37

[Markus Stange [:mstange]]

It's a one line fix that requires the patches in bug 1147673, so we'll need to leave this bug open.

Comment 38

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1152049 - Apply all scroll clips when computing plugin clips in content. r?tn

Review commit: https://reviewboard.mozilla.org/r/28969/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28969/

Comment 39

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8701474 [details]
MozReview Request: Bug 1152049 - Apply all scroll clips when computing plugin clips in content. r?tn

https://reviewboard.mozilla.org/r/28969/#review25811

::: layout/generic/nsPluginFrame.cpp:1023
(Diff revision 1)
> +      visibleRegion.And(*aVisibleRegion, GetClippedBoundsUpTo(aBuilder, nullptr));

As discussed on irc. GetClippedBoundsUpTo is a little confusing. GetScrolledClippedBoundsUpTo is an improvement, here or followup bug is fine.

Comment 40

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/058b21faea53
https://hg.mozilla.org/integration/mozilla-inbound/rev/7116fd86bc65

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/058b21faea53
https://hg.mozilla.org/mozilla-central/rev/7116fd86bc65