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

Status: ASSIGNED

(Core :: XPCOM, task, P3)

Description

[Randell Jesup [:jesup] (needinfo me)]

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

[(no longer active)]

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!

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP patch for thread-safety static analysis. DO NOT PUSH TO TRY!

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

[Jeff Walden [:Waldo]]

(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.  :-\

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP patch for thread-safety static analysis. DO NOT PUSH TO TRY!

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.

Comment 6

[Nathan Froyd [:froydnj]]

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?

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

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

[Al Billings [:abillings - ex-MoCo]]

Sylvestre, is this something your new static analysis person could look at?

Comment 10

[Sylvestre Ledru [:Sylvestre]]

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?

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

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

[Andi [:andi]]

Attachment: bug_1207753_add_static_thread_safety_lock_mutex_analysis_based_on_clang_guarded_by

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

[Andi [:andi]]

The missing file from the original patch was xpcom/glue/MutexThreadSafety.h, it's content has been copied from webrtc/base/thread_annotations.h

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch 1 - WIP patch for thread-safety static analysis. DO NOT PUSH TO TRY!

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch 2 - Skia compile fixes

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch 3 - NSS fix

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch 4 - Datachannels mutex assertions and fixes

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch 5 - MediaResource mutex assertions and fixes

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch 6 - GMP mutex assertions

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch 7 - GMP mutex fixes and assertions

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch 8 - WebAudio mutex assertions and fixes

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch 9 - CurrentThreadOwns attributions

Comment 23

[Andi [:andi]]

Attachment: GUARDED_BY

This is WIP. I've upload just to make sure we're on the right track.

Comment 24

[radu stoica]

Attachment: GUARDED_BY_II

I haven't analyze the warnings yet, I'm planning to do it next week.
Any feedback is appreciated.

Comment 25

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 26

[Randell Jesup [:jesup] (needinfo me)]

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).

Comment 27

[Randell Jesup [:jesup] (needinfo me)]

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 28

[Randell Jesup [:jesup] (needinfo me)]

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

[radu stoica]

Attachment: 1207753_GUARDED_BY_I

thanks for the help Jesup, I've modified the patches

Comment 30

[radu stoica]

Attachment: 1207753_GUARDED_BY_II

Comment 31

[Randell Jesup [:jesup] (needinfo me)]

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)

Comment 32

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch 1 - WIP patch for thread-safety static analysis. DO NOT PUSH TO TRY!

parent is a285884cd669

Comment 33

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch 2 - DataChannels thread-safety attributions

Comment 34

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch 3 - MediaResource thread-safety attributions

Comment 35

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch 4 - GMP thread-safety attributions

Comment 36

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch 5 - GMP lock changes

Comment 37

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch 6 - WebAudio thread-safety attributions/fixes

Comment 38

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch 7 - Add back AssertCurrentThreadOwns thread-safety attributions

AssertCurrentThreadOwns implies the method should be EXCLUSIVE_LOCKS_REQUIRED

Comment 39

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch 8 - Thread-safety attributions/fixes for a lot of bits

Comment 40

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch 9 - dom/media thread-safety attributions/fixes

Comment 41

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch 10 - more thread-safety updates

Comment 42

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch 11 - More updates for things flagged

Comment 43

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP patch for thread-safety static analysis. DO NOT PUSH TO TRY!

Rebased, parent is b4b5fa96e8ee

Comment 44

[Randell Jesup [:jesup] (needinfo me)]

Attachment: DataChannels thread-safety attributions

Comment 45

[Randell Jesup [:jesup] (needinfo me)]

Attachment: MediaResource thread-safety attributions

Comment 46

[Randell Jesup [:jesup] (needinfo me)]

Attachment: GMP thread-safety attributions

Comment 47

[Randell Jesup [:jesup] (needinfo me)]

Attachment: GMP lock changes

Comment 48

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WebAudio thread-safety attributions/fixes

Comment 49

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Add back AssertCurrentThreadOwns thread-safety attributions

AssertCurrentThreadOwns implies the method should be EXCLUSIVE_LOCKS_REQUIRED

Comment 50

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Thread-safety attributions/fixes for a lot of bits

Comment 51

[Randell Jesup [:jesup] (needinfo me)]

Attachment: dom/media thread-safety attributions/fixes

Comment 52

