1256547 - 100% CPU core usage while tooltip, context menu and menupopup is displayed (Windows7 classic & High Contrast) with D3D11 compositor

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Alice0775 White]

Build Identifier:
https://hg.mozilla.org/mozilla-central/rev/f0c0480732d36153e8839c7f17394d45f679f87d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0 ID:20160314030215

Reproducible: always


Steps To Reproduce:
1. Open a tab (about:home is enough to reproduce)
2. hovering the mouse over a tab. And do not move mouse

Actual Results:
100% CPU core usage.

Expected Results:
CPU usage should not be increased.

Comment 1

[Alice0775 White]

[Tracking Requested - why for this release]: Browser eats up CPU power without reasonable reason

Comment 2

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4e56796fa59b7be8d0ebc717f0a6db410cb3a0fa&tochange=2dd26535e73c160f9ce478cd0561b7722042c0d3

Regressed by: Bug 1232042

Comment 3

[Alice0775 White]

The problem seems to occur on Windows7 Classic only.

Graphics
--------

Adapter Description: AMD Radeon HD 6450
Adapter Drivers: aticfx64 aticfx64 aticfx64 aticfx32 aticfx32 aticfx32 atiumd64 atidxx64 atidxx64 atiumdag atidxx32 atidxx32 atiumdva atiumd6a atitmm64
Adapter RAM: 1024
Asynchronous Pan/Zoom: wheel input enabled; touch input enabled
ClearType Parameters: Gamma: 2200 Pixel Structure: R ClearType Level: 100 Enhanced Contrast: 300
Device ID: 0x6779
Direct2D Enabled: true
DirectWrite Enabled: true (6.2.9200.17568)
Driver Date: 7-28-2015
Driver Version: 15.200.1062.1003
GPU #2 Active: false
GPU Accelerated Windows: 1/1 Direct3D 11 (OMTC)
Subsys ID: 23111787
Supports Hardware H264 Decoding: Yes
Vendor ID: 0x1002
WebGL Renderer: Google Inc. -- ANGLE (AMD Radeon HD 6450 Direct3D11 vs_5_0 ps_5_0)
windowLayerManagerRemote: true
AzureCanvasAccelerated: 0
AzureCanvasBackend: direct2d 1.1
AzureContentBackend: direct2d 1.1
AzureFallbackCanvasBackend: cairo

Comment 4

[Alice0775 White]

Profiler: https://cleopatra.io/#report=e2d96e562da7ed51b415c3c6018a1dff8556f94d

Comment 5

[Bas Schouten (:bas.schouten)]

This sounds like somehow we receive continuous WM_PAINT messages when we hover over a tab. That is probably the 'real' bug here and a good thing to know regardless. I'll have a look at this.

Comment 6

[Alice0775 White]

It also occurs when tooltip of title attribute pops up.

Comment 7

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked as this is a new regression for Fx47.

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

Interesting that it happens on Windows 7 only; we need the fix in bug 1232042, but if we really get stuck we may want to consider making that fix for later versions of Windows only, as we've mostly seen those problems on Windows 10.

Comment 9

[Bas Schouten (:bas.schouten)]

(In reply to Milan Sreckovic [:milan] from comment #8)
> Interesting that it happens on Windows 7 only; we need the fix in bug
> 1232042, but if we really get stuck we may want to consider making that fix
> for later versions of Windows only, as we've mostly seen those problems on
> Windows 10.

I don't have Windows 7 machines anymore, could you check this one one of our Win7 machines Jeff?

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

I do see it on Windows 7, but only on the classic theme (and the high contrast), not on random aero ones.  I'd guess it's related to the tooltips that show up when we hover over the tab.

I'm not convinced this is the highest urgency, it doesn't seem to be a common workflow, but it would be good to understand what's going on.

Comment 11

[Alice0775 White]

コンテキストメニューが表示されている間

Comment 12

[Bas Schouten (:bas.schouten)]

Hmm.. It shouldn't be hard to figure this out with a machine that reproduces it. It's probably just firing continuous WM_PAINT events. This may actually be exposing an underlying bug.

Comment 13

[Alice0775 White]

The fps continue to count when setting layers.acceleration.draw-fps = true :(.
The fps should not increase.

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

I can confirm that nsWindow::ProcessMessage gets called with WM_PAINT continuously in the Windows Classic/High Contrast scenario, but only with D3D11 compositor.  D3D9 or basic don't get in this trouble.

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

More info I can gather?

Comment 16

[Bas Schouten (:bas.schouten)]

(In reply to Milan Sreckovic [:milan] from comment #14)
> I can confirm that nsWindow::ProcessMessage gets called with WM_PAINT
> continuously in the Windows Classic/High Contrast scenario, but only with
> D3D11 compositor.  D3D9 or basic don't get in this trouble.

So, my guess is, that without the DWM, a Present call will trigger a WM_PAINT when it occurs outside of the WM_PAINT message. The best thing to do is probably to disable the ForcePresent call when the DWM is disabled. I'm going to create a try build for you to test. One question is if we're drawing other stuff ineffectively even without the ForcePresent if the DWM is disabled, but that doesn't seem like something we really need to worry about for now.

Comment 17

[Bas Schouten (:bas.schouten)]

Attachment: MozReview Request: Bug 1256547: When the DWM is disabled don't force presentation. r=milan

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

Comment 18

[Bas Schouten (:bas.schouten)]

(In reply to Milan Sreckovic [:milan] from comment #15)
> More info I can gather?

Try build for verification here:

https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-d5df5c6f569eebf4cdd4630bbbd403fa0c9a9b79/

Comment 19

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8738831 [details]
MozReview Request: Bug 1256547: When the DWM is disabled don't force presentation. r=milan

https://reviewboard.mozilla.org/r/44717/#review41461

::: widget/windows/nsWindowGfx.cpp:531
(Diff revision 1)
>        case LayersBackend::LAYERS_CLIENT:
>          {
>            result = listener->PaintWindow(
>              this, LayoutDeviceIntRegion::FromUnknownRegion(region));
> -          if (!gfxEnv::DisableForcePresent()) {
> +          BOOL dwmEnabled;
> +          if (!gfxEnv::DisableForcePresent() && !FAILED(WinUtils::dwmIsCompositionEnabledPtr(&dwmEnabled)) && dwmEnabled) {

This will crash if that function ptr is null.  There's a DwmCompositionEnabled helper but it's still not safe to call. Can you clean that stuff up?

Comment 20

[Milan Sreckovic [:milan] (needinfo for best results)]

Unsafe patch if the function pointer is null non-withstanding, this patch does fix the problem for me.

Comment 21

[Bas Schouten (:bas.schouten)]

Comment on attachment 8738831 [details]
MozReview Request: Bug 1256547: When the DWM is disabled don't force presentation. r=milan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44717/diff/1-2/

Comment 22

[Bas Schouten (:bas.schouten)]

Comment on attachment 8738831 [details]
MozReview Request: Bug 1256547: When the DWM is disabled don't force presentation. r=milan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44717/diff/2-3/

Comment 23

[Milan Sreckovic [:milan] (needinfo for best results)]

https://reviewboard.mozilla.org/r/44717/#review41549

::: gfx/thebes/gfxWindowsPlatform.cpp:2599
(Diff revision 3)
>  }
>  
> -static bool
> -DwmCompositionEnabled()
> +bool
> +gfxWindowsPlatform::DwmCompositionEnabled()
>  {
> +  if (!IsVistaOrLater()) {

This would have a (positive) effect of disabling force present on XP, which could take care of one of the other bugs.

::: widget/windows/nsWindowGfx.cpp:37
(Diff revision 3)
>  #include "mozilla/gfx/2D.h"
>  #include "mozilla/gfx/DataSurfaceHelpers.h"
>  #include "mozilla/gfx/Tools.h"
>  #include "mozilla/RefPtr.h"
>  #include "mozilla/UniquePtrExtensions.h"
> +#include "mozilla/WindowsVersion.h"

Do we still need this and the "using mozilla::IsVistaOrLater" below?

::: widget/windows/nsWindowGfx.cpp:532
(Diff revision 3)
>          break;
>        case LayersBackend::LAYERS_CLIENT:
>          {
>            result = listener->PaintWindow(
>              this, LayoutDeviceIntRegion::FromUnknownRegion(region));
> -          if (!gfxEnv::DisableForcePresent()) {
> +          BOOL dwmEnabled;

Don't need this anymore, right?

Comment 24

[Bas Schouten (:bas.schouten)]

Comment on attachment 8738831 [details]
MozReview Request: Bug 1256547: When the DWM is disabled don't force presentation. r=milan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44717/diff/3-4/

Comment 25

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8738831 [details]
MozReview Request: Bug 1256547: When the DWM is disabled don't force presentation. r=milan

https://reviewboard.mozilla.org/r/44717/#review41999

::: gfx/thebes/gfxWindowsPlatform.cpp:2599
(Diff revision 4)
>  }
>  
> -static bool
> -DwmCompositionEnabled()
> +bool
> +gfxWindowsPlatform::DwmCompositionEnabled()
>  {
> +  if (!IsVistaOrLater()) {

Perhaps it's cleaner to just check the function pointer, instead of assuming we know what WinUtils is going to do. The way the code is right now, if WinUtils changes when it sets that pointer, we have ot make a matching change here.  If we just replaced:

if (!IsVistOrLater()) {
  return false;
}
MOZ_ASSERT(WinUtils::dwmIsCompositionEnabledPtr);

with

if (!WinUtils::dwmIsCompositionEnabledPtr) {
  return false;
}

it still works, and we remove the close coupling.  And we could add the assert in WinUtils if we know that we should always have this function for Vista or later.

Comment 26

[Bas Schouten (:bas.schouten)]

(In reply to Milan Sreckovic [:milan] from comment #25)
> Comment on attachment 8738831 [details]
> MozReview Request: Bug 1256547: When the DWM is disabled don't force
> presentation. r=milan
> 
> https://reviewboard.mozilla.org/r/44717/#review41999
> 
> ::: gfx/thebes/gfxWindowsPlatform.cpp:2599
> (Diff revision 4)
> >  }
> >  
> > -static bool
> > -DwmCompositionEnabled()
> > +bool
> > +gfxWindowsPlatform::DwmCompositionEnabled()
> >  {
> > +  if (!IsVistaOrLater()) {
> 
> Perhaps it's cleaner to just check the function pointer, instead of assuming
> we know what WinUtils is going to do. The way the code is right now, if
> WinUtils changes when it sets that pointer, we have ot make a matching
> change here.  If we just replaced:
> 
> if (!IsVistOrLater()) {
>   return false;
> }
> MOZ_ASSERT(WinUtils::dwmIsCompositionEnabledPtr);
> 
> with
> 
> if (!WinUtils::dwmIsCompositionEnabledPtr) {
>   return false;
> }
> 
> it still works, and we remove the close coupling.  And we could add the
> assert in WinUtils if we know that we should always have this function for
> Vista or later.

We already crash in this function -currently- (this is called on startup), if that function wasn't present though. So we make the assumption in various places the DWM functions be available everywhere where we have Vista or later, it seems a little strange to deviate from this approach in this specific instance although if you feel strongly I don't mind.

Comment 27

[Milan Sreckovic [:milan] (needinfo for best results)]

I don't live in this code nearly as much as you do, so whatever works for you :)

Comment 28

[Bas Schouten (:bas.schouten)]

(In reply to Milan Sreckovic [:milan] from comment #27)
> I don't live in this code nearly as much as you do, so whatever works for
> you :)

I've decided to land this, the situation where this could crash is if we somehow fail to find the DWM DLL in WinUtils::Initialize(), if that happens I'd really like to know about it since that would mean something is seriously weird, so let's keep an eye on if this crash does occur. But it seems unlikely since, like I said, we should -already- crash when that is the case.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9defa3cb804c

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9defa3cb804c

Comment 31

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8738831 [details]
MozReview Request: Bug 1256547: When the DWM is disabled don't force presentation. r=milan

Approval Request Comment
[Feature/regressing bug #]: 1232042
[User impact if declined]: Other than the mentioned case, it is possible pre-Windows 7 systems could get in trouble with the patch from bug 1232042.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Low, this limits the reach of bug 1232042.
[String/UUID change made/needed]:

Comment 32

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Alice0775 White, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 33

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8738831 [details]
MozReview Request: Bug 1256547: When the DWM is disabled don't force presentation. r=milan

Recent regression, fixes unnecessary CPU spike in said scenario, Aurora47+

Comment 34

[Alice0775 White]

(In reply to Ritu Kothari (:ritu) from comment #32)
> Alice0775 White, could you please verify this issue is fixed as expected on
> a latest Nightly build? Thanks!

The problem is no longer reproduced on latest Nightly.

https://hg.mozilla.org/mozilla-central/rev/afd82f887093e5e9e4015115ca5795ec82a6f732
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0 ID:20160415030231

Comment 35

[Carsten Book [:Tomcat]]

has problems to apply for uplift:

grafting 339368:9defa3cb804c "Bug 1256547: When the DWM is disabled don't force presentation. r=milan"
merging gfx/thebes/gfxWindowsPlatform.cpp
merging gfx/thebes/gfxWindowsPlatform.h
merging widget/windows/nsWindowGfx.cpp
warning: conflicts while merging widget/windows/nsWindowGfx.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

Comment 36

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Alice0775 White from comment #34)
> (In reply to Ritu Kothari (:ritu) from comment #32)
> > Alice0775 White, could you please verify this issue is fixed as expected on
> > a latest Nightly build? Thanks!
> 
> The problem is no longer reproduced on latest Nightly.
> 
> https://hg.mozilla.org/mozilla-central/rev/
> afd82f887093e5e9e4015115ca5795ec82a6f732
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0
> ID:20160415030231

Awesome! Thank you as always for such a prompt verification. :)

Comment 37

[Bas Schouten (:bas.schouten)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/1a229bf7891d