[memory-infra] Make thread check at unregistration stricter for new dump providers

This CL makes the check for unregistering dump providers on right thread
stricter by checking even if tracing is not enabled. This is made
stricter only for new dump providers while the existing ones (except few
which are verified correct) are added to a blacklist which does not hit
this check. In next CLs this blacklist will be cleared not to cause
flakiness on the bots with lots of dump providers failing.

BUG=643438

Review-Url: https://codereview.chromium.org/2592233002
Cr-Commit-Position: refs/heads/master@{#442777}

base/trace_event/memory_dump_manager.cc

@@ -47,6 +47,45 @@ const unsigned char kTraceEventArgTypes[] = {TRACE_VALUE_TYPE_CONVERTABLE};
 StaticAtomicSequenceNumber g_next_guid;
 MemoryDumpManager* g_instance_for_testing = nullptr;
 
+// The list of names of dump providers that are blacklisted from strict thread
+// affinity check on unregistration. These providers could potentially cause
+// crashes on build bots if they do not unregister on right thread.
+// TODO(ssid): Fix all the dump providers to unregister if needed and clear the
+// blacklist, crbug.com/643438.
+const char* const kStrictThreadCheckBlacklist[] = {
+    "android::ResourceManagerImpl",
+    "AndroidGraphics",
+    "BrowserGpuMemoryBufferManager",
+    "ClientDiscardableSharedMemoryManager",
+    "ContextProviderCommandBuffer",
+    "DOMStorage",
+    "DiscardableSharedMemoryManager",
+    "FontCaches",
+    "GpuMemoryBufferVideoFramePool",
+    "IndexedDBBackingStore",
+    "LeveldbValueStore",
+    "MemoryCache",
+    "Skia",
+    "Sql",
+    "ThreadLocalEventBuffer",
+    "TraceLog",
+    "URLRequestContext",
+    "V8Isolate",
+    "VpxVideoDecoder",
+    "cc::ResourcePool",
+    "cc::ResourceProvider",
+    "cc::SoftwareImageDecodeCache",
+    "cc::StagingBufferPool",
+    "gpu::BufferManager",
+    "gpu::MappedMemoryManager",
+    "gpu::RenderbufferManager",
+    "gpu::TextureManager",
+    "gpu::TransferBufferManager",
+    "sql::Connection",
+    "SyncDirectory",
+    "BlacklistTestDumpProvider"  // for testing
+};
+
 // Callback wrapper to hook upon the completion of RequestGlobalDump() and
 // inject trace markers.
 void OnGlobalDumpDone(MemoryDumpCallback wrapped_callback,
@@ -138,6 +177,9 @@ MemoryDumpManager::MemoryDumpManager()
   // At this point the command line may not be initialized but we try to
   // enable the heap profiler to capture allocations as soon as possible.
   EnableHeapProfilingIfNeeded();
+
+  strict_thread_check_blacklist_.insert(std::begin(kStrictThreadCheckBlacklist),
+                                        std::end(kStrictThreadCheckBlacklist));
 }
 
 MemoryDumpManager::~MemoryDumpManager() {
@@ -339,7 +381,15 @@ void MemoryDumpManager::UnregisterDumpProviderInternal(
     // - When the provider is removed from |dump_providers_for_polling_|.
     DCHECK(!(*mdp_iter)->owned_dump_provider);
     (*mdp_iter)->owned_dump_provider = std::move(owned_mdp);
-  } else if (subtle::NoBarrier_Load(&memory_tracing_enabled_)) {
+  } else if (strict_thread_check_blacklist_.count((*mdp_iter)->name) == 0 ||
+             subtle::NoBarrier_Load(&memory_tracing_enabled_)) {
+    // If dump provider's name is on |strict_thread_check_blacklist_|, then the
+    // DCHECK is fired only when tracing is enabled. Otherwise the DCHECK is
+    // fired even when tracing is not enabled (stricter).
+    // TODO(ssid): Remove this condition after removing all the dump providers
+    // in the blacklist and the buildbots are no longer flakily hitting the
+    // DCHECK, crbug.com/643438.
+
     // If you hit this DCHECK, your dump provider has a bug.
     // Unregistration of a MemoryDumpProvider is safe only if:
     // - The MDP has specified a sequenced task runner affinity AND the

base/trace_event/memory_dump_manager.h

@@ -366,6 +366,11 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
   // Shared among all the PMDs to keep state scoped to the tracing session.
   scoped_refptr<MemoryDumpSessionState> session_state_;
 
+  // The list of names of dump providers that are blacklisted from strict thread
+  // affinity check on unregistration.
+  std::unordered_set<StringPiece, StringPieceHash>
+      strict_thread_check_blacklist_;
+
   MemoryDumpManagerDelegate* delegate_;  // Not owned.
 
   // When true, this instance is in charge of coordinating periodic dumps.

base/trace_event/memory_dump_manager_unittest.cc

@@ -1266,5 +1266,22 @@ TEST_F(MemoryDumpManagerTest, TestBackgroundTracingSetup) {
   DisableTracing();
 }
 
+TEST_F(MemoryDumpManagerTest, TestBlacklistedUnsafeUnregistration) {
+  InitializeMemoryDumpManager(false /* is_coordinator */);
+  MockMemoryDumpProvider mdp1;
+  RegisterDumpProvider(&mdp1, nullptr, kDefaultOptions,
+                       "BlacklistTestDumpProvider");
+  // Not calling UnregisterAndDeleteDumpProviderSoon() should not crash.
+  mdm_->UnregisterDumpProvider(&mdp1);
+
+  Thread thread("test thread");
+  thread.Start();
+  RegisterDumpProvider(&mdp1, thread.task_runner(), kDefaultOptions,
+                       "BlacklistTestDumpProvider");
+  // Unregistering on wrong thread should not crash.
+  mdm_->UnregisterDumpProvider(&mdp1);
+  thread.Stop();
+}
+
 }  // namespace trace_event
 }  // namespace base