[Randell Jesup [:jesup] (needinfo me)]

Attachment: more thread-safety updates

Comment 53

[Randell Jesup [:jesup] (needinfo me)]

Attachment: More updates for things flagged

Comment 54

[Randell Jesup [:jesup] (needinfo me)]

Attachment: More updates for things flagged

Comment 55

[Randell Jesup [:jesup] (needinfo me)]

Attachment: DataChannels thread-safety attributions

Comment 56

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 57

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - Base thread-safety attribution support

Comment 58

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - xpcom Timer thread-safety annotations r=nika

Depends on D130571

Comment 59

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - xpcom nsThreadPool thread-safety annotations r=nika

Depends on D130572

Comment 60

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - xpcom MozPromise and TaskQueue thread-safety annotations r=nika

Depends on D130573

Comment 61

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - Smaller xpcom/threads & xpfe thread-safety annotations r=nika

Depends on D130574

Comment 62

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - xpcom TaskController thread-safety annotations r=bas

Depends on D130575

Comment 63

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - xpcom nsCategoryManager thread-safety annotations r=nika

Depends on D130576

Comment 64

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - xpcom nsComponentManager thread-safety annotations r=nika

Depends on D130577

Comment 65

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - Various xpcom thread-safety annotations r=nika

Depends on D130578

Comment 66

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - netwerk/dns thread-safety annotations r=#necko-reviewers

Depends on D130579

Comment 67

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - netwerk/cache2

Depends on D130580

Comment 68

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - netwerk nsSocketTransport2

Depends on D130581

Comment 69

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - DataChannel thread-annotations r=bwc

Depends on D130582

Comment 70

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - netwerk thread-safety annotations r=#necko-reviewers

Depends on D130583

Comment 71

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - dom/media/systemservices thread-safety annotations r=jib

Depends on D130584

Comment 72

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - dom/media Audio

Depends on D130585

Comment 73

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - dom/media/File* thread-safety annotations r=bryce

Depends on D130586

Comment 74

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - webaudio thread-safety annotations r=padenot

Depends on D130587

Comment 75

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - dom/media/MediaFormatReader thread-safety annotations r=bryce

Depends on D130588

Comment 76

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - MediaTrackGraph* thread-safety annotations r=padenot

Depends on D130589

Comment 77

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - GMP thread-safety annotations r=bwc

Depends on D130590

Comment 78

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - dom/media misc thread-safety annotations r=bryce

Depends on D130591

Comment 79

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - WebSocket thread-safety annotations r=#necko-reviewers

Depends on D130592

Comment 80

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - DOM workers

Depends on D130593

Comment 81

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - dom indexdb/localstorage/quota thread-safety annotations r=#dom-storage-reviewers

Depends on D130594

Comment 82

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - dom/base/BodyStream thread-safety annotations r=smaug

Depends on D130595

Comment 83

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - dom/cache

Depends on D130596

Comment 84

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - dom misc thread-safety annotations r=hsivonen,smaug

Depends on D130597

Comment 85

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - ipc

Depends on D130598

Comment 86

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - gfx & image thread-safety annotations r=#gfx-reviewers

Depends on D130599

Comment 87

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - StartupCache

Depends on D130600

Comment 88

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - nsUrlClassifier thread-safety annotations r=dimi?

Depends on D130601

Comment 89

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - wayland clipboard

Depends on D130602

Comment 90

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - script preloader thread-safety annotations r=mccr8

Depends on D130603

Comment 91

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - Parser

Depends on D130604

Comment 92

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - Base thread-safety attribution support r=nika,glandium

Comment 93

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 94

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - dom/WebGPU thread-safety annotations r=kvark

Depends on D130605

Comment 95

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - dom/promises thread-safety annotations r=smaug

Depends on D130946

Comment 96

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - netwerk misc

Depends on D130582

Comment 97

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - gfx/layers/apz

Depends on D130600

Comment 98

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - dom/media ChannelMediaResource

Depends on D130947

Comment 99

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - dom/media MediaSourceDemuxer

Depends on D131816

Comment 100

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - dom/media webm

Depends on D131817

Comment 101

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - security/certverifier

Depends on D131818

Comment 102

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - security/manager

Depends on D131819

Comment 103

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - security misc

Depends on D131820

Comment 104

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - netwerk misc

Depends on D130582

Comment 105

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - gfx/layers/apz

Depends on D130600

