Closed Bug 1121811 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Plug-ins, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox35 --- unaffected
firefox36 --- unaffected
firefox37 + fixed
firefox38 + fixed

People

(Reporter: BenWa, Assigned: handyman)

References

()

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(2 files)

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
Flags: needinfo?(joshmoz)
David, Markus - can one of you take a look at this?
Flags: needinfo?(joshmoz)
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
Attachment #8550576 - Flags: review?(joshmoz)
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.
Flags: needinfo?(davidp99)
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)
Flags: needinfo?(davidp99)
Attachment #8550581 - Flags: review?(davidp99) → review?(joshmoz)
Attachment #8550576 - Flags: 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 :-)
https://hg.mozilla.org/mozilla-central/rev/b5842b906435
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
BenWa - 37 is marked as affected. Should we apply this backout to Aurora as well? If so, can you please request uplift?
Flags: needinfo?(bgirard)
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
Flags: needinfo?(bgirard)
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+
Blocks: 1130435
Blocks: 1158439
Depends on: 1290505
You need to log in before you can comment on or make changes to this bug.