Open Bug 1207753 (threadsafety) Opened 7 years ago Updated 5 days ago

Add static thread-safety lock/mutex analysis based on clang GUARDED_BY()

Categories

(Core :: XPCOM, task, P3)

task

Tracking

()

ASSIGNED

People

(Reporter: jesup, Assigned: jesup, NeedInfo)

References

(Depends on 4 open bugs)

Details

(Keywords: leave-open, sec-audit)

Attachments

(57 files, 78 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
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.
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!
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.
(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.  :-\
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
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.
Attachment #8665165 - Attachment is obsolete: true
Group: core-security → dom-core-security
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?
Flags: needinfo?(rjesup)
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.
Flags: needinfo?(rjesup)
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?
Flags: needinfo?(abillings)
Sylvestre, is this something your new static analysis person could look at?
Flags: needinfo?(abillings) → needinfo?(sledru)
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?
Flags: needinfo?(sledru)
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
This is just a merge with the latest from mozilla-centrall + some definistion inside the missing file in order to have the build working.
Attachment #8713128 - Flags: review?(rjesup)
The missing file from the original patch was xpcom/glue/MutexThreadSafety.h, it's content has been copied from webrtc/base/thread_annotations.h
Attachment #8713128 - Attachment is patch: true
Attachment #8665261 - Attachment is obsolete: true
Attached patch Patch 3 - NSS fix (obsolete) — Splinter Review
Attachment #8713128 - Flags: review?(rjesup)
Attached patch GUARDED_BY (obsolete) — Splinter Review
This is WIP. I've upload just to make sure we're on the right track.
Attachment #8713128 - Attachment is obsolete: true
Attachment #8715196 - Flags: review?(rjesup)
Attachment #8715196 - Attachment is patch: true
Attachment #8715196 - Flags: review?(rjesup) → feedback?(rjesup)
Attached patch GUARDED_BY_II (obsolete) — Splinter Review
I haven't analyze the warnings yet, I'm planning to do it next week.
Any feedback is appreciated.
Attachment #8716216 - Flags: feedback?(rjesup)
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
Attachment #8716216 - Attachment is patch: true
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).
Attachment #8716216 - Flags: feedback?(rjesup) → feedback+
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
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
Attachment #8715196 - Flags: feedback?(rjesup) → feedback+
Attached patch 1207753_GUARDED_BY_I (obsolete) — Splinter Review
thanks for the help Jesup, I've modified the patches
Attached patch 1207753_GUARDED_BY_II (obsolete) — Splinter Review
Attachment #8717930 - Attachment is patch: true
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)
Attachment #8717931 - Attachment is patch: true
Attachment #8713302 - Attachment is obsolete: true
Attachment #8713306 - Attachment is obsolete: true
Attachment #8713307 - Attachment is obsolete: true
Attachment #8713308 - Attachment is obsolete: true
Attachment #8713309 - Attachment is obsolete: true
Attachment #8713310 - Attachment is obsolete: true
AssertCurrentThreadOwns implies the method should be EXCLUSIVE_LOCKS_REQUIRED
Attachment #8713311 - Attachment is obsolete: true
Attachment #8717930 - Attachment is obsolete: true
Attachment #8717931 - Attachment is obsolete: true
Attachment #8713305 - Attachment is obsolete: true
Attachment #8715196 - Attachment is obsolete: true
Attachment #8716216 - Attachment is obsolete: true
Attachment #8713304 - Attachment is obsolete: true
Rebased, parent is b4b5fa96e8ee
Attachment #8761189 - Attachment is obsolete: true
Attachment #8761193 - Attachment is obsolete: true
Attachment #8761195 - Attachment is obsolete: true
Attachment #8761196 - Attachment is obsolete: true
Attached patch GMP lock changes (obsolete) — Splinter Review
Attachment #8761197 - Attachment is obsolete: true
Attachment #8761198 - Attachment is obsolete: true
AssertCurrentThreadOwns implies the method should be EXCLUSIVE_LOCKS_REQUIRED
Attachment #8761199 - Attachment is obsolete: true
Attachment #8761200 - Attachment is obsolete: true
Attachment #8761201 - Attachment is obsolete: true
Attachment #8761202 - Attachment is obsolete: true
Attachment #8761204 - Attachment is obsolete: true
Attachment #8787840 - Attachment is obsolete: true
See Also: → 1300390
Attachment #8787831 - Attachment is obsolete: true
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
Type: defect → task
Status: NEW → ASSIGNED
Priority: -- → P3
Attachment #8787830 - Attachment is obsolete: true
Attachment #8787832 - Attachment is obsolete: true
Attachment #8787833 - Attachment is obsolete: true
Attachment #8787834 - Attachment is obsolete: true
Attachment #8787835 - Attachment is obsolete: true
Attachment #8787836 - Attachment is obsolete: true
Attachment #8787837 - Attachment is obsolete: true
Attachment #8787838 - Attachment is obsolete: true
Attachment #8787839 - Attachment is obsolete: true
Attachment #8787841 - Attachment is obsolete: true
Attachment #9002003 - Attachment is obsolete: true
Attached file WIP: Bug 1207753 - netwerk/cache2 (obsolete) —