Comment 106

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - dom/media ChannelMediaResource

Depends on D130947

Comment 107

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - dom/media MediaSourceDemuxer

Depends on D131829

Comment 108

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - dom/media webm

Depends on D131830

Comment 109

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - security/certverifier

Depends on D131831

Comment 110

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - security/manager

Depends on D131832

Comment 111

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - netwerk misc

Depends on D130582

Comment 112

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - gfx/layers/apz

Depends on D130600

Comment 113

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - dom/media ChannelMediaResource

Depends on D130947

Comment 114

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - dom/media MediaSourceDemuxer

Depends on D131839

Comment 115

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - dom/media webm

Depends on D131840

Comment 116

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - security/certverifier

Depends on D131841

Comment 117

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - security/manager

Depends on D131842

Comment 118

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - netwerk misc

Depends on D130582

Comment 119

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - gfx/layers/apz thread-safety annotations r=#gfx-reviewers

Depends on D130600

Comment 120

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - dom/media ChannelMediaResource

Depends on D130947

Comment 121

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - dom/media MediaSourceDemuxer

Depends on D131846

Comment 122

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - dom/media webm

Depends on D131847

Comment 123

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - security/certverifier

Depends on D131848

Comment 124

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - security/manager

Depends on D131849

Comment 125

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - netwerk misc

Depends on D130582

Comment 126

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - dom/media ChannelMediaResource thread-safety annotations r=bryce

Depends on D130947

Comment 127

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - dom/media MediaSourceDemuxer thread-safety annotations r=bryce

Depends on D131875

Comment 128

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - dom/media webm thread-safety annotations r=bryce

Depends on D131876

Comment 129

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - security/certverifier thread-safety annotations r=keeler

Depends on D131877

Comment 130

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - security/manager thread-safety annotations r=keeler

Depends on D131878

Comment 131

[Randell Jesup [:jesup] (needinfo me)]

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 132

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - ffmpeg integration thread-safety annotations r=bryce

Depends on D131879

Comment 133

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - image thread-safety annotations r=jrmuizel

Depends on D132436

Comment 134

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - modules/jar thread-safety annotations r=nika

Depends on D132641

Comment 135

[Phabricator Automation]

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.

Comment 136

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753: ChannelEventQueue Thread-safety annotations r=#necko-reviewers

Comment 137

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - Add MOZ_UNANNOTATED to all Mutexes/Monitors r=nika

Comment 139

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753: Basic thread-safety annotations to quiet errors until real annotations land r=nika

Comment 140

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a68ee4b09f92
Add MOZ_UNANNOTATED to all Mutexes/Monitors r=nika,kershaw

Comment 141

[Atila Butkovits]

Backed out for causing Hazard bustages.

Backout link: https://hg.mozilla.org/integration/autoland/rev/cf0358be9f097701fd02780876723576d5597d89

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=Q2BsgLH0TwqouQIbV2TjiQ.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=a68ee4b09f92b336cd65524d35c3ff7fd2a9845e

Failure log: https://treeherder.mozilla.org/logviewer?job_id=371247412&repo=autoland&lineNumber=12073

Comment 142

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/12a59e5a50bf
Add MOZ_UNANNOTATED to all Mutexes/Monitors r=nika,kershaw

Comment 143

[Noemi Erli[:noemi_erli]]

Backed out changeset 12a59e5a50bf (Bug 1207753) for causing build bustage CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&collapsedPushes=842382&selectedTaskRun=ByMzYAoAQZ-Jh8isTjFBxA.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=12a59e5a50bf4eaac3dbd9a455231cab8ffa96ea

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1a24671d0ce8
Add MOZ_UNANNOTATED to all Mutexes/Monitors r=nika,kershaw

Comment 145

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b8c7fa73e82
Base thread-safety attribution support r=nika

Comment 146

[Cristian Tuns]

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

Comment 147

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753: Fix ASSERT_CAPABILITY() use r=glandium

Comment 148

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5fcffc287880
Base thread-safety attribution support r=nika

Comment 149

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/1a24671d0ce8
https://hg.mozilla.org/mozilla-central/rev/5fcffc287880

Comment 150

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753: Fix ReleaseableMonitorAutoLock thread-safety annotation r=nika

Comment 151

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/12e876557ed4
Fix ReleaseableMonitorAutoLock thread-safety annotation r=nika

Comment 152

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/15ce8ba0932c
Basic thread-safety annotations to quiet errors until real annotations land r=nika

Comment 153

[Sandor Molnar]

https://hg.mozilla.org/mozilla-central/rev/12e876557ed4
https://hg.mozilla.org/mozilla-central/rev/15ce8ba0932c

Comment 154

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP: Bug 1207753 - netwerk/cache2 thread-safety annotations r=#necko-reviewers

Comment 155

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/77c6a01d5bbc
xpcom Timer thread-safety annotations r=nika

Comment 156

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ff35cafbdc2
ChannelEventQueue Thread-safety annotations r=necko-reviewers,valentin

Comment 157

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/77c6a01d5bbc
https://hg.mozilla.org/mozilla-central/rev/1ff35cafbdc2

Comment 158

[Andrew McCreight [:mccr8]]

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.

Comment 159

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - baseprofiler thread-safety annotations r=gerald

Comment 160

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - gfx thread-safety annotations r=#gfx-reviewers

Comment 161

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - InputStreamPump thread-safety annotations r=#necko-reviewers

Comment 162

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - nsHttpConnectionMgr thread-safety annotations r=#necko-reviewers

Comment 163

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - windows JumpListBuilder thread-safety annotations r=mhowell

Comment 164

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - netwerk nsWifiMonitor thread-safety annotations r=#necko-reviewers

Comment 165

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/328857c0299d
gfx thread-safety annotations r=gfx-reviewers,lsalzman

Comment 166

[Phabricator Automation]

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

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

Comment 167

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/74c9088f7922
windows JumpListBuilder thread-safety annotations r=mhowell

Comment 168

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c73e66ec8e14
script preloader thread-safety annotations r=mccr8

Comment 169

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/37388f5fbd2d
xpcom nsThreadPool thread-safety annotations r=nika

Comment 170

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e36fd04aa847
gfx & image thread-safety annotations r=gfx-reviewers,aosmond

Comment 171

[Phabricator Automation]

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

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

Comment 172

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e23f071eb6f0
image thread-safety annotations r=aosmond

Comment 173

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c0702641e5d3
security/certverifier thread-safety annotations r=keeler

Comment 174

[Phabricator Automation]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/74709c76387c
gfx/layers/apz thread-safety annotations r=botond

Comment 176

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/22dd9ee7e4a4
dom misc thread-safety annotations r=hsivonen,smaug,media-playback-reviewers,alwu

Comment 177

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a554b37a407
Smaller xpcom/threads & xpfe thread-safety annotations r=nika

Comment 178

[Natalia Csoregi [:nataliaCs]]

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

[Cristian Tuns]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd209cbc621b
baseprofiler thread-safety annotations r=gerald

Comment 181

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a3d2a5a43398
nsUrlClassifier thread-safety annotations r=dimi?

Comment 182

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ba8a5637b6ec
webaudio thread-safety annotations r=padenot

Comment 183

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/bd209cbc621b
https://hg.mozilla.org/mozilla-central/rev/a3d2a5a43398
https://hg.mozilla.org/mozilla-central/rev/ba8a5637b6ec

Comment 184

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec444f85837f
netwerk nsWifiMonitor thread-safety annotations r=necko-reviewers,dragana

Comment 185

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5d7eed758045
nsHttpConnectionMgr thread-safety annotations r=necko-reviewers,dragana

Comment 186

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/44ae041f0f9f
InputStreamPump thread-safety annotations r=necko-reviewers,dragana

Comment 187

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/95175b21b512
dom/base/BodyStream thread-safety annotations r=smaug

Comment 188

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a68541e609b7
WebSocket thread-safety annotations r=necko-reviewers,kershaw

Comment 189

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1207753 - xpcom base thread-safety annotations r=nika

Comment 190

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/ec444f85837f
https://hg.mozilla.org/mozilla-central/rev/5d7eed758045
https://hg.mozilla.org/mozilla-central/rev/44ae041f0f9f
https://hg.mozilla.org/mozilla-central/rev/95175b21b512
https://hg.mozilla.org/mozilla-central/rev/a68541e609b7

Comment 191

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1a6d4186a415
Various xpcom thread-safety annotations r=nika

Comment 192

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/685901935d66
dom/promises thread-safety annotations r=smaug

Comment 193

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/1a6d4186a415
https://hg.mozilla.org/mozilla-central/rev/685901935d66