Replace SharedMemory::UniqueID usages with SharedMemoryHandle's guid

base/memory/shared_memory_tracker.cc

Index: base/memory/shared_memory_tracker.cc
diff --git a/base/memory/shared_memory_tracker.cc b/base/memory/shared_memory_tracker.cc
index 8613f595336ada19357c900cf0ab9bf4d243b7f4..3880f34466dc4da0ba8dc3acc26f57be187e3867 100644
--- a/base/memory/shared_memory_tracker.cc
+++ b/base/memory/shared_memory_tracker.cc
@@ -5,18 +5,11 @@
 #include "base/memory/shared_memory_tracker.h"
 
 #include "base/memory/shared_memory.h"
-#include "base/strings/stringprintf.h"
 #include "base/trace_event/memory_dump_manager.h"
 #include "base/trace_event/process_memory_dump.h"
 
 namespace base {
 
-SharedMemoryTracker::Usage::Usage() = default;
-
-SharedMemoryTracker::Usage::Usage(const Usage& rhs) = default;
-
-SharedMemoryTracker::Usage::~Usage() = default;
-
 // static
 SharedMemoryTracker* SharedMemoryTracker::GetInstance() {
   static SharedMemoryTracker* instance = new SharedMemoryTracker;
@@ -25,19 +18,8 @@ SharedMemoryTracker* SharedMemoryTracker::GetInstance() {
 
 void SharedMemoryTracker::IncrementMemoryUsage(
     const SharedMemory& shared_memory) {
-  Usage usage;
-  // |shared_memory|'s unique ID must be generated here and it'd be too late at
-  // OnMemoryDump. An ID is generated with a SharedMemoryHandle, but the handle
-  // might already be closed at that time. Now IncrementMemoryUsage is called
-  // just after mmap and the handle must live then. See the discussion at
-  // crbug.com/604726#c30.
-  SharedMemory::UniqueId id;
-  if (!shared_memory.GetUniqueId(&id))
-    return;
-  usage.unique_id = id;
-  usage.size = shared_memory.mapped_size();
   AutoLock hold(usages_lock_);
-  usages_[&shared_memory] = usage;
+  usages_[&shared_memory] = shared_memory.mapped_size();
 }
 
 void SharedMemoryTracker::DecrementMemoryUsage(
@@ -48,31 +30,31 @@ void SharedMemoryTracker::DecrementMemoryUsage(
 
 bool SharedMemoryTracker::OnMemoryDump(const trace_event::MemoryDumpArgs& args,
                                        trace_event::ProcessMemoryDump* pmd) {
-  std::unordered_map<SharedMemory::UniqueId, size_t, SharedMemory::UniqueIdHash>
-      sizes;
-  {
-    AutoLock hold(usages_lock_);
-    for (const auto& usage : usages_)
-      sizes[usage.second.unique_id] += usage.second.size;
-  }
-  for (auto& size : sizes) {
-    const SharedMemory::UniqueId& id = size.first;
-    std::string dump_name = StringPrintf("%s/%lld.%lld", "shared_memory",
-                                         static_cast<long long>(id.first),
-                                         static_cast<long long>(id.second));
-    auto guid = trace_event::MemoryAllocatorDumpGuid(dump_name);
+  AutoLock hold(usages_lock_);

[danakj, 2017/05/09 14:54:18]

wdyt of copying the usages_ into a local map with the lock held, then doing the
rest of the work without the lock?

[hajimehoshi, 2017/05/10 05:55:22]

I don't have a strong opinion: OnMemoryDump is called only when memory-infra is
enabled at chrome://tracing, so I don't think the current implementation would
be problematic. On the other hand, avoiding locks in any situations seems a good
convention.

OK, I adopt copying strategy. Done.

[Primiano Tucci (use gerrit), 2017/05/10 18:33:55]

This is true right now, but very soon we want to use this for UMA. Not a hot
path, but this is going to be invoked every ~some minutes.
This is racy now. since you don't own the SharedMemory object, at this point you
have no guarantee that the SharedMemory* pointer will be valid. If you get a
call in the meantime to DecrementMemoryUsage this will crash for UAF.
I think you actually have to iterate while holding the lock, and populate a
vector of (GUID, address, size) tuples, and then fill the dump after releasing
the lock (which I think is what implicitly Dana was suggesting)

+  for (auto& usage : usages_) {
+    auto guid = usage.first->GetGUID();
+    // TODO(hajimehoshi): As passing ID across mojo is not implemented yet

[danakj, 2017/05/09 14:54:18]

Since this patch effectively doesn't work until the mojo part is done, and then
you have to come back and remove this if, maybe it's better to do the mojo piece
before landing this?

[hajimehoshi, 2017/05/10 05:55:22]

Right, but the current memory-infra doesn't show reliable values regarding
shared memory anyway (crbug/713768), and this fix will take long time (<Q3?). I
think the current situation of memory-infra doesn't change even if this CL is
committed now.

primiano@, what do you think?

[Primiano Tucci (use gerrit), 2017/05/10 18:33:55]

I think we want this before the mojo fix comes (sadly there seems to be some
timing issue due to API stability of mojo).
erikchen and I discussed and even without the precise accounting, we can have an
approximation based on the assumption that shared memory is shared only between
two processes, so we can just /2 (which is a reasonable assumption).

However I wouldn't have the continue below. if you do this, shared_memory is
gong to report nothing until mojo is fixed. Instead I would just have a fallback
for the dump_name (use the address  of the mmaped shared memory if mapped, or
something else, e.g. a monotonic counter, if not mapped)
this will allow the approximation mentioned above

+    // (crbug/713763), ID can be empty.
+    if (guid.is_empty())
+      continue;
+    std::string dump_name = "shared_memory/" + guid.ToString();
+    auto dump_guid = trace_event::MemoryAllocatorDumpGuid(dump_name);
+    // The dump might already be created on single-process mode.
     trace_event::MemoryAllocatorDump* local_dump =
-        pmd->CreateAllocatorDump(dump_name);
+        pmd->GetAllocatorDump(dump_name);
+    if (local_dump)
+      continue;
+    local_dump = pmd->GetOrCreateAllocatorDump(dump_name);
     // TODO(hajimehoshi): The size is not resident size but virtual size so far.
     // Fix this to record resident size.
     local_dump->AddScalar(trace_event::MemoryAllocatorDump::kNameSize,
                           trace_event::MemoryAllocatorDump::kUnitsBytes,
-                          size.second);
+                          usage.second);
     trace_event::MemoryAllocatorDump* global_dump =
-        pmd->CreateSharedGlobalAllocatorDump(guid);
+        pmd->CreateSharedGlobalAllocatorDump(dump_guid);
     global_dump->AddScalar(trace_event::MemoryAllocatorDump::kNameSize,
                            trace_event::MemoryAllocatorDump::kUnitsBytes,
-                           size.second);
+                           usage.second);
     // TOOD(hajimehoshi): Detect which the shared memory comes from browser,
     // renderer or GPU process.
     // TODO(hajimehoshi): Shared memory reported by GPU and discardable is