All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED in Firefox 46

Status

()

P1
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: alice0775, Assigned: mstange)

Tracking

(Blocks: 2 bugs, {regression})

40 Branch
mozilla46
x86_64
All
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm8+, firefox46 fixed)

Details

(Whiteboard: gfx-noted)

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 8589281 [details]
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
(Reporter)

Comment 1

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

Triggered by: Bug 1095754
Blocks: 1095754
Component: Plug-ins → Graphics: Layers
Flags: needinfo?(jmathies)
Keywords: regression
(Reporter)

Updated

4 years ago
Whiteboard: [dupeme]

Comment 2

4 years ago
Any time you run into this plugin tearing / chrome overdraw problem Alice, it's bug 1137944.
No longer blocks: 1095754
tracking-e10s: ? → +
Depends on: 1137944
Flags: needinfo?(jmathies)
Whiteboard: gfx-noted

Updated

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
No longer depends on: 1137944
Resolution: --- → DUPLICATE
Duplicate of bug: 1137944
(Reporter)

Comment 4

3 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
(Reporter)

Comment 5

3 years ago
I can also reproduce on ubuntu14.04.
OS: Windows 7 → All
(Reporter)

Comment 6

3 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
Blocks: 1095754
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(Reporter)

Comment 7

3 years ago
Created attachment 8677497 [details]
screencast

Updated

3 years ago
Blocks: 874016
No longer blocks: 1095754

Comment 8

3 years ago
Created attachment 8677509 [details]
scrollbar clip.png

this I can reproduce.

Updated

3 years ago
tracking-e10s: + → ?

Comment 9

3 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
Flags: needinfo?(roc)
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.
Flags: needinfo?(roc)

Updated

3 years ago
Summary: [e10s] Flash advertising is drawn on top of the scrollbar → [e10s] Windowed plugins aren't clipped by scrollbars

Updated

3 years ago
Assignee: nobody → jmathies
tracking-e10s: ? → m8+

Comment 11

3 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?
Flags: needinfo?(mstange)

Comment 12

3 years ago
btw I tested this with bug 1217168, but unfortunately it didn't help.
(Assignee)

Comment 13

3 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
Flags: needinfo?(mstange)

Comment 14

3 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.
Flags: needinfo?(mstange)
(Assignee)

Comment 15

3 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.
Flags: needinfo?(mstange)

Comment 16

3 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.
Flags: needinfo?(mstange)
(Assignee)

Comment 17

3 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.
Flags: needinfo?(mstange)

Comment 18

3 years ago
Created attachment 8682625 [details] [diff] [review]
wip

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

Comment 19

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

Comment 20

3 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

Comment 21

3 years ago
Appears to be on a child of the content root, which GetVisibleRegionRelativeToRootLayer doesn't look at.
(Assignee)

Comment 22

3 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

Comment 23

3 years ago
Created attachment 8683093 [details]
layers with a plugin.txt
Attachment #8682625 - Attachment is obsolete: true

Comment 24

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

Comment 25

3 years ago
I can ask matt to review the AutoResolveRefLayers changes if you like, he reviewed the original work there.
(Assignee)

Comment 26

3 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?

Comment 27

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

Comment 28

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

Comment 29

3 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

Comment 30

3 years ago
oops, ignore that top paragraph.

Comment 31

3 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.
Blocks: 1013364
Summary: [e10s] Windowed plugins aren't clipped by scrollbars → [e10s][apz] Windowed plugins aren't clipped by scrollbars

Updated

3 years ago
tracking-e10s: m8+ → ?

Updated

3 years ago
Attachment #8683099 - Flags: review?(mstange)

Updated

3 years ago
tracking-e10s: ? → +

Comment 32

3 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.
Flags: needinfo?(milan)
Sounds good.
Component: Graphics: Layers → Panning and Zooming
Flags: needinfo?(milan)
Marking as blocking all-aboard-apz as per comment 32.
Blocks: 1178298
No longer blocks: 1013364

Updated

3 years ago
Priority: -- → P1

Updated

3 years ago
Depends on: 1229429

Updated

3 years ago
Blocks: 1229429
No longer depends on: 1229429

Updated

3 years ago
Blocks: 1229451

Updated

3 years ago
Assignee: jmathies → nobody
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).
Assignee: nobody → mstange

Updated

3 years ago
tracking-e10s: + → m8+

Comment 36

3 years ago
mstange, isn't this a dupe now?
Flags: needinfo?(mstange)
(Assignee)

Comment 37

3 years ago
It's a one line fix that requires the patches in bug 1147673, so we'll need to leave this bug open.
Flags: needinfo?(mstange)

Updated

3 years ago
Attachment #8683099 - Attachment is obsolete: true
(Assignee)

Comment 38

3 years ago
Created attachment 8701474 [details]
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/
Attachment #8701474 - Flags: review?(tnikkel)
(Assignee)

Updated

3 years ago
Status: REOPENED → ASSIGNED
Attachment #8701474 - Flags: review?(tnikkel) → review+
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 41

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/058b21faea53
https://hg.mozilla.org/mozilla-central/rev/7116fd86bc65
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
No longer blocks: 1229451
Duplicate of this bug: 1229451
You need to log in before you can comment on or make changes to this bug.