1127794 - Windowed plugin origin doesn't always match rendered content after a scroll

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Jim Mathies [:jimm]]

STR:

1) load test case and let flash load completely
2) scroll the page up and then back to the original stop at the top of the page

result:

Sometimes the plugin window never makes it back to its original origin in the page. I've dumped these coordinates in debug mode and can confirm that the last layer update with plugin data holds coordinates that are slightly out of sync.

A few potential causes:

1) the location of the final viewport the compositor chooses in the parent doesn't match the information sent to the content process when reflow was triggered.

2) It might have something to do with this weird delayed plugin geometry update timer [1] we have for non-e10s.

3) ??

Roc suggested in the original compositor / plugin work that we might have problems with pushing plugin data over from layout in the content process, and that it might behoove us to use an actual layer injected into the layer tree for plugin position calculations. This solution might help here.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresContext.cpp#2933

Comment 1

[Jim Mathies [:jimm]]

Attachment: flash test case

Comment 2

[Jim Mathies [:jimm]]

Attachment: test plugin test case

Comment 3

[Jim Mathies [:jimm]]

This bug wasn't as bad as I thought. Currently we collect up plugin geometry updates for content a little too late for the updates to get shipped over with the shadow layer updates, which occurs in nsDisplayList::PaintRoot. We need to make sure we've computed and collected the updates before the call to EndTransaction on the layer manager.

Comment 4

[Jim Mathies [:jimm]]

Attachment: patch

Comment 5

[Jim Mathies [:jimm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=405767bacf32

Comment 6

[Jim Mathies [:jimm]]

Hey roc, do you remember why you introduced this geometry timer?

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresContext.cpp#2923

From InitApplyPluginGeometryTimer -

"paints might get optimized away if the old plugin geometry covers the invalid region"

and somehow making a delayed call to ApplyPluginGeometryUpdates fixes this. I'm wondering if we need to get this hooked up in e10s.

Comment 8

[Jim Mathies [:jimm]]

Attachment: patch

also please see comment 6.

Comment 9

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

(In reply to Jim Mathies [:jimm] from comment #6)
> Hey roc, do you remember why you introduced this geometry timer?
> 
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresContext.
> cpp#2923
> 
> From InitApplyPluginGeometryTimer -
> 
> "paints might get optimized away if the old plugin geometry covers the
> invalid region"
> 
> and somehow making a delayed call to ApplyPluginGeometryUpdates fixes this.
> I'm wondering if we need to get this hooked up in e10s.

Consider the case where a plugin window covers the entire content area and then the plugin is supposed to move. We would request a paint by invalidating some area of the content, but that invalidation would be suppressed by Windows since it's still covered by the plugin's old window area, so we'd never get A WM_PAINT, never get around to painting, and hence never get around to moving the plugin window.

I don't think this should be a problem anymore since we're not relying on Windows' WM_PAINT events to drive painting.

Comment 10

[Jim Mathies [:jimm]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> (In reply to Jim Mathies [:jimm] from comment #6)
> > Hey roc, do you remember why you introduced this geometry timer?
> > 
> > http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresContext.
> > cpp#2923
> > 
> > From InitApplyPluginGeometryTimer -
> > 
> > "paints might get optimized away if the old plugin geometry covers the
> > invalid region"
> > 
> > and somehow making a delayed call to ApplyPluginGeometryUpdates fixes this.
> > I'm wondering if we need to get this hooked up in e10s.
> 
> Consider the case where a plugin window covers the entire content area and
> then the plugin is supposed to move. We would request a paint by
> invalidating some area of the content, but that invalidation would be
> suppressed by Windows since it's still covered by the plugin's old window
> area, so we'd never get A WM_PAINT, never get around to painting, and hence
> never get around to moving the plugin window.
> 
> I don't think this should be a problem anymore since we're not relying on
> Windows' WM_PAINT events to drive painting.

Excellent, thanks for the update.

Comment 11

[Jim Mathies [:jimm]]

Attachment: patch for landing r=roc

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/702f6a2260ed

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/702f6a2260ed