1762839 - Some xpcom files fail clang thread safety analysis on Windows

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

When attempting to build Fx on Windows with thread safety annotations enabled, I get the following warnings (with warnings as errors):

 0:10.14 C:/projects/mozilla/mozilla-unified/xpcom/threads/nsProcessCommon.cpp(123,43): error: reading variable 'mProcess' requires holding mutex 'process->mLock' [-Werror,-Wthread-safety-analysis]
 0:10.15   dwRetVal = WaitForSingleObject(process->mProcess, INFINITE);
 0:10.15                                           ^
 0:10.15 C:/projects/mozilla/mozilla-unified/xpcom/threads/nsProcessCommon.cpp(125,37): error: reading variable 'mProcess' requires holding mutex 'process->mLock' [-Werror,-Wthread-safety-analysis]
 0:10.15     if (GetExitCodeProcess(process->mProcess, &exitCode) == FALSE) {
 0:10.15                                     ^
 0:10.15 C:/projects/mozilla/mozilla-unified/xpcom/threads/nsProcessCommon.cpp(365,5): error: writing variable 'mProcess' requires holding mutex 'mLock' exclusively [-Werror,-Wthread-safety-analysis]
 0:10.15     mProcess = processInfo.hProcess;
 0:10.15     ^
 0:10.15 C:/projects/mozilla/mozilla-unified/xpcom/threads/nsProcessCommon.cpp(390,5): error: writing variable 'mProcess' requires holding mutex 'mLock' exclusively [-Werror,-Wthread-safety-analysis]
 0:10.16     mProcess = sinfo.hProcess;
 0:10.16     ^
 0:10.16 C:/projects/mozilla/mozilla-unified/xpcom/threads/nsProcessCommon.cpp(393,23): error: reading variable 'mProcess' requires holding mutex 'mLock' [-Werror,-Wthread-safety-analysis]
 0:10.17   mPid = GetProcessId(mProcess);
 0:10.17                       ^
 0:10.17 In file included from Unified_cpp_xpcom_base0.cpp:29:
 0:10.17 C:/projects/mozilla/mozilla-unified/xpcom/base/AvailableMemoryWatcherWin.cpp(264,23): error: releasing mutex 'mMutex' that was not held [-Werror,-Wthread-safety-analysis]
 0:10.17       MutexAutoUnlock unlock(mMutex);

I have a patch to mitigate these locally which I'll get uploaded in a moment.

Comment 1

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1762839 - Fix Win specific thread annotations in nsProcessCommon. r?jesup!

This adds in some locks that appear to be missing for the Windows case. It also
adds some locks which I think are missing for the non-Win case where mProcess
still exists in nsProcessCommon (though I'm less confident of that case).

Comment 2

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1762839 - Adjust locking in AvailableMemoryWatcherWin to appease thread safety checker. r?jesup,nika

This stops the checker complaining that locks aren't held by adding further
annotations and doing some locking slightly earlier.

I've tried to adhere to the convention of passing a lock when a function
requires it, though I don't believe we have to do so to make the checker happy.

Depends on D142789

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c24aa64a2ef
Fix Win specific thread annotations in nsProcessCommon. r=jesup,rkraesig
https://hg.mozilla.org/integration/autoland/rev/d7d98f74a261
Adjust locking in AvailableMemoryWatcherWin to appease thread safety checker. r=nika

Comment 4

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

FWIW, try run prior to landing - https://treeherder.mozilla.org/jobs?repo=try&revision=74e6e62eed3d072d2274ab0c8a06b946387296f4

Comment 5

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/0c24aa64a2ef
https://hg.mozilla.org/mozilla-central/rev/d7d98f74a261