1068177 - MSVC build warning: "actorsparent.cpp(5365) : warning C4722: 'mozilla::dom::indexedDB::`anonymous namespace'::DEBUGThreadSlower::~DEBUGThreadSlower' : destructor never returns, potential memory leak"

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Daniel Holbert [:dholbert]]

This Try run (with FAIL_ON_WARNINGS added back in dom/indexedDB)...
  https://tbpl.mozilla.org/php/getParsedLog.php?id=48213506&tree=Try
shows that we hit this build warning on Windows (MSVC):
{
c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\dom\indexeddb\actorsparent.cpp(5365) : warning C4722: 'mozilla::dom::indexedDB::`anonymous namespace'::DEBUGThreadSlower::~DEBUGThreadSlower' : destructor never returns, potential memory leak
}

Comment 1

[Daniel Holbert [:dholbert]]

The destructor is implemented as follows:

> 5365   ~DEBUGThreadSlower()
> 5366   {
> 5367     AssertIsOnBackgroundThread();
> 5368     MOZ_ASSERT(kDEBUGThreadSleepMS);
> 5369   }

I'm guessing MSVC says it never returns because kDEBUGThreadSleepMS is declared as:
> const uint32_t kDEBUGThreadSleepMS = 0;

...which means that MOZ_ASSERT is guaranteed to abort (and the compiler can detect that).

bent, is this assertion really asserting what it means to?  (and/or, is kDEBUGThreadSleepMS really supposed to be 0?)

Comment 2

[Daniel Holbert [:dholbert]]

Ah, it looks like this class (DEBUGThreadSlower) is only ever instantiated if kDEBUGThreadSleepMS is set to something nonzero.  So, the whole class is unreachable code, unless you modify the source to make kDEBUGThreadSleepMS nonzero.

That makes a bit more sense. (and it's presumably why we're not *really* failing this MOZ_ASSERT -- it's never reached)

Assuming we want to keep this code (despite it being unreachable) for debugging purposes: could we just drop the assertion from the destructor, to make MSVC shupt up?  We've got the same assertion in the constructor, so given that 'kDEBUGThreadSleepMS' is constant, there's not really any value to having it in the destructor as well, aside from as documentation.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Yeah, removing the assertion from the destructor, or quieting the warning there, seem acceptable.

This code is awesome for finding races so I really don't want to get rid of it...

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: fix v1: remove assertion

Cool; here's a patch to remove the assertion, then.

Comment 5

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3774cabb49f2

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3774cabb49f2

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

FWIW bug 994190 was backed out, but when it relands it will include this fix.