Depends on D130580

Depends on D130581

Depends on D130585

Attached file WIP: Bug 1207753 - DOM workers (obsolete) —

Depends on D130593

Depends on D130596

Attached file WIP: Bug 1207753 - ipc (obsolete) —

Depends on D130598

Attached file WIP: Bug 1207753 - StartupCache (obsolete) —

Depends on D130600

Depends on D130602

Attached file WIP: Bug 1207753 - Parser (obsolete) —

Depends on D130604

Attachment #9249669 - Attachment is obsolete: true

One random low-severity issue this code found (there are a lot): bug 1740284. Also bug 1739421

Depends on: 1740747
Alias: threadsafety
Depends on: 1742388

Depends on D130582

Attachment #9249692 - Attachment description: WIP: Bug 1207753 - workers → WIP: Bug 1207753 - DOM workers
Attached file WIP: Bug 1207753 - gfx/layers/apz (obsolete) —

Depends on D130600

Depends on D130947

Depends on D131816

Attached file WIP: Bug 1207753 - dom/media webm (obsolete) —

Depends on D131817

Depends on D131818

Attached file WIP: Bug 1207753 - security/manager (obsolete) —

Depends on D131819

Attached file WIP: Bug 1207753 - security misc (obsolete) —

Depends on D131820

Attached file WIP: Bug 1207753 - netwerk misc (obsolete) —

Depends on D130582

Attached file WIP: Bug 1207753 - gfx/layers/apz (obsolete) —

Depends on D130600

Depends on D130947

Depends on D131829

Attached file WIP: Bug 1207753 - dom/media webm (obsolete) —

Depends on D131830

Depends on D131831

Attached file WIP: Bug 1207753 - security/manager (obsolete) —

Depends on D131832

Attached file WIP: Bug 1207753 - netwerk misc (obsolete) —

Depends on D130582

Attached file WIP: Bug 1207753 - gfx/layers/apz (obsolete) —

Depends on D130600

Depends on D130947

Depends on D131839

Attached file WIP: Bug 1207753 - dom/media webm (obsolete) —

Depends on D131840

Depends on D131841

Attached file WIP: Bug 1207753 - security/manager (obsolete) —

Depends on D131842

Attached file WIP: Bug 1207753 - netwerk misc (obsolete) —

Depends on D130582

Depends on D130947

Depends on D131846

Attached file WIP: Bug 1207753 - dom/media webm (obsolete) —

Depends on D131847

Depends on D131848

Attached file WIP: Bug 1207753 - security/manager (obsolete) —

Depends on D131849

Attached file WIP: Bug 1207753 - netwerk misc (obsolete) —

Depends on D130582

Sorry for the bug-spam; moz-phab has failed multiple times on me. Finally got a clean, complete upload of the current WIP patches

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.

