Closed Bug 1068177 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Storage: IndexedDB, defect)

All
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
}
Flags: needinfo?(bent.mozilla)
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?)
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.
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...
Flags: needinfo?(bent.mozilla)
Cool; here's a patch to remove the assertion, then.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #8490910 - Flags: review?(bent.mozilla)
Blocks: 1068113
No longer blocks: 1068175
OS: Linux → Windows XP
Hardware: x86_64 → All
Attachment #8490910 - Flags: review?(bent.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/3774cabb49f2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
FWIW bug 994190 was backed out, but when it relands it will include this fix.
You need to log in before you can comment on or make changes to this bug.