Alice0775 White Reporter
	Description • 9 years ago

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

	Alice0775 White Reporter
	Comment 1 • 9 years ago

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

Triggered by: Bug 1095754

	Jim Mathies [:jimm]
	Comment 2 • 9 years ago

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

	Alice0775 White Reporter
	Comment 4 • 9 years ago

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

	Alice0775 White Reporter
	Comment 5 • 9 years ago

I can also reproduce on ubuntu14.04.

	Alice0775 White Reporter
	Comment 6 • 9 years ago

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

	Jim Mathies [:jimm]
	Comment 8 • 9 years ago

this I can reproduce.

	Jim Mathies [:jimm]
	Comment 9 • 9 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 10 • 9 years ago

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.

	Jim Mathies [:jimm]
	Comment 11 • 9 years ago

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?

	Jim Mathies [:jimm]
	Comment 12 • 9 years ago

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

	Markus Stange [:mstange] Assignee
	Comment 13 • 9 years ago

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

	Jim Mathies [:jimm]
	Comment 14 • 9 years ago

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.

	Markus Stange [:mstange] Assignee
	Comment 15 • 9 years ago

(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.

	Jim Mathies [:jimm]
	Comment 16 • 9 years ago

(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.

	Markus Stange [:mstange] Assignee
	Comment 17 • 9 years ago

(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.

	Jim Mathies [:jimm]
	Comment 18 • 9 years ago

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

	Markus Stange [:mstange] Assignee
	Comment 19 • 9 years ago

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.

	Jim Mathies [:jimm]
	Comment 20 • 9 years ago

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

	Jim Mathies [:jimm]
	Comment 21 • 9 years ago

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

	Markus Stange [:mstange] Assignee
	Comment 22 • 9 years ago

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

	Jim Mathies [:jimm]
	Comment 24 • 9 years ago

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.

	Jim Mathies [:jimm]
	Comment 25 • 9 years ago

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

	Markus Stange [:mstange] Assignee
	Comment 26 • 9 years ago

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

	Jim Mathies [:jimm]
	Comment 27 • 9 years ago

(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

	Markus Stange [:mstange] Assignee
	Comment 28 • 9 years ago

(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.

	Jim Mathies [:jimm]
	Comment 29 • 9 years ago

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

	Jim Mathies [:jimm]
	Comment 30 • 9 years ago

oops, ignore that top paragraph.

	Jim Mathies [:jimm]
	Comment 31 • 9 years ago

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.

	Jim Mathies [:jimm]
	Comment 32 • 9 years ago

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.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 33 • 9 years ago

Sounds good.

	Botond Ballo [:botond]
	Comment 34 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 35 • 9 years ago

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).

	Jim Mathies [:jimm]
	Comment 36 • 9 years ago

mstange, isn't this a dupe now?

	Markus Stange [:mstange] Assignee
	Comment 37 • 9 years ago

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

	Markus Stange [:mstange] Assignee
	Comment 38 • 9 years ago

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

	Timothy Nikkel (:tnikkel)
	Comment 39 • 9 years ago

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.

	Pulsebot
	Comment 40 • 9 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 41 • 9 years ago
bugherder

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