Bug 1758115 - Part 1: Clean up locking in ObservedDocShell, r=smaug

This simplifies some of the locking in ObservedDocShell, annotates the mutex,
and reduces the scope of the problematic critical section.

Differential Revision: https://phabricator.services.mozilla.com/D150442

docshell/base/timeline/MarkersStorage.cpp

@@ -1,20 +0,0 @@
-/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
-/* vim: set ts=8 sts=2 et sw=2 tw=80: */
-/* This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this
- * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-
-#include "MarkersStorage.h"
-#include "MainThreadUtils.h"
-
-namespace mozilla {
-
-MarkersStorage::MarkersStorage(const char* aMutexName) : mLock(aMutexName) {
-  MOZ_ASSERT(NS_IsMainThread());
-}
-
-MarkersStorage::~MarkersStorage() { MOZ_ASSERT(NS_IsMainThread()); }
-
-Mutex& MarkersStorage::GetLock() { return mLock; }
-
-}  // namespace mozilla

docshell/base/timeline/MarkersStorage.h

@@ -8,7 +8,7 @@
 #define mozilla_MarkersStorage_h_
 
 #include "TimelineMarkerEnums.h"  // for MarkerReleaseRequest
-#include "mozilla/Mutex.h"
+#include "MainThreadUtils.h"
 #include "mozilla/UniquePtr.h"
 #include "mozilla/LinkedList.h"
 #include "nsTArray.h"
@@ -21,26 +21,18 @@ struct ProfileTimelineMarker;
 }
 
 class MarkersStorage : public LinkedListElement<MarkersStorage> {
- private:
-  MarkersStorage() = delete;
+ public:
+  MarkersStorage() { MOZ_ASSERT(NS_IsMainThread()); }
+  virtual ~MarkersStorage() { MOZ_ASSERT(NS_IsMainThread()); }
+
   MarkersStorage(const MarkersStorage& aOther) = delete;
   void operator=(const MarkersStorage& aOther) = delete;
 
- public:
-  explicit MarkersStorage(const char* aMutexName);
-  virtual ~MarkersStorage();
-
   virtual void AddMarker(UniquePtr<AbstractTimelineMarker>&& aMarker) = 0;
   virtual void AddOTMTMarker(UniquePtr<AbstractTimelineMarker>&& aMarker) = 0;
   virtual void ClearMarkers() = 0;
   virtual void PopMarkers(JSContext* aCx,
                           nsTArray<dom::ProfileTimelineMarker>& aStore) = 0;
-
- protected:
-  Mutex& GetLock();
-
- private:
-  Mutex mLock MOZ_UNANNOTATED;
 };
 
 }  // namespace mozilla

docshell/base/timeline/ObservedDocShell.cpp

@@ -17,9 +17,7 @@
 namespace mozilla {
 
 ObservedDocShell::ObservedDocShell(nsIDocShell* aDocShell)
-    : MarkersStorage("ObservedDocShellMutex"),
-      mDocShell(aDocShell),
-      mPopping(false) {
+    : mDocShell(aDocShell), mLock("ObservedDocShellMutex") {
   MOZ_ASSERT(NS_IsMainThread());
 }
 
@@ -42,13 +40,13 @@ void ObservedDocShell::AddOTMTMarker(
   // avoid dealing with multithreading scenarios until all the markers are
   // actually cleared or popped in `ClearMarkers` or `PopMarkers`.
   MOZ_ASSERT(!NS_IsMainThread());
-  MutexAutoLock lock(GetLock());  // for `mOffTheMainThreadTimelineMarkers`.
+  MutexAutoLock lock(mLock);  // for `mOffTheMainThreadTimelineMarkers`.
   mOffTheMainThreadTimelineMarkers.AppendElement(std::move(aMarker));
 }
 
 void ObservedDocShell::ClearMarkers() {
   MOZ_ASSERT(NS_IsMainThread());
-  MutexAutoLock lock(GetLock());  // for `mOffTheMainThreadTimelineMarkers`.
+  MutexAutoLock lock(mLock);  // for `mOffTheMainThreadTimelineMarkers`.
   mTimelineMarkers.Clear();
   mOffTheMainThreadTimelineMarkers.Clear();
 }
@@ -56,16 +54,20 @@ void ObservedDocShell::ClearMarkers() {
 void ObservedDocShell::PopMarkers(
     JSContext* aCx, nsTArray<dom::ProfileTimelineMarker>& aStore) {
   MOZ_ASSERT(NS_IsMainThread());
-  MutexAutoLock lock(GetLock());  // for `mOffTheMainThreadTimelineMarkers`.
 
   MOZ_RELEASE_ASSERT(!mPopping);
   AutoRestore<bool> resetPopping(mPopping);
   mPopping = true;
 
-  // First, move all of our markers into a single array. We'll chose
-  // the `mTimelineMarkers` store because that's where we expect most of
-  // our markers to be.
-  mTimelineMarkers.AppendElements(std::move(mOffTheMainThreadTimelineMarkers));
+  {
+    MutexAutoLock lock(mLock);  // for `mOffTheMainThreadTimelineMarkers`.
+
+    // First, move all of our markers into a single array. We'll chose
+    // the `mTimelineMarkers` store because that's where we expect most of
+    // our markers to be, and we can access it without holding the lock.
+    mTimelineMarkers.AppendElements(
+        std::move(mOffTheMainThreadTimelineMarkers));
+  }
 
   // If we see an unpaired START, we keep it around for the next call
   // to ObservedDocShell::PopMarkers. We store the kept START objects here.

docshell/base/timeline/ObservedDocShell.h

@@ -9,6 +9,7 @@
 
 #include "MarkersStorage.h"
 #include "mozilla/RefPtr.h"
+#include "mozilla/Mutex.h"
 #include "mozilla/UniquePtr.h"
 #include "nsTArray.h"
 
@@ -31,10 +32,12 @@ class ObservedDocShell : public MarkersStorage {
 
   // Main thread only.
   nsTArray<UniquePtr<AbstractTimelineMarker>> mTimelineMarkers;
-  bool mPopping;
+  bool mPopping = false;
 
   // Off the main thread only.
-  nsTArray<UniquePtr<AbstractTimelineMarker>> mOffTheMainThreadTimelineMarkers;
+  Mutex mLock;
+  nsTArray<UniquePtr<AbstractTimelineMarker>> mOffTheMainThreadTimelineMarkers
+      GUARDED_BY(mLock);
 
  public:
   explicit ObservedDocShell(nsIDocShell* aDocShell);

docshell/base/timeline/moz.build

@@ -34,7 +34,6 @@ UNIFIED_SOURCES += [
     "AutoGlobalTimelineMarker.cpp",
     "AutoRestyleTimelineMarker.cpp",
     "AutoTimelineMarker.cpp",
-    "MarkersStorage.cpp",
     "ObservedDocShell.cpp",
     "TimelineConsumers.cpp",
     "TimelineMarker.cpp",