1331402 - Animated plugins in background tabs do CALayer updates and cause wakeups

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Markus Stange [:mstange]]

Steps to reproduce:
 1. Open http://zombo.com/ in a tab.
 2. Switch to a different tab that does not have a plugin in it.

Expected results:
The plugin background tab should not cause any calls to PPluginInstance::SendShow.

Actual results:
The plugin keeps submitting frames at 60fps.

Example profile: https://clptr.io/2iyM9Om

Comment 1

[Markus Stange [:mstange]]

This is because we're not setting the clipRect to empty.

Comment 2

[Markus Stange [:mstange]]

Attachment: Bug 1331402 - Update plugins' clipRects when hiding a tab.

In non-e10s, all tabs of a window would share one nsRootPresContext. When
switching tabs, the tab switch paint would trigger a call to
nsRootPresContext::ComputePluginGeometryUpdates, which would call
SetEmptyWidgetConfiguration on all registered nsPluginFrames, notably also
those inside the tab that was hidden.

With e10s, every tab has its own nsRootPresContext, and paints in one tab do
not trigger calls to nsRootPresContext::ComputePluginGeometryUpdates on the
root pres contexts of other tabs. So when painting the new tab, the registered
plugin frames in the old tab were not notified that they were now hidden.
This patch now does that in TabChild::MakeHidden.

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

Comment 3

[Jim Mathies [:jimm]]

Comment on attachment 8827521 [details]
Bug 1331402 - Update plugins' clipRects when hiding a tab.

https://reviewboard.mozilla.org/r/105182/#review106318

Comment 4

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8827521 [details]
Bug 1331402 - Update plugins' clipRects when hiding a tab.

https://reviewboard.mozilla.org/r/105182/#review106482

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6587e676b76d

Comment 6

[Jim Mathies [:jimm]]

is this safe to uplift to 52 (beta as of today)?

Comment 7

[Markus Stange [:mstange]]

I'd like to uplift it but I'm not 100% sure how / whether this bug and the patch affect non-Mac platforms, so I'm not comfortable requesting uplift quite yet. Can you check whether plugins on Windows are affected by it?

Comment 8

[Jim Mathies [:jimm]]

(In reply to Markus Stange [:mstange] from comment #7)
> I'd like to uplift it but I'm not 100% sure how / whether this bug and the
> patch affect non-Mac platforms, so I'm not comfortable requesting uplift
> quite yet. Can you check whether plugins on Windows are affected by it?

Hmm, so we update clipRect for both windowed and windowless on Windows in a slightly different place - 

xul.dll!nsPluginInstanceOwner::UpdateWindowPositionAndClipRect(bool aSetWindow) Line 3625	C++
xul.dll!nsPluginInstanceOwner::UpdateDocumentActiveState(bool aIsActive) Line 3689	C++
xul.dll!SetPluginIsActive(nsISupports * aSupports, void * aClosure) Line 10728	C++
xul.dll!nsIDocument::EnumerateActivityObservers(void(*)(nsISupports *, void *) aEnumerator, void * aData) Line 9779	C++
xul.dll!mozilla::PresShell::SetIsActive(bool aIsActive) Line 10750	C++
xul.dll!nsDocShell::SetIsActive(bool aIsActive) Line 6198	C++
xul.dll!mozilla::dom::TabChild::RecvSetDocShellIsActive(const bool & aIsActive, const bool & aPreserveLayers, const unsigned __int64 & aLayerObserverEpoch) Line 2424	C++

So on Windows I think your added code is redundant. However it doesn't result in any additional calls to SetWindow on the plugin since we have checks for this in nsPluginInstanceOwner::UpdateWindowPositionAndClipRect. I haven't check yet but I'[m pretty sure linux follows this code path as well.

Maybe we should ifdef that code you added to osx?

Comment 9

[Jim Mathies [:jimm]]

or maybe move it to the nsPluginFrame code specific to OSX, assuming there's something in there that gets triggered by the doc active state changes.

Comment 10

[Markus Stange [:mstange]]

Or we could make nsPluginInstanceOwner::UpdateDocumentActiveState call into similar code on OSX (which sets the clip rect to empty) and remove the code I added in this bug. That would make things work more similarly on the different platforms.