Attachment #9253324 - Attachment is obsolete: true
Attachment #9253324 - Attachment is obsolete: false
Depends on: 1747344
Attachment #9249704 - Attachment description: WIP: Bug 1207753 - Base thread-safety attribution support → Bug 1207753 - Base thread-safety attribution support r=nika
Depends on: 1752171
Depends on: 1757863
Group: dom-core-security
Blocks: 1759501
Duplicate of this bug: 1098387
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/a68ee4b09f92
Add MOZ_UNANNOTATED to all Mutexes/Monitors r=nika,kershaw
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/12a59e5a50bf
Add MOZ_UNANNOTATED to all Mutexes/Monitors r=nika,kershaw
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/1a24671d0ce8
Add MOZ_UNANNOTATED to all Mutexes/Monitors r=nika,kershaw
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/3b8c7fa73e82
Base thread-safety attribution support r=nika

Backed out for causing build bustages on Monitor.h

Flags: needinfo?(rjesup)
Keywords: leave-open
Attachment #9249704 - Attachment description: Bug 1207753 - Base thread-safety attribution support r=nika → Bug 1207753 - Base thread-safety attribution support r=nika,glandium
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/5fcffc287880
Base thread-safety attribution support r=nika
Attachment #9268156 - Attachment is obsolete: true
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/12e876557ed4
Fix ReleaseableMonitorAutoLock thread-safety annotation r=nika
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
Attachment #9249670 - Attachment description: WIP: Bug 1207753 - Timers → Bug 1207753 - xpcom Timer thread-safety annotations r=nika
Attachment #9249671 - Attachment description: WIP: Bug 1207753 - Threadpools → Bug 1207753 - xpcom nsThreadPool thread-safety annotations r=nika
Attachment #9249672 - Attachment description: WIP: Bug 1207753 - MozPromise and TaskQueue → WIP: Bug 1207753 - xpcom MozPromise and TaskQueue thread-safety annotations r=nika
Depends on: 1760315
Attachment #9249673 - Attachment description: WIP: Bug 1207753 - Smaller xpcom/threads stuff → Bug 1207753 - Smaller xpcom/threads & xpfe thread-safety annotations r=nika
Attachment #9249674 - Attachment description: WIP: Bug 1207753 - TaskController → WIP: Bug 1207753 - xpcom TaskController thread-safety annotations r=bas
Attachment #9249675 - Attachment description: WIP: Bug 1207753 - nsCategoryManager → Bug 1207753 - xpcom nsCategoryManager thread-safety annotations r=nika
Attachment #9249677 - Attachment description: WIP: Bug 1207753 - Other smaller xpcom stuff → Bug 1207753 - Various xpcom thread-safety annotations r=nika
Attachment #9249678 - Attachment description: WIP: Bug 1207753 - netwerk/dns → Bug 1207753 - netwerk/dns thread-safety annotations r=#necko-reviewers
Attachment #9249681 - Attachment description: WIP: Bug 1207753 - DataChannel → Bug 1207753 - DataChannel thread-annotations r=bwc
Attachment #9249682 - Attachment description: WIP: Bug 1207753 - Other netwerk → Bug 1207753 - netwerk thread-safety annotations r=#necko-reviewers
Attachment #9249683 - Attachment description: WIP: Bug 1207753 - dom/media/systemservices (Camera, etc) → Bug 1207753 - dom/media/systemservices thread-safety annotations r=jib
Attachment #9249685 - Attachment description: WIP: Bug 1207753 - dom/media/File* → Bug 1207753 - dom/media/File* thread-safety annotations r=bryce
Attachment #9249686 - Attachment description: WIP: Bug 1207753 - webaudio → Bug 1207753 - webaudio thread-safety annotations r=padenot
Attachment #9249687 - Attachment description: WIP: Bug 1207753 - dom/media/MediaFormatReader → Bug 1207753 - dom/media/MediaFormatReader thread-safety annotations r=bryce
Attachment #9249688 - Attachment description: WIP: Bug 1207753 - MediaTrackGraph* → Bug 1207753 - MediaTrackGraph* thread-safety annotations r=padenot
Attachment #9249689 - Attachment description: WIP: Bug 1207753 - GMP → Bug 1207753 - GMP thread-safety annotations r=bwc
Attachment #9249690 - Attachment description: WIP: Bug 1207753 - dom/media misc → Bug 1207753 - dom/media misc thread-safety annotations r=bryce
Attachment #9249691 - Attachment description: WIP: Bug 1207753 - WebSocket → Bug 1207753 - WebSocket thread-safety annotations r=#necko-reviewers
Attachment #9249693 - Attachment description: WIP: Bug 1207753 - dom indexdb/localstorage/quota → WIP: Bug 1207753 - dom indexdb/localstorage/quota thread-safety annotations r=#dom-storage-reviewers
Attachment #9249676 - Attachment description: WIP: Bug 1207753 - nsComponentManager → WIP: Bug 1207753 - xpcom nsComponentManager thread-safety annotations r=nika
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/77c6a01d5bbc
xpcom Timer thread-safety annotations r=nika
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/1ff35cafbdc2
ChannelEventQueue Thread-safety annotations r=necko-reviewers,valentin
No longer blocks: 1759501
Severity: normal → --
Depends on: 1759501
Attachment #9249694 - Attachment description: WIP: Bug 1207753 - dom/base/BodyStream → Bug 1207753 - dom/base/BodyStream thread-safety annotations r=smaug
Attachment #9249696 - Attachment description: WIP: Bug 1207753 - dom misc → Bug 1207753 - dom misc thread-safety annotations r=hsivonen,smaug
Attachment #9249698 - Attachment description: WIP: Bug 1207753 - gfx & image → Bug 1207753 - gfx & image thread-safety annotations r=#gfx-reviewers
Attachment #9251999 - Attachment description: WIP: Bug 1207753 - gfx/layers/apz → Bug 1207753 - gfx/layers/apz thread-safety annotations r=#gfx-reviewers
Attachment #9249700 - Attachment description: WIP: Bug 1207753 - nsUrlClassifier → Bug 1207753 - nsUrlClassifier thread-safety annotations r=dimi?
Attachment #9249702 - Attachment description: WIP: Bug 1207753 - script preloader → Bug 1207753 - script preloader thread-safety annotations r=mccr8
Attachment #9250322 - Attachment description: WIP: Bug 1207753 - dom/WebGPU → WIP: Bug 1207753 - dom/WebGPU thread-safety annotations r=kvark
Attachment #9250323 - Attachment description: WIP: Bug 1207753 - dom/promises → Bug 1207753 - dom/promises thread-safety annotations r=smaug
Attachment #9252040 - Attachment description: WIP: Bug 1207753 - dom/media ChannelMediaResource → Bug 1207753 - dom/media ChannelMediaResource thread-safety annotations r=bryce
Attachment #9252041 - Attachment description: WIP: Bug 1207753 - dom/media MediaSourceDemuxer → Bug 1207753 - dom/media MediaSourceDemuxer thread-safety annotations r=bryce
Attachment #9252042 - Attachment description: WIP: Bug 1207753 - dom/media webm → Bug 1207753 - dom/media webm thread-safety annotations r=bryce
Attachment #9252043 - Attachment description: WIP: Bug 1207753 - security/certverifier → Bug 1207753 - security/certverifier thread-safety annotations r=keeler
Attachment #9252044 - Attachment description: WIP: Bug 1207753 - security/manager → Bug 1207753 - security/manager thread-safety annotations r=keeler
Attachment #9252975 - Attachment description: WIP: Bug 1207753 - ffmpeg → Bug 1207753 - ffmpeg integration thread-safety annotations r=bryce
Attachment #9253323 - Attachment description: WIP: Bug 1207753 - image → Bug 1207753 - image thread-safety annotations r=jrmuizel

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.

Attachment #9249676 - Attachment description: WIP: Bug 1207753 - xpcom nsComponentManager thread-safety annotations r=nika → Bug 1207753 - xpcom nsComponentManager thread-safety annotations r=nika
Depends on: 1760644
Depends on: 1760645
Depends on: 1760647
Depends on: 1760649
Depends on: 1760651
Depends on: 1760652
Depends on: 1760655
Depends on: 1760656
Depends on: 1760657
Depends on: 1760659
Depends on: 1760660
Depends on: 1760661
Depends on: 1760662
Depends on: 1760664
Depends on: 1760667
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/328857c0299d
gfx thread-safety annotations r=gfx-reviewers,lsalzman

Comment on attachment 9249697 [details]
WIP: Bug 1207753 - ipc

Revision D130599 was moved to bug 1760659. Setting attachment 9249697 [details] to obsolete.

Attachment #9249697 - Attachment is obsolete: true
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/74c9088f7922
windows JumpListBuilder thread-safety annotations r=mhowell
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/c73e66ec8e14
script preloader thread-safety annotations r=mccr8
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/37388f5fbd2d
xpcom nsThreadPool thread-safety annotations r=nika
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/e36fd04aa847
gfx & image thread-safety annotations r=gfx-reviewers,aosmond

Comment on attachment 9249703 [details]
WIP: Bug 1207753 - Parser

Revision D130605 was moved to bug 1760661. Setting attachment 9249703 [details] to obsolete.

Attachment #9249703 - Attachment is obsolete: true
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/e23f071eb6f0
image thread-safety annotations r=aosmond
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/c0702641e5d3
security/certverifier thread-safety annotations r=keeler

Comment on attachment 9249692 [details]
WIP: Bug 1207753 - DOM workers

Revision D130594 was moved to bug 1760662. Setting attachment 9249692 [details] to obsolete.

Attachment #9249692 - Attachment is obsolete: true
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/74709c76387c
gfx/layers/apz thread-safety annotations r=botond
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
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/4a554b37a407
Smaller xpcom/threads & xpfe thread-safety annotations r=nika

Backed out 22dd9ee7e4a4b6e007c5245c0ef29d1f516bce25 for causing bustage on ProcessHangMonitor.cpp

[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 -                   
Flags: needinfo?(rjesup)
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/bd209cbc621b
baseprofiler thread-safety annotations r=gerald
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/a3d2a5a43398
nsUrlClassifier thread-safety annotations r=dimi?
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/ba8a5637b6ec
webaudio thread-safety annotations r=padenot
Attachment #9252004 - Attachment is obsolete: true
Attachment #9252039 - Attachment is obsolete: true
Attachment #9252003 - Attachment is obsolete: true
Attachment #9252002 - Attachment is obsolete: true
Attachment #9252001 - Attachment is obsolete: true
Attachment #9251998 - Attachment is obsolete: true
Attachment #9252000 - Attachment is obsolete: true
Attachment #9251997 - Attachment is obsolete: true
Attachment #9251996 - Attachment is obsolete: true
Attachment #9251995 - Attachment is obsolete: true
Attachment #9251994 - Attachment is obsolete: true
Attachment #9251992 - Attachment is obsolete: true
Attachment #9251991 - Attachment is obsolete: true
Attachment #9251987 - Attachment is obsolete: true
Attachment #9251986 - Attachment is obsolete: true
Attachment #9251985 - Attachment is obsolete: true
Attachment #9251982 - Attachment is obsolete: true
Attachment #9251993 - Attachment is obsolete: true
Attachment #9251984 - Attachment is obsolete: true
Attachment #9251983 - Attachment is obsolete: true
Attachment #9251981 - Attachment is obsolete: true
Attachment #9251958 - Attachment is obsolete: true
Attachment #9251957 - Attachment is obsolete: true
Attachment #9251956 - Attachment is obsolete: true
Attachment #9251955 - Attachment is obsolete: true
Attachment #9251954 - Attachment is obsolete: true
Attachment #9251953 - Attachment is obsolete: true
Attachment #9251952 - Attachment is obsolete: true
Attachment #9249699 - Attachment is obsolete: true
Attachment #9249680 - Attachment is obsolete: true
Attachment #9249679 - Attachment is obsolete: true
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/ec444f85837f
netwerk nsWifiMonitor thread-safety annotations r=necko-reviewers,dragana
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/5d7eed758045
nsHttpConnectionMgr thread-safety annotations r=necko-reviewers,dragana
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/44ae041f0f9f
InputStreamPump thread-safety annotations r=necko-reviewers,dragana
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/95175b21b512
dom/base/BodyStream thread-safety annotations r=smaug
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/a68541e609b7
WebSocket thread-safety annotations r=necko-reviewers,kershaw
Depends on: 1761040
Depends on: 1761107
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/1a6d4186a415
Various xpcom thread-safety annotations r=nika
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/685901935d66
dom/promises thread-safety annotations r=smaug
Attachment #9253324 - Attachment description: WIP: Bug 1207753 - modules/jar → Bug 1207753 - modules/jar thread-safety annotations r=nika
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/383273d8967c
dom/media/File* thread-safety annotations r=bryce
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/3adee115e772
dom misc thread-safety annotations r=hsivonen,smaug
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/25a863d8b931
minor cleanup of a comment r=nika
Attachment #9249693 - Attachment description: WIP: Bug 1207753 - dom indexdb/localstorage/quota thread-safety annotations r=#dom-storage-reviewers → Bug 1207753 - dom indexdb/localstorage/quota thread-safety annotations r=#dom-storage-reviewers
Depends on: 1761579
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/cd14b620c039
ThrottledEventQueue thread-safety annotations r=nika
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/3d3edbd3368a
modules/jar thread-safety annotations r=nika
Depends on: 1761771
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/51cce32355af
dom/media webm thread-safety annotations r=kinetik
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/87667331b157
MediaTrackGraph* thread-safety annotations r=padenot
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/7efb455dcaf2
DataChannel thread-annotations r=bwc
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/4d020158e79f
Add thread-safety annotations to CompositorBridgeParent r=gfx-reviewers,aosmond

Backed out for causing xpcshell jobs to fail with Task aborted - max run time exceeded error.

Push with failures

Failure log

Backout link

[task 2022-03-29T23:52:53.643Z] 23:52:53     INFO -  TEST-START | dom/base/test/unit/test_serializers_entities.js
[taskcluster:error] Aborting task...
[task 2022-03-29T23:52:53.859Z] 23:52:53     INFO -  TEST-PASS | dom/base/test/unit/test_range.js | took 1106ms
[task 2022-03-29T23:52:53.859Z] 23:52:53     INFO -  TEST-START | dom/base/test/unit/test_serializers_entities_in_attr.js
[taskcluster 2022-03-29T23:52:55.804Z] SUCCESS: The process with PID 2952 (child process of PID 5164) has been terminated.
[taskcluster 2022-03-29T23:52:55.804Z] SUCCESS: The process with PID 6796 (child process of PID 5164) has been terminated.
[taskcluster 2022-03-29T23:52:55.804Z] ERROR: The process with PID 3328 (child process of PID 5164) could not be terminated.
[taskcluster 2022-03-29T23:52:55.804Z] Reason: There is no running instance of the task.
[taskcluster 2022-03-29T23:52:55.804Z] SUCCESS: The process with PID 9756 (child process of PID 5164) has been terminated.
[taskcluster 2022-03-29T23:52:55.804Z] SUCCESS: The process with PID 5164 (child process of PID 2996) has been terminated.
[taskcluster 2022-03-29T23:52:55.804Z] SUCCESS: The process with PID 984 (child process of PID 4648) has been terminated.
[taskcluster 2022-03-29T23:52:55.804Z] ERROR: The process with PID 2996 (child process of PID 4648) could not be terminated.
[taskcluster 2022-03-29T23:52:55.804Z] Reason: There is no running instance of the task.
[taskcluster 2022-03-29T23:52:55.804Z] SUCCESS: The process with PID 4648 (child process of PID 3928) has been terminated.
[taskcluster 2022-03-29T23:52:55.804Z] SUCCESS: The process with PID 2332 (child process of PID 2360) has been terminated.
[taskcluster 2022-03-29T23:52:55.804Z] SUCCESS: The process with PID 3928 (child process of PID 2360) has been terminated.
[taskcluster 2022-03-29T23:52:55.804Z] SUCCESS: The process with PID 2360 (child process of PID 8708) has been terminated.
[taskcluster 2022-03-29T23:52:55.804Z] 
[taskcluster:warn 2022-03-29T23:52:55.804Z] exit status 128
[taskcluster 2022-03-29T23:52:55.804Z] === Task Finished ===
[taskcluster 2022-03-29T23:52:55.804Z] Task Duration: 45m2.1188175s
[taskcluster 2022-03-29T23:52:55.928Z] Uploading artifact public/logs/localconfig.json from file logs\localconfig.json with content encoding "gzip", mime type "application/json" and expiry 2023-03-29T23:02:12.408Z
[taskcluster 2022-03-29T23:52:56.116Z] Uploading artifact public/test_info/ce8f71f2-df70-455f-ba94-00331adf100c.dmp from file build\blobber_upload_dir\ce8f71f2-df70-455f-ba94-00331adf100c.dmp with content encoding "gzip", mime type "application/octet-stream" and expiry 2023-03-29T23:02:12.408Z
[taskcluster 2022-03-29T23:52:56.285Z] Uploading artifact public/test_info/ce8f71f2-df70-455f-ba94-00331adf100c.extra from file build\blobber_upload_dir\ce8f71f2-df70-455f-ba94-00331adf100c.extra with content encoding "gzip", mime type "application/octet-stream" and expiry 2023-03-29T23:02:12.408Z
[taskcluster 2022-03-29T23:52:56.381Z] Uploading artifact public/test_info/system-info.log from file build\blobber_upload_dir\system-info.log with content encoding "gzip", mime type "text/plain" and expiry 2023-03-29T23:02:12.408Z
[taskcluster 2022-03-29T23:52:56.496Z] Uploading artifact public/test_info/xpcshell_errorsummary.log from file build\blobber_upload_dir\xpcshell_errorsummary.log with content encoding "gzip", mime type "text/plain" and expiry 2023-03-29T23:02:12.408Z
[taskcluster 2022-03-29T23:52:56.582Z] Uploading artifact public/test_info/xpcshell_raw.log from file build\blobber_upload_dir\xpcshell_raw.log with content encoding "gzip", mime type "text/plain" and expiry 2023-03-29T23:02:12.408Z
[taskcluster 2022-03-29T23:52:56.714Z] Uploading link artifact public/logs/live.log to artifact public/logs/live_backing.log with expiry 2023-03-29T23:02:12.408Z
[taskcluster:error] Task aborted - max run time exceeded
Flags: needinfo?(rjesup)
Flags: needinfo?(rjesup)
Flags: needinfo?(rjesup)
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/cfa71d622b6c
nsThreadManager thread-safety annotations r=nika
Depends on: 1762605
Depends on: 1762910
Attachment #9269073 - Attachment is obsolete: true
Depends on: 1762959

Comment on attachment 9269403 [details]
Bug 1207753 - xpcom/io thread-safety annotations r=nika

Revision D142066 was moved to bug 1762959. Setting attachment 9269403 [details] to obsolete.

Attachment #9269403 - Attachment is obsolete: true
Depends on: 1766378
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/cdba1d6ffd46
ffmpeg integration thread-safety annotations r=bryce
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/b82a2207f77e
security/manager thread-safety annotations r=keeler,necko-reviewers,dragana
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/5a6fa4dd6203
netwerk thread-safety annotations r=necko-reviewers,dragana
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/257e72c30a60
netwerk/dns thread-safety annotations r=necko-reviewers,dragana
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/autoland/rev/2c1cd96c5acf
Add thread-safety annotations to CompositorBridgeParent r=gfx-reviewers,aosmond