Closed Bug 1121811 Opened 5 years ago Closed 5 years ago
[Power] Flash on mac is triggering 60 FPS recomposites on Nightly
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.
Better test page: http://www.fastswf.com/
Regression range: -g 2014-12-11 -b 2014-12-12
good 0cf461e62ce5 bad 5288b15d22de http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0cf461e62ce5&tochange=5288b15d22de Guessing bug 1092630 for now.
Depends on: 1092630
David, Markus - can one of you take a look at this?
I can, if Benoit doesn't beat me to it.
[Tracking Requested - why for this release]: Regressed in 37. It should also block 37.
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.
Assignee: nobody → davidp99
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.
Assignee: davidp99 → nobody
Ahh, looks like you beat me to it. Thanks!
Assignee: nobody → davidp99
Component: Graphics → Plug-ins
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.
This fixes the problem locally by removing the bad hunk. Do you know why this change was originally made?
Attachment #8550581 - Flags: review?(davidp99)
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)
Attachment #8550581 - Flags: review?(davidp99) → review?(joshmoz)
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!
Attachment #8550581 - Flags: review?(joshmoz) → review+
(In reply to Josh Aas (Mozilla Corporation) from comment #13) > 3) Local HiDPI tests pass How do I run those?
(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).
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 :-)
BenWa - 37 is marked as affected. Should we apply this backout to Aurora as well? If so, can you please request uplift?
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
Attachment #8550581 - Flags: approval-mozilla-aurora?
(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 on attachment 8550581 [details] [diff] [review] Back out a6db8f54f5aa#l21.208 Aurora+
Attachment #8550581 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.