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

base/trace_event/memory_dump_manager.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/trace_event/memory_dump_manager.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/allocator/features.h"
@@ skipping 29 matching lines @@
 
 namespace {
 
 const int kTraceEventNumArgs = 1;
 const char* kTraceEventArgNames[] = {"dumps"};
 const unsigned char kTraceEventArgTypes[] = {TRACE_VALUE_TYPE_CONVERTABLE};
 
 StaticAtomicSequenceNumber g_next_guid;
 MemoryDumpManager* g_instance_for_testing = nullptr;
 
+// The list of dump providers that are blacklisted from strict checks when
+// unregistration. These providers could potentially cause crashes on build bots

[Primiano Tucci (use gerrit), 2017/01/06 14:00:39]

either "checks on unregistration" or "checks when unregistering".

[ssid, 2017/01/10 23:59:38]

Done.

+// 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* kStrictThreadCheckBlacklist[] = {

[Primiano Tucci (use gerrit), 2017/01/06 14:00:39]

+const (consts are never enough), i.e.
const char* const kStrictThreadCheckBlacklist, so would make the array actually
immutable (As the kNamingNotation suggests). Otherwise you end up with a mutable
array to immutable char pointers.

[ssid, 2017/01/10 23:59:38]

Sorry forgot const here.

+    "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
+};
+
+// Returns true if the name is present in the blacklist.
+bool IsDumpproviderBlackListedForStrictThreadCheck(const char* name) {

[Primiano Tucci (use gerrit), 2017/01/06 14:00:39]

my suggestion is that you keep the array above and:

1)  add a std::unordered_set<base::StringPiece, base::StringPieceHash> to the
MDM instance.

2) In the MDM ctor you do:
thread_check_blacklist_.insert(std::begin(kArray), std::end(kArray)) (or
something like that, not sure if this is possible in one op with stringpiece)

3) and then below during unregistration, where you call this function you just
do
thread_check_blacklist_.count((*mdp_iter)->name)

[ssid, 2017/01/10 23:59:38]

Done.

+  for (auto* str : kStrictThreadCheckBlacklist) {
+    if (strcmp(name, str) == 0)
+      return true;
+  }
+  return false;
+}
+
 // Callback wrapper to hook upon the completion of RequestGlobalDump() and
 // inject trace markers.
 void OnGlobalDumpDone(MemoryDumpCallback wrapped_callback,
                       uint64_t dump_guid,
                       bool success) {
   TRACE_EVENT_NESTABLE_ASYNC_END1(
       MemoryDumpManager::kTraceCategory, "GlobalMemoryDump",
       TRACE_ID_MANGLE(dump_guid), "success", success);
 
   if (!wrapped_callback.is_null()) {
@@ skipping 272 matching lines @@
     return;  // Not registered / already unregistered.
 
   if (take_mdp_ownership_and_delete_async) {
     // The MDP will be deleted whenever the MDPInfo struct will, that is either:
     // - At the end of this function, if no dump is in progress.
     // - Either in SetupNextMemoryDump() or InvokeOnMemoryDump() when MDPInfo is
     //   removed from |pending_dump_providers|.
     // - 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 (!IsDumpproviderBlackListedForStrictThreadCheck(
+                 (*mdp_iter)->name) ||
+             subtle::NoBarrier_Load(&memory_tracing_enabled_)) {
+    // If IsDumpproviderBlackListedForStrictThreadCheck() is true, 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
     //   unregistration happens on the same task runner. So that the MDP cannot
     //   unregister and be in the middle of a OnMemoryDump() at the same time.
     // - The MDP has NOT specified a task runner affinity and its ownership is
     //   transferred via UnregisterAndDeleteDumpProviderSoon().
     // In all the other cases, it is not possible to guarantee that the
     // unregistration will not race with OnMemoryDump() calls.
     DCHECK((*mdp_iter)->task_runner &&
@@ skipping 603 matching lines @@
   if (heavy_dump_rate_ > 0 && periodic_dumps_count_ % heavy_dump_rate_ == 0)
     level_of_detail = MemoryDumpLevelOfDetail::DETAILED;
   ++periodic_dumps_count_;
 
   MemoryDumpManager::GetInstance()->RequestGlobalDump(
       MemoryDumpType::PERIODIC_INTERVAL, level_of_detail);
 }
 
 }  // namespace trace_event
 }  // namespace base