1121811 - [Power] Flash on mac is triggering 60 FPS recomposites on Nightly

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Benoit Girard (:BenWa)]

There's a flash plugin on the facebook feed page triggering 60 FPS recomposites. Doesn't trigger on release. It's the 'Create Photo Album' Button. I also believe various instance of Flash also triggers this.

Requesting block for severe power regression.

Comment 1

[Benoit Girard (:BenWa)]

Better test page:
http://www.fastswf.com/

Comment 2

[Benoit Girard (:BenWa)]

Regression range:
-g 2014-12-11 -b 2014-12-12

Comment 3

[Benoit Girard (:BenWa)]

good 0cf461e62ce5
bad 5288b15d22de

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0cf461e62ce5&tochange=5288b15d22de

Guessing bug 1092630 for now.

Comment 4

[Josh Aas]

David, Markus - can one of you take a look at this?

Comment 5

[Markus Stange [:mstange]]

I can, if Benoit doesn't beat me to it.

Comment 6

[Benoit Girard (:BenWa)]

[Tracking Requested - why for this release]:
Regressed in 37. It should also block 37.

Comment 7

[David Parks [:handyman]]

Attachment: Only notify plugin of changed window attributes if attributes actually changed

Yeah, this is annoying.  Its caused me to have to reboot my MBP multiple times today due to overheating.

The issue seems to be that we notify the plugin of changed window attributes, even when the attributes are unchanged.  Whenever Flash gets that call, it invalidates the display.  This behavior can be seen in this call stack:

    (lldb) bt
    * thread #1: tid = 0xb94e, 0x0000000102667302 XUL`mozilla::plugins::PluginInstanceChild::InvalidateRect(_NPRect*) [inlined] mozilla::plugins::PluginInstanceChild::AsyncShowPluginFrame(this=0x000000010970f800) at PluginInstanceChild.cpp:3492, queue = 'com.apple.main-thread', stop reason = breakpoint 2.3
      * frame #0: 0x0000000102667302 XUL`mozilla::plugins::PluginInstanceChild::InvalidateRect(_NPRect*) [inlined] mozilla::plugins::PluginInstanceChild::AsyncShowPluginFrame(this=0x000000010970f800) at PluginInstanceChild.cpp:3492
    frame #1: 0x0000000102667302 XUL`mozilla::plugins::PluginInstanceChild::InvalidateRect(this=0x000000010970f800, aInvalidRect=<unavailable>) + 258 at PluginInstanceChild.cpp:3525
    frame #2: 0x000000010fb75b40 FlashPlayer-10.6`___lldb_unnamed_function20685$$FlashPlayer-10.6 + 592
    frame #3: 0x000000010fa99214 FlashPlayer-10.6`___lldb_unnamed_function17175$$FlashPlayer-10.6 + 692
    frame #4: 0x000000010fa1fa6c FlashPlayer-10.6`___lldb_unnamed_function14782$$FlashPlayer-10.6 + 268
    frame #5: 0x0000000102669639 XUL`mozilla::plugins::PluginInstanceChild::UpdateWindowAttributes(this=<unavailable>, aForceSetWindow=<unavailable>) + 201 at PluginInstanceChild.cpp:2989
    frame #6: 0x00000001026694c9 XUL`mozilla::plugins::PluginInstanceChild::DoAsyncSetWindow(this=0x000000010970f800, aSurfaceType=0x00000001128421c0, aWindow=<unavailable>, aIsAsync=<unavailable>) + 521 at PluginInstanceChild.cpp:2651
    frame #7: 0x00000001009b4a90 XUL`MessageLoop::RunTask(this=0x00007fff5fbfef48, task=0x0000000112842190) + 144 at message_loop.cc:361
    frame #8: 0x00000001009b5339 XUL`MessageLoop::DeferOrRunPendingTask(this=0x00007fff5fbfef48, pending_task=<unavailable>) + 121 at message_loop.cc:369
    frame #9: 0x00000001009b569b XUL`MessageLoop::DoWork(this=0x00007fff5fbfef48) + 283 at message_loop.cc:447
    frame #10: 0x00000001009c34da XUL`base::MessagePumpCFRunLoopBase::RunWorkSource(void*) [inlined] base::MessagePumpCFRunLoopBase::RunWork(this=0x00000001097623e0) + 32 at message_pump_mac.mm:277
    frame #11: 0x00000001009c34ba XUL`base::MessagePumpCFRunLoopBase::RunWorkSource(info=0x00000001097623e0) + 10 at message_pump_mac.mm:255
    frame #12: 0x00007fff938215b1 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
    frame #13: 0x00007fff93812c62 CoreFoundation`__CFRunLoopDoSources0 + 242
    frame #14: 0x00007fff938123ef CoreFoundation`__CFRunLoopRun + 831
    frame #15: 0x00007fff93811e75 CoreFoundation`CFRunLoopRunSpecific + 309
    frame #16: 0x00007fff92b10a0d HIToolbox`RunCurrentEventLoopInMode + 226
    frame #17: 0x00007fff92b107b7 HIToolbox`ReceiveNextEventCommon + 479
    frame #18: 0x00007fff92b105bc HIToolbox`_BlockUntilNextEventMatchingListInModeWithFilter + 65
    frame #19: 0x00007fff8a3f524e AppKit`_DPSNextEvent + 1434
    frame #20: 0x00007fff8a3f489b AppKit`-[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 122
    frame #21: 0x00007fff8a3e899c AppKit`-[NSApplication run] + 553
    frame #22: 0x00000001009c3f67 XUL`base::MessagePumpNSApplication::DoRun(this=0x00000001097623e0, delegate=<unavailable>) + 279 at message_pump_mac.mm:663
    frame #23: 0x00000001009c3a1a XUL`base::MessagePumpCFRunLoopBase::Run(this=0x00000001097623e0, delegate=0x00007fff5fbfef48) + 138 at message_pump_mac.mm:213
    frame #24: 0x00000001009b48c0 XUL`MessageLoop::Run() [inlined] MessageLoop::RunHandler(this=<unavailable>) + 64 at message_loop.cc:226
    frame #25: 0x00000001009b48bb XUL`MessageLoop::Run(this=<unavailable>) + 59 at message_loop.cc:200
    frame #26: 0x00000001034e6153 XUL`XRE_InitChildProcess(aArgc=<unavailable>, aArgv=<unavailable>, aGMPLoader=<unavailable>) + 1971 at nsEmbedFunctions.cpp:575
    frame #27: 0x0000000100001b62 plugin-container`content_process_main(argc=7, argv=0x00007fff5fbff210) + 82 at plugin-container.cpp:211
    frame #28: 0x0000000100001864 plugin-container`start + 52

This patch simply checks that window attributes changed before notifying Flash.

Comment 8

[Benoit Girard (:BenWa)]

Bad hunk:
https://hg.mozilla.org/mozilla-central/rev/a6db8f54f5aa#l21.208

This causes a render loop:
Plugin invalidates -> We run the drawing code -> DidSetWidgetGeometry is called on every paint -> SetWindow is called -> Plugin responds to SetWindow by invalidating, goto step 1.

In the old code we used to call FixUpPluginWindow which would only call SetWindow if there actually was a change. This prevented the loop.

Comment 9

[Benoit Girard (:BenWa)]

Ahh, looks like you beat me to it.

Thanks!

Comment 10

[Benoit Girard (:BenWa)]

Actually I'm not sure you have the right fix. With your fix we're still going to send needless messages to the child and if my diagnostic is right we're should still have the problem with inprocess plugin with your patch.

Comment 11

[Benoit Girard (:BenWa)]

Attachment: Back out a6db8f54f5aa#l21.208

This fixes the problem locally by removing the bad hunk. Do you know why this change was originally made?

Comment 12

[David Parks [:handyman]]

Comment on attachment 8550581 [details] [diff] [review]
Back out a6db8f54f5aa#l21.208

Yeah, I hadn't actually hit the root of the problem, which is that the cycle is really caused by the nsPluginFrame, not the nsPluginInstanceOwner.  I'm suspicious of the condition in FixUpPluginWindow -- that only looks for changes the clip rect -- but otherwise it seems to what would be expected.

I'm not sure why the change went in... it was pretty early on tho.  Here's the change in the github repo where the work was done:

https://github.com/bdaehlie/gecko-dev/commit/ade5701563a1d84c8a1b165b5be5a46cbe50e067

Maybe Josh knows if its safe to revert?

I'm reassigning the review to josh (I cant do reviews)

Comment 13

[Josh Aas]

Comment on attachment 8550581 [details] [diff] [review]
Back out a6db8f54f5aa#l21.208

Review of attachment 8550581 [details] [diff] [review]:
-----------------------------------------------------------------

This seems fine to me, provided:

1) Basic local testing shows the issue is resolved
2) Tests pass on try
3) Local HiDPI tests pass

Thanks for the fix!

Comment 14

[Benoit Girard (:BenWa)]

(In reply to Josh Aas (Mozilla Corporation) from comment #13)
> 3) Local HiDPI tests pass

How do I run those?

Comment 15

[Josh Aas]

(In reply to Benoit Girard (:BenWa) from comment #14)
> > 3) Local HiDPI tests pass
> 
> How do I run those?

Just run the plugin tests locally on a Retina machine (MBP).

Comment 16

[Steven Michaud [:smichaud] (Retired)]

Which version of OS X you run/build on can make a difference in which mochitests pass.  I'd say run locally with and without this bug's patch.  If you see the same failure with and without the patch, then it made no difference and can be landed ... as long as the "official" tests pass, on the "official" servers :-)

Comment 17

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=9f539347ff27

Comment 18

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5842b906435

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b5842b906435

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

BenWa - 37 is marked as affected. Should we apply this backout to Aurora as well? If so, can you please request uplift?

Comment 21

[Benoit Girard (:BenWa)]

Comment on attachment 8550581 [details] [diff] [review]
Back out a6db8f54f5aa#l21.208

Approval Request Comment
[Feature/regressing bug #]: bug 1092630
[User impact if declined]: Power & Performance regression
[Describe test coverage new/current, TreeHerder]: CI + Manual testing for HIDPI. Has been on central for several days now.
[Risks and why]: Low-Med. Reverting hunk we don't really know why it was changed but it's a significant change in control flow.
[String/UUID change made/needed]: None

Comment 22

[Benoit Girard (:BenWa)]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #20)
> BenWa - 37 is marked as affected. Should we apply this backout to Aurora as
> well? If so, can you please request uplift?

Yes we need to. This is without a doubt a firm release blocker.

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8550581 [details] [diff] [review]
Back out a6db8f54f5aa#l21.208

Aurora+

Comment 24

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4811407460e6