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

VERIFIED FIXED in Firefox 47

Status

()

defect
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: alice0775, Assigned: bas.schouten)

Tracking

({power, regression})

47 Branch
mozilla48
Unspecified
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 unaffected, firefox47+ fixed, firefox48+ verified)

Details

(Whiteboard: gfx-noted, URL)

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
[Tracking Requested - why for this release]: Browser eats up CPU power without reasonable reason
(Reporter)

Updated

3 years ago
OS: Unspecified → Windows 7
(Reporter)

Updated

3 years ago
Summary: 100% CPU core usage when hovering the mouse over a tab → 100% CPU core usage when hovering the mouse over a tab (Windows7 classic)
(Reporter)

Comment 2

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

Regressed by: Bug 1232042
Blocks: 1232042
Component: Tabbed Browser → Graphics
Flags: needinfo?(jmuizelaar)
Flags: needinfo?(bas)
Keywords: regression
Product: Firefox → Core
Version: 48 Branch → 47 Branch
(Reporter)

Comment 3

3 years ago
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
(Assignee)

Comment 5

3 years ago
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.
Flags: needinfo?(bas)
(Reporter)

Comment 6

3 years ago
It also occurs when tooltip of title attribute pops up.
Whiteboard: gfx-noted
Tracked as this is a new regression for Fx47.
Flags: needinfo?(jmuizelaar)
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.
Assignee: nobody → bas
(Assignee)

Comment 9

3 years ago
(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?
Flags: needinfo?(jmuizelaar)
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.
Flags: needinfo?(jmuizelaar) → needinfo?(bas)
Comment hidden (offtopic)
(Assignee)

Comment 12

3 years ago
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.
Flags: needinfo?(bas)
(Reporter)

Comment 13

3 years ago
The fps continue to count when setting layers.acceleration.draw-fps = true :(.
The fps should not increase.
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.
Summary: 100% CPU core usage while tooltip, context menu and menupopup is displayed (Windows7 classic & High Contrast) → 100% CPU core usage while tooltip, context menu and menupopup is displayed (Windows7 classic & High Contrast) with D3D11 compositor
More info I can gather?
Flags: needinfo?(bas)
(Assignee)

Comment 16

3 years ago
(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.
Flags: needinfo?(bas)
(Assignee)

Comment 18

3 years ago
(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/bschouten@mozilla.com-d5df5c6f569eebf4cdd4630bbbd403fa0c9a9b79/
Flags: needinfo?(milan)
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?
Attachment #8738831 - Flags: review?(jmuizelaar)
Unsafe patch if the function pointer is null non-withstanding, this patch does fix the problem for me.
Flags: needinfo?(milan)
(Assignee)

Comment 21

3 years ago
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/
Attachment #8738831 - Flags: review- → review?(jmuizelaar)
(Assignee)

Comment 22

3 years ago
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/
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?
(Assignee)

Comment 24

3 years ago
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/
Attachment #8738831 - Attachment description: MozReview Request: Bug 1256547: When the DWM is disabled don't force presentation. r=jrmuizel → MozReview Request: Bug 1256547: When the DWM is disabled don't force presentation. r=milan
Attachment #8738831 - Flags: review?(jmuizelaar) → review?(milan)
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.
Attachment #8738831 - Flags: review?(milan) → review+
(Assignee)

Comment 26

3 years ago
(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.
Flags: needinfo?(milan)
I don't live in this code nearly as much as you do, so whatever works for you :)
Flags: needinfo?(milan)
(Assignee)

Comment 28

3 years ago
(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 30

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9defa3cb804c
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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]:
Attachment #8738831 - Flags: approval-mozilla-aurora?
Alice0775 White, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(alice0775)
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+
Attachment #8738831 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Reporter)

Comment 34

3 years ago
(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
Flags: needinfo?(alice0775)
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)
Flags: needinfo?(bas)
(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. :)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.