Add static thread-safety lock/mutex analysis based on clang GUARDED_BY()
Categories
(Core :: XPCOM, task, P3)
Tracking
()
People
(Reporter: jesup, Assigned: jesup, NeedInfo)
References
(Depends on 5 open bugs)
Details
(Keywords: leave-open, sec-audit)
Attachments
(58 files, 79 obsolete files)
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review |
See also bug 1098387 (I didn't want to flag that we'd started work on that, since this could lead to discovery of a bunch of bugs). This is the master bug for developing and eventually landing thread-safety annotations based on clang thread-safety tools (http://clang.llvm.org/docs/ThreadSafetyAnalysis.html). Due to the sensitivity of the bugs that may be found via this, this bug is a sec-bug and many of the spinoffs will be as well. We'll want to get this working, and than land fixes for bugs immediately found by making *local* (NOT TRY) runs before landing this code in the tree itself. Since this is purely static analysis, we can clean up the tree with this to a large extent before landing all the patches for GUARDED_BY. They *will* bitrot some, so eventually we'll want to bite the bullet and land at least the GUARDED_BY()/etc patches, but not land the annotated Mutex/etc patches (perhaps - a good hacker knowing we're landing GUARDED_BY patches might recreate my work on Mutex/CondVar/Monitor/etc. Once we're public on it we'll want to push to get all the known-danger-spots clean (and uplifts done) ASAP, so the more we can do before the better. Note: this may not be easy to do, since there are plenty of corner-cases this analysis can't handle, and may require us to annotate to not try.
Comment 1•8 years ago
|
||
This was on the list of things that I was planning to do, I'm happy to see that you're working on it. Thanks Randell, let me know if you hit any issues!
Assignee | ||
Comment 2•8 years ago
|
||
mostly functions; enough to see some results. Several bits turned off (mostly around use of AssertCurrentThreadOwns(), especially in Monitor). Build with CC=clang, CXX=clang++, CFLAGS/CXXFLAGS="-Wthread-safety". I applied some GUARDED_BY()s to MediaStreamGraphs mMutex to see what happens.
Comment 3•8 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #0) > than land fixes for bugs immediately found by making *local* (NOT TRY) > runs Nice paranoia, but historically we haven't required developers to exercise this care when tryservering security patches. Normal caution has been to not include bug summary in the commit, perhaps mix it in with another fix for obscurity, and sometimes to delay the try-pushing as long as possible. But it hasn't been to not push, and can't be so many platforms/compilers. Given how finicky threads/ownership is, and the likely increase in difficulty to exploit, I don't see reason to warrant more caution than normal when tryservering stuff. Delay the trypush until you're otherwise sure the patch is correct? Sure, great. Not push at all? I think we have to wait for bug 1136954 to deal with not exposing security issues too early via try. :-\
Assignee | ||
Comment 4•8 years ago
|
||
Ok. I have some ideas about a more-safe Try I'll open a bug for. For *this* bug, let's not push to try. It gains us little since it's static analysis that we can easily do on Linux/Linux-vm. FYI, Google's paper: http://static.googleusercontent.com/media/research.google.com/en/us/pubs/archive/42958.pdf
Assignee | ||
Comment 5•8 years ago
|
||
progress - most warnings now are for AssertCurrentThreadOwns, and those can generally be solved by making the function definition (usually in the .h file) have EXCLUSIVE_LOCKS_REQUIRED(mMutex) (or mMonitor). Still with very few GUARDED_BYs; I want to get the basics clean before I get deep into that.
Assignee | ||
Updated•8 years ago
|
Updated•8 years ago
|
![]() |
||
Comment 6•7 years ago
|
||
Looking through the patch, I wonder if it's not just as much work to move the lock annotations into the type system: https://blog.mozilla.org/nfroyd/2015/09/17/compiler-enforced-locked-accesses/ That way, we'd get checking everywhere, at an (epsilon) small performance loss (since we're now taking up an argument register for a proof that we own the lock). WDYT?
Assignee | ||
Comment 7•7 years ago
|
||
I'd want to be sure we aren't shooting ourselves in the foot perf/footprint-wise; I'm always hesitant to predict how a compiler will react to heavily-templated/classed setups. (You may have more direct knowledge of it in current generation compilers; I have memories of people being burned by over-templating and so am cautious.) A compiler that knows all callers to a function could optimize-away unused parameters, but that's not so easy in practice; though maybe for PGO-optimized builds of xul (external symbol suppression) it might work. Not betting on it. One of the stumbling blocks for this patchset are all the existing lock assertions; they need to generally have an lock annotation added to the function (or disable checking in the function); it's too bad you can't do one thing that provides both - the parameter passing would replace most lock assertions and provide static checking. One practical downside of your suggestion is that a LOT of lines of code need to change to access guarded values, such as in the first-part of your example. The function-level stuff is a lot less intrusive than mFlags.Value(lock) and mFlags.Assign(lock, ...) everywhere, compared to a single static GUARDED_BY(lock) on the definition of mFlags. So at least for member/etc var access, I think it is a lot more work to move to parameter-based checking, and it has more ongoing impact on how code gets written. (though perhaps I missed something...). Also, we're starting to import code using Clang static analysis, there's a plus being able to leverage that and verify we didn't break it in any way. That doesn't mean we can't have both, of course.
Assignee | ||
Comment 8•7 years ago
|
||
I have a mostly-working patchset, and applied GUARDED_BY's to a few objects - and not surprisingly found some more-or-less ok issues (not holding the lock when initializing, which gets flagged but is in this case safe), and some not-ok issues (grabbing the lock after the first access to something used from multiple threads, etc). I'll file some followup sec bugs for those. There are lots of rough edges, and I turned off the EXCLUSIVE_LOCKS_REQUIRED() on AssertCurrentThreadOwns() for now - it should be turned on, but that requires going through and annotating functions using it with EXCLUSIVE_LOCKS_REQUIRED(). I did some, but there are several hundred in the tree (and some variants). Also I haven't added LOCK_RETURNED()'s to GetLock() functions, and various other improvements. However, even now, just running a build, then adding GUARDED_BY() to some vars currently marked in the source with a comment like "// mLock protects the following" (or equivalent) and rebuilding will pretty easily give you a list of any issues to check. Al - who do you want me to hand this off to?
Comment 9•7 years ago
|
||
Sylvestre, is this something your new static analysis person could look at?
Comment 10•7 years ago
|
||
Maybe if he gets some guidance. Randell, how are would it be for Andi and I to build this list ? And then to go through the list and fix the various issues?
Assignee | ||
Comment 11•7 years ago
|
||
I can show you the current patchset and how we extend it. Mostly it's adding GUARDED_BY() attributions, one file/dir at a time, and working to resolve all the AssertCurrentThreadOwns(). There are large numbers of false-positives from certain code sequences (most notably MutexAutoUnlock() isn't supported and causes kickouts). Real bugs often have a different error flagged, so as you turn it on for an area you can examine the errors flagged, and decide if they're real or not. Using this after first-pass checking would likely benefit from something I think some other tools do, which is to flag changes in the number of hits. (though it would help a ton if we could somehow make it ignore MutexAutoUnlocks and the like). I'm less worried about automating it as I am about being able to audit the tree to start with. I can help walk him through adding it to a few classes and checking the results, for example
Comment 12•7 years ago
|
||
This is just a merge with the latest from mozilla-centrall + some definistion inside the missing file in order to have the build working.
Comment 13•7 years ago
|
||
The missing file from the original patch was xpcom/glue/MutexThreadSafety.h, it's content has been copied from webrtc/base/thread_annotations.h
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 14•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 15•7 years ago
|
||
Assignee | ||
Comment 16•7 years ago
|
||
Assignee | ||
Comment 17•7 years ago
|
||
Assignee | ||
Comment 18•7 years ago
|
||
Assignee | ||
Comment 19•7 years ago
|
||
Assignee | ||
Comment 20•7 years ago
|
||
Assignee | ||
Comment 21•7 years ago
|
||
Assignee | ||
Comment 22•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Comment 23•7 years ago
|
||
This is WIP. I've upload just to make sure we're on the right track.
Assignee | ||
Updated•7 years ago
|
Comment 24•7 years ago
|
||
I haven't analyze the warnings yet, I'm planning to do it next week. Any feedback is appreciated.
Assignee | ||
Comment 25•7 years ago
|
||
Comment on attachment 8716216 [details] [diff] [review] GUARDED_BY_II It helps to check the 'patch' box for patches; then they can be reviewed in splinter
Assignee | ||
Comment 26•7 years ago
|
||
Comment on attachment 8716216 [details] [diff] [review] GUARDED_BY_II Review of attachment 8716216 [details] [diff] [review]: ----------------------------------------------------------------- Generally moving in the right direction; some minor things noted. ::: dom/media/EncodedBufferCache.h @@ +27,5 @@ > */ > class EncodedBufferCache > { > public: > + explicit EncodedBufferCache(uint32_t aMaxMemoryStorage) NO_THREAD_SAFETY_ANALYSIS I thought (and perhaps I mis-remembered) that constructors get no analysis by default. Looks like I'm right: "No checking inside constructors and destructors. -- The analysis currently does not do any checking inside constructors or destructors. In other words, every constructor and destructor is treated as if it was annotated with NO_THREAD_SAFETY_ANALYSIS." http://clang.llvm.org/docs/ThreadSafetyAnalysis.html ::: dom/media/MP3FrameParser.h @@ +112,1 @@ > MutexAutoLock mon(mLock); You can't require the lock, AND obtain the lock within the function. @@ +140,1 @@ > MutexAutoLock mon(mLock); ditto ::: dom/media/RtspMediaResource.cpp @@ +135,1 @@ > MonitorAutoLock monitor(mMonitor); You can't require the lock, AND obtain the lock within the function. @@ +474,5 @@ > > // static > void > RtspTrackBuffer::PlayoutDelayTimerCallback(nsITimer *aTimer, > + void *aClosure) EXCLUSIVE_LOCKS_REQUIRED(mMonitor) You can't require the lock, AND obtain the lock within the function. ::: dom/media/VideoFrameContainer.h @@ +96,5 @@ > // ImageContainer's current Image at. > // This can differ from the Image's actual size when the media resource > // specifies that the Image should be stretched to have the correct aspect > // ratio. > gfx::IntSize mIntrinsicSize; It looked like this was supposed to be under the mutex before, and perhaps other variables (I'd need to look more closely at the full file).
Assignee | ||
Comment 27•7 years ago
|
||
Comment on attachment 8715196 [details] [diff] [review] GUARDED_BY Review of attachment 8715196 [details] [diff] [review]: ----------------------------------------------------------------- In LOTS of places, you have EXCLUSIVE_LOCK_FUNCTION() where you want EXCLUSIVE_LOCKS_REQUIRED() (genenerally in functions calling AssertCurrentThreadOwns()) Overall moving in the right direction ::: dom/ipc/Blob.cpp @@ +1198,5 @@ > NS_INTERFACE_MAP_ENTRY(IPrivateRemoteInputStream) > NS_INTERFACE_MAP_END > > NS_IMETHODIMP > +RemoteInputStream::Close() NO_THREAD_SAFETY_ANALYSIS why no analysis here? @@ +1266,5 @@ > return NS_OK; > } > > NS_IMETHODIMP > +RemoteInputStream::Read(char* aBuffer, uint32_t aCount, uint32_t* aResult) NO_THREAD_SAFETY_ANALYSIS ditto @@ +4501,3 @@ > { > MOZ_ASSERT(sIDTableMutex); > sIDTableMutex->AssertNotCurrentThreadOwns(); This will fail; you can't assert the lock is required (statically) and assert that the lock is NOT owned by the current thread (dynamically) (and then go grab it). Also: "No checking inside constructors and destructors" http://clang.llvm.org/docs/ThreadSafetyAnalysis.html ::: dom/media/MediaTimer.cpp @@ +48,5 @@ > (void) rv; > } > > void > +MediaTimer::Destroy() NO_THREAD_SAFETY_ANALYSIS Right - Destroy functions often are invoked knowing there are no other people holding references to the object ::: dom/media/MediaTimer.h @@ +63,2 @@ > { > return !mCurrentTimerTarget.IsNull(); Why? Also, if this may be accessed by multiple threads, and IsNull doesn't lock (and it's not Atomic<>), then this is in fact wrong. @@ +63,5 @@ > { > return !mCurrentTimerTarget.IsNull(); > } > > + void CancelTimerIfArmed() NO_THREAD_SAFETY_ANALYSIS why? Generally, it's best to document why you're turning off analysis via a comment ::: dom/media/RtspMediaResource.cpp @@ +441,5 @@ > mMonitor.NotifyAll(); > } > > bool > +RtspTrackBuffer::IsBufferOverThreshold() NO_THREAD_SAFETY_ANALYSIS Why? ::: dom/media/systemservices/CamerasChild.cpp @@ +350,5 @@ > int > CamerasChild::AllocateCaptureDevice(CaptureEngine aCapEngine, > const char* unique_idUTF8, > const unsigned int unique_idUTF8Length, > + int& capture_id) NO_THREAD_SAFETY_ANALYSIS Standard question: why? @@ +611,5 @@ > MOZ_COUNT_DTOR(CamerasChild); > } > > webrtc::ExternalRenderer* CamerasChild::Callback(CaptureEngine aCapEngine, > + int capture_id) NO_THREAD_SAFETY_ANALYSIS ditto ::: dom/media/systemservices/CamerasChild.h @@ +86,1 @@ > Mutex().AssertCurrentThreadOwns(); Mutex() can be annotated as returning a mutex pointer - LOCK_RETURNED(). You can then here use EXCLUSIVE_LOCKS_REQUIRED(Mutex()) @@ +86,5 @@ > Mutex().AssertCurrentThreadOwns(); > return GetInstance().mCameras; > } > > + static nsCOMPtr<nsIThread>& Thread() NO_THREAD_SAFETY_ANALYSIS { ditto ::: dom/media/systemservices/CamerasParent.cpp @@ +405,5 @@ > return true; > } > > void > +CamerasParent::CloseEngines() NO_THREAD_SAFETY_ANALYSIS why? (but I know why - just document it) @@ +449,5 @@ > mWebRTCAlive = false; > } > > bool > +CamerasParent::EnsureInitialized(int aEngine) NO_THREAD_SAFETY_ANALYSIS Why? ::: dom/media/systemservices/CamerasParent.h @@ +103,2 @@ > || mDestroyed > || !mWebRTCAlive; }; Is this really known-safe? I would expect not, though maybe why you annotated it this way is it's used in known-safe functions to assert ::: dom/quota/ActorsParent.cpp @@ +4717,5 @@ > } > } > > CollectOriginsHelper::CollectOriginsHelper(mozilla::Mutex& aMutex, > + uint64_t aMinSizeToBeFreed) NO_THREAD_SAFETY_ANALYSIS shouldn't be needed: "no thread-safety analysis in constructors or destructors
Assignee | ||
Comment 28•7 years ago
|
||
Comment on attachment 8715196 [details] [diff] [review] GUARDED_BY Review of attachment 8715196 [details] [diff] [review]: ----------------------------------------------------------------- In LOTS of places, you have EXCLUSIVE_LOCK_FUNCTION() where you want EXCLUSIVE_LOCKS_REQUIRED() (genenerally in functions calling AssertCurrentThreadOwns()) Overall moving in the right direction
Comment 29•7 years ago
|
||
thanks for the help Jesup, I've modified the patches
Comment 30•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 31•7 years ago
|
||
Comment on attachment 8717931 [details] [diff] [review] 1207753_GUARDED_BY_II Please mark patches with the "patch" checkbox (or export them to bugzilla with bzexport)
Assignee | ||
Comment 32•7 years ago
|
||
parent is a285884cd669
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 33•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 34•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 35•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 36•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 37•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 38•7 years ago
|
||
AssertCurrentThreadOwns implies the method should be EXCLUSIVE_LOCKS_REQUIRED
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 39•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 40•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 41•7 years ago
|
||
Assignee | ||
Comment 42•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 43•7 years ago
|
||
Rebased, parent is b4b5fa96e8ee
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 44•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 45•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 46•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 47•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 48•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 49•7 years ago
|
||
AssertCurrentThreadOwns implies the method should be EXCLUSIVE_LOCKS_REQUIRED
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 50•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 51•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 52•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 53•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 54•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Updated•6 years ago
|
Assignee | ||
Comment 55•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 56•5 years ago
|
||
Note: the un-bitrot I'm finished was started last Oct, with a base of b4b5fa96e8ee. I plan to finish the unbitrot and push them up here
Assignee | ||
Updated•2 years ago
|
Updated•2 years ago
|
Assignee | ||
Updated•1 year ago
|
Assignee | ||
Updated•1 year ago
|
Assignee | ||
Updated•1 year ago
|
Assignee | ||
Updated•1 year ago
|
Assignee | ||
Updated•1 year ago
|
Assignee | ||
Updated•1 year ago
|
Assignee | ||
Updated•1 year ago
|
Assignee | ||
Updated•1 year ago
|
Assignee | ||
Updated•1 year ago
|
Assignee | ||
Updated•1 year ago
|
Assignee | ||
Updated•1 year ago
|
Assignee | ||
Comment 57•1 year ago
|
||
Assignee | ||
Comment 58•1 year ago
|
||
Depends on D130571
Assignee | ||
Comment 59•1 year ago
|
||
Depends on D130572
Assignee | ||
Comment 60•1 year ago
|
||
Depends on D130573
Assignee | ||
Comment 61•1 year ago
|
||
Depends on D130574
Assignee | ||
Comment 62•1 year ago
|
||
Depends on D130575
Assignee | ||
Comment 63•1 year ago
|
||
Depends on D130576
Assignee | ||
Comment 64•1 year ago
|
||
Depends on D130577
Assignee | ||
Comment 65•1 year ago
|
||
Depends on D130578
Assignee | ||
Comment 66•1 year ago
|
||
Depends on D130579
Assignee | ||
Comment 67•1 year ago
|
||
Depends on D130580
Assignee | ||
Comment 68•1 year ago
|
||
Depends on D130581
Assignee | ||
Comment 69•1 year ago
|
||
Depends on D130582
Assignee | ||
Comment 70•1 year ago
|
||
Depends on D130583
Assignee | ||
Comment 71•1 year ago
|
||
Depends on D130584
Assignee | ||
Comment 72•1 year ago
|
||
Depends on D130585
Assignee | ||
Comment 73•1 year ago
|
||
Depends on D130586
Assignee | ||
Comment 74•1 year ago
|
||
Depends on D130587
Assignee | ||
Comment 75•1 year ago
|
||
Depends on D130588
Assignee | ||
Comment 76•1 year ago
|
||
Depends on D130589
Assignee | ||
Comment 77•1 year ago
|
||
Depends on D130590
Assignee | ||
Comment 78•1 year ago
|
||
Depends on D130591
Assignee | ||
Comment 79•1 year ago
|
||
Depends on D130592
Assignee | ||
Comment 80•1 year ago
|
||
Depends on D130593
Assignee | ||
Comment 81•1 year ago
|
||
Depends on D130594
Assignee | ||
Comment 82•1 year ago
|
||
Depends on D130595
Assignee | ||
Comment 83•1 year ago
|
||
Depends on D130596
Assignee | ||
Comment 84•1 year ago
|
||
Depends on D130597
Assignee | ||
Comment 85•1 year ago
|
||
Depends on D130598
Assignee | ||
Comment 86•1 year ago
|
||
Depends on D130599
Assignee | ||
Comment 87•1 year ago
|
||
Depends on D130600
Assignee | ||
Comment 88•1 year ago
|
||
Depends on D130601
Assignee | ||
Comment 89•1 year ago
|
||
Depends on D130602
Assignee | ||
Comment 90•1 year ago
|
||
Depends on D130603
Assignee | ||
Comment 91•1 year ago
|
||
Depends on D130604
Assignee | ||
Comment 92•1 year ago
|
||
Assignee | ||
Updated•1 year ago
|
Assignee | ||
Comment 93•1 year ago
|
||
One random low-severity issue this code found (there are a lot): bug 1740284. Also bug 1739421
Assignee | ||
Comment 94•1 year ago
|
||
Depends on D130605
Assignee | ||
Comment 95•1 year ago
|
||
Depends on D130946
Assignee | ||
Updated•1 year ago
|
Assignee | ||
Comment 96•1 year ago
|
||
Depends on D130582
Updated•1 year ago
|
Assignee | ||
Comment 97•1 year ago
|
||
Depends on D130600
Assignee | ||
Comment 98•1 year ago
|
||
Depends on D130947
Assignee | ||
Comment 99•1 year ago
|
||
Depends on D131816
Assignee | ||
Comment 100•1 year ago
|
||
Depends on D131817
Assignee | ||
Comment 101•1 year ago
|
||
Depends on D131818
Assignee | ||
Comment 102•1 year ago
|
||
Depends on D131819
Assignee | ||
Comment 103•1 year ago
|
||
Depends on D131820
Assignee | ||
Comment 104•1 year ago
|
||
Depends on D130582
Assignee | ||
Comment 105•1 year ago
|
||
Depends on D130600
Assignee | ||
Comment 106•1 year ago
|
||
Depends on D130947
Assignee | ||
Comment 107•1 year ago
|
||
Depends on D131829
Assignee | ||
Comment 108•1 year ago
|
||
Depends on D131830
Assignee | ||
Comment 109•1 year ago
|
||
Depends on D131831
Assignee | ||
Comment 110•1 year ago
|
||
Depends on D131832
Assignee | ||
Comment 111•1 year ago
|
||
Depends on D130582
Assignee | ||
Comment 112•1 year ago
|
||
Depends on D130600
Assignee | ||
Comment 113•1 year ago
|
||
Depends on D130947
Assignee | ||
Comment 114•1 year ago
|
||
Depends on D131839
Assignee | ||
Comment 115•1 year ago
|
||
Depends on D131840
Assignee | ||
Comment 116•1 year ago
|
||
Depends on D131841
Assignee | ||
Comment 117•1 year ago
|
||
Depends on D131842
Assignee | ||
Comment 118•1 year ago
|
||
Depends on D130582
Assignee | ||
Comment 119•1 year ago
|
||
Depends on D130600
Assignee | ||
Comment 120•1 year ago
|
||
Depends on D130947
Assignee | ||
Comment 121•1 year ago
|
||
Depends on D131846
Assignee | ||
Comment 122•1 year ago
|
||
Depends on D131847
Assignee | ||
Comment 123•1 year ago
|
||
Depends on D131848
Assignee | ||
Comment 124•1 year ago
|
||
Depends on D131849
Assignee | ||
Comment 125•1 year ago
|
||
Depends on D130582
Assignee | ||
Comment 126•1 year ago
|
||
Depends on D130947
Assignee | ||
Comment 127•1 year ago
|
||
Depends on D131875
Assignee | ||
Comment 128•1 year ago
|
||
Depends on D131876
Assignee | ||
Comment 129•1 year ago
|
||
Depends on D131877
Assignee | ||
Comment 130•1 year ago
|
||
Depends on D131878
Assignee | ||
Comment 131•1 year ago
|
||
Sorry for the bug-spam; moz-phab has failed multiple times on me. Finally got a clean, complete upload of the current WIP patches
Assignee | ||
Comment 132•1 year ago
|
||
Depends on D131879
Assignee | ||
Comment 133•1 year ago
|
||
Depends on D132436
Assignee | ||
Comment 134•1 year ago
|
||
Depends on D132641
Comment 135•1 year ago
|
||
Comment on attachment 9253324 [details]
Bug 1207753 - modules/jar thread-safety annotations r=nika
Revision D132642 was moved to bug 1744043. Setting attachment 9253324 [details] to obsolete.
Updated•1 year ago
|
Updated•1 year ago
|
Assignee | ||
Comment 136•1 year ago
|
||
Updated•11 months ago
|
Assignee | ||
Comment 137•11 months ago
|
||
Assignee | ||
Comment 139•11 months ago
|
||
Comment 140•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/a68ee4b09f92 Add MOZ_UNANNOTATED to all Mutexes/Monitors r=nika,kershaw
Comment 141•11 months ago
|
||
Backed out for causing Hazard bustages.
Backout link: https://hg.mozilla.org/integration/autoland/rev/cf0358be9f097701fd02780876723576d5597d89
Failure log: https://treeherder.mozilla.org/logviewer?job_id=371247412&repo=autoland&lineNumber=12073
Comment 142•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/12a59e5a50bf Add MOZ_UNANNOTATED to all Mutexes/Monitors r=nika,kershaw
Comment 143•11 months ago
|
||
Backed out changeset 12a59e5a50bf (Bug 1207753) for causing build bustage CLOSED TREE
Log: https://treeherder.mozilla.org/logviewer?job_id=371274693&repo=autoland&lineNumber=5661
https://treeherder.mozilla.org/logviewer?job_id=371274698&repo=autoland&lineNumber=2103
Lint failure: https://treeherder.mozilla.org/logviewer?job_id=371274727&repo=autoland&lineNumber=114
Backout: https://hg.mozilla.org/integration/autoland/rev/b8aed504421d5e1fa4b7ace950b7aef73038aad8
Comment 144•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/1a24671d0ce8 Add MOZ_UNANNOTATED to all Mutexes/Monitors r=nika,kershaw
Comment 145•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/3b8c7fa73e82 Base thread-safety attribution support r=nika
Comment 146•11 months ago
|
||
Backed out for causing build bustages on Monitor.h
- Backout link
- Push with failures
- Failure Log
- Failure line: /builds/worker/workspace/obj-build/dist/include/mozilla/Monitor.h:280:40: error: 'assert_capability' attribute takes one argument
Assignee | ||
Updated•11 months ago
|
Assignee | ||
Comment 147•11 months ago
|
||
Updated•11 months ago
|
Comment 148•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/5fcffc287880 Base thread-safety attribution support r=nika
Updated•11 months ago
|
Comment 149•11 months ago
|
||
bugherder |
Assignee | ||
Comment 150•11 months ago
|
||
Comment 151•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/12e876557ed4 Fix ReleaseableMonitorAutoLock thread-safety annotation r=nika
Comment 152•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/15ce8ba0932c Basic thread-safety annotations to quiet errors until real annotations land r=nika
Comment 153•11 months ago
|
||
bugherder |
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Assignee | ||
Comment 154•11 months ago
|
||
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Comment 155•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/77c6a01d5bbc xpcom Timer thread-safety annotations r=nika
Comment 156•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/1ff35cafbdc2 ChannelEventQueue Thread-safety annotations r=necko-reviewers,valentin
Comment 157•11 months ago
|
||
bugherder |
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Comment 158•11 months ago
|
||
I think it would be better to file new bugs for these various patches you are adding here. The base bug already has an old and complex history, and it'll be difficult to figure out which patches landed in which version, should that be relevant later.
Assignee | ||
Comment 159•11 months ago
|
||
Assignee | ||
Comment 160•11 months ago
|
||
Assignee | ||
Comment 161•11 months ago
|
||
Assignee | ||
Comment 162•11 months ago
|
||
Assignee | ||
Comment 163•11 months ago
|
||
Assignee | ||
Comment 164•11 months ago
|
||
Updated•11 months ago
|
Comment 165•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/328857c0299d gfx thread-safety annotations r=gfx-reviewers,lsalzman
Comment 166•11 months ago
|
||
Comment on attachment 9249697 [details]
WIP: Bug 1207753 - ipc
Revision D130599 was moved to bug 1760659. Setting attachment 9249697 [details] to obsolete.
Comment 167•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/74c9088f7922 windows JumpListBuilder thread-safety annotations r=mhowell
Comment 168•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/c73e66ec8e14 script preloader thread-safety annotations r=mccr8
Comment 169•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/37388f5fbd2d xpcom nsThreadPool thread-safety annotations r=nika
Comment 170•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/e36fd04aa847 gfx & image thread-safety annotations r=gfx-reviewers,aosmond
Comment 171•11 months ago
|
||
Comment on attachment 9249703 [details]
WIP: Bug 1207753 - Parser
Revision D130605 was moved to bug 1760661. Setting attachment 9249703 [details] to obsolete.
Comment 172•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/e23f071eb6f0 image thread-safety annotations r=aosmond
Comment 173•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/c0702641e5d3 security/certverifier thread-safety annotations r=keeler
Comment 174•11 months ago
|
||
Comment on attachment 9249692 [details]
WIP: Bug 1207753 - DOM workers
Revision D130594 was moved to bug 1760662. Setting attachment 9249692 [details] to obsolete.
Comment 175•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/74709c76387c gfx/layers/apz thread-safety annotations r=botond
Comment 176•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/22dd9ee7e4a4 dom misc thread-safety annotations r=hsivonen,smaug,media-playback-reviewers,alwu
Comment 177•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/4a554b37a407 Smaller xpcom/threads & xpfe thread-safety annotations r=nika
Comment 178•11 months ago
|
||
Backed out 22dd9ee7e4a4b6e007c5245c0ef29d1f516bce25 for causing bustage on ProcessHangMonitor.cpp
- backout: https://hg.mozilla.org/integration/autoland/rev/24bca595e5e6d695959cb07b42b120e4325e3755
- push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=22dd9ee7e4a4b6e007c5245c0ef29d1f516bce25
- failure log: https://treeherder.mozilla.org/logviewer?job_id=371820798&repo=autoland&lineNumber=11287
[task 2022-03-21T23:30:28.199Z] 23:30:28 ERROR - /builds/worker/checkouts/gecko/dom/ipc/ProcessHangMonitor.cpp:248:53: error: writing variable 'mProcess' requires holding mutex 'mMonitor' exclusively [-Werror,-Wthread-safety-analysis]
[task 2022-03-21T23:30:28.199Z] 23:30:28 INFO - void SetProcess(HangMonitoredProcess* aProcess) { mProcess = aProcess; }
[task 2022-03-21T23:30:28.200Z] 23:30:28 INFO - ^
[task 2022-03-21T23:30:28.200Z] 23:30:28 ERROR - /builds/worker/checkouts/gecko/dom/ipc/ProcessHangMonitor.cpp:367:38: error: reading variable 'mContext' requires holding mutex 'mMonitor' [-Werror,-Wthread-safety-analysis]
[task 2022-03-21T23:30:28.200Z] 23:30:28 INFO - js::AutoAssertNoContentJS nojs(mContext);
[task 2022-03-21T23:30:28.200Z] 23:30:28 INFO -
Comment 179•11 months ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/328857c0299d
https://hg.mozilla.org/mozilla-central/rev/74c9088f7922
https://hg.mozilla.org/mozilla-central/rev/c73e66ec8e14
https://hg.mozilla.org/mozilla-central/rev/37388f5fbd2d
https://hg.mozilla.org/mozilla-central/rev/e36fd04aa847
https://hg.mozilla.org/mozilla-central/rev/e23f071eb6f0
https://hg.mozilla.org/mozilla-central/rev/c0702641e5d3
https://hg.mozilla.org/mozilla-central/rev/74709c76387c
https://hg.mozilla.org/mozilla-central/rev/4a554b37a407
Comment 180•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/bd209cbc621b baseprofiler thread-safety annotations r=gerald
Comment 181•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/a3d2a5a43398 nsUrlClassifier thread-safety annotations r=dimi?
Comment 182•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/ba8a5637b6ec webaudio thread-safety annotations r=padenot
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Comment 183•11 months ago
|
||
bugherder |
Updated•11 months ago
|
Updated•11 months ago
|
Comment 184•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/ec444f85837f netwerk nsWifiMonitor thread-safety annotations r=necko-reviewers,dragana
Comment 185•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/5d7eed758045 nsHttpConnectionMgr thread-safety annotations r=necko-reviewers,dragana
Comment 186•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/44ae041f0f9f InputStreamPump thread-safety annotations r=necko-reviewers,dragana
Comment 187•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/95175b21b512 dom/base/BodyStream thread-safety annotations r=smaug
Comment 188•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/a68541e609b7 WebSocket thread-safety annotations r=necko-reviewers,kershaw
Assignee | ||
Comment 189•11 months ago
|
||
Comment 190•11 months ago
|
||
bugherder |
Comment 191•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/1a6d4186a415 Various xpcom thread-safety annotations r=nika
Comment 192•11 months ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/685901935d66 dom/promises thread-safety annotations r=smaug
Comment 193•11 months ago
|
||
bugherder |
Description
•