Benoit Girard (:BenWa) Reporter
	Description • 10 years ago

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.

	Benoit Girard (:BenWa) Reporter
	Comment 1 • 10 years ago

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

	Benoit Girard (:BenWa) Reporter
	Comment 2 • 10 years ago

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

	Benoit Girard (:BenWa) Reporter
	Comment 3 • 10 years ago

good 0cf461e62ce5
bad 5288b15d22de

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

Guessing bug 1092630 for now.

	Josh Aas
	Comment 4 • 10 years ago

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

	Markus Stange [:mstange]
	Comment 5 • 10 years ago

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

	Benoit Girard (:BenWa) Reporter
	Comment 6 • 10 years ago

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

	David Parks [:handyman] Assignee
	Comment 7 • 10 years ago

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.

	Benoit Girard (:BenWa) Reporter
	Comment 8 • 10 years ago

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.

	Benoit Girard (:BenWa) Reporter
	Comment 9 • 10 years ago

Ahh, looks like you beat me to it.

Thanks!

	Benoit Girard (:BenWa) Reporter
	Comment 10 • 10 years ago

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.

	Benoit Girard (:BenWa) Reporter
	Comment 11 • 10 years ago

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

	David Parks [:handyman] Assignee
	Comment 12 • 10 years ago

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)

	Josh Aas
	Comment 13 • 10 years ago

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!

	Benoit Girard (:BenWa) Reporter
	Comment 14 • 10 years ago

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

How do I run those?

	Josh Aas
	Comment 15 • 10 years ago

(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).

	Steven Michaud [:smichaud] (Retired)
	Comment 16 • 10 years ago

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 :-)

	Benoit Girard (:BenWa) Reporter
	Comment 17 • 9 years ago

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

	Benoit Girard (:BenWa) Reporter
	Comment 18 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 19 • 9 years ago

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

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 20 • 9 years ago

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

	Benoit Girard (:BenWa) Reporter
	Comment 21 • 9 years ago

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

	Benoit Girard (:BenWa) Reporter
	Comment 22 • 9 years ago

(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.

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 23 • 9 years ago

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

Aurora+

	Ryan VanderMeulen [:RyanVM]
	Comment 24 • 9 years ago

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