Add ownership edges between HostSharedBitmap and shared memory

content/common/host_shared_bitmap_manager.cc

 // Copyright 2013 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 "content/common/host_shared_bitmap_manager.h"
 
 #include <stdint.h>
 
 #include <utility>
 
 #include "base/lazy_instance.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/shared_memory_tracker.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/trace_event/process_memory_dump.h"
 #include "build/build_config.h"
 #include "content/common/view_messages.h"
 #include "ui/gfx/geometry/size.h"
 
 namespace content {
 
 class BitmapData : public base::RefCountedThreadSafe<BitmapData> {
  public:
   explicit BitmapData(size_t buffer_size) : buffer_size(buffer_size) {}
   std::unique_ptr<base::SharedMemory> memory;
   std::unique_ptr<uint8_t[]> pixels;
   size_t buffer_size;
 
  private:
   friend class base::RefCountedThreadSafe<BitmapData>;
   ~BitmapData() {}
   DISALLOW_COPY_AND_ASSIGN(BitmapData);
 };
 
 namespace {
 
 class HostSharedBitmap : public cc::SharedBitmap {
  public:
   HostSharedBitmap(uint8_t* pixels,
                    scoped_refptr<BitmapData> bitmap_data,
                    const cc::SharedBitmapId& id,
                    HostSharedBitmapManager* manager)
-      : SharedBitmap(pixels, id),
+      : SharedBitmap(pixels, nullptr, id),
         bitmap_data_(bitmap_data),
         manager_(manager) {}
 
   ~HostSharedBitmap() override {
     if (manager_)
       manager_->FreeSharedMemoryFromMap(id());
   }
 
  private:
   scoped_refptr<BitmapData> bitmap_data_;
@@ skipping 104 matching lines @@
     const base::trace_event::MemoryDumpArgs& args,
     base::trace_event::ProcessMemoryDump* pmd) {
   base::AutoLock lock(lock_);
 
   for (const auto& bitmap : handle_map_) {
     base::trace_event::MemoryAllocatorDump* dump =
         pmd->CreateAllocatorDump(base::StringPrintf(
             "sharedbitmap/%s",
             base::HexEncode(bitmap.first.name, sizeof(bitmap.first.name))
                 .c_str()));
     if (!dump)

[ssid, 2017/03/31 00:25:21]

I think you can remove this condition.

       return false;
 
     dump->AddScalar(base::trace_event::MemoryAllocatorDump::kNameSize,
                     base::trace_event::MemoryAllocatorDump::kUnitsBytes,
                     bitmap.second->buffer_size);
 
-    // Generate a global GUID used to share this allocation with renderer
-    // processes.
-    auto guid = cc::GetSharedBitmapGUIDForTracing(bitmap.first);
-    pmd->CreateSharedGlobalAllocatorDump(guid);
-    pmd->AddOwnershipEdge(dump->guid(), guid);
+    if (bitmap.second->memory) {

[ssid, 2017/03/31 00:25:21]

When would the shared_memory field be null?
What happens before this change id it was null? We still dumped the memory and
added edges?

I am thinking if you should move this condition to the beginning of the loop and
not create the "sharedbitmap/xxx" at all since it will not have any edge and the
actual memory backing does not exist.

+      base::SharedMemoryTracker::AddOwnershipEdges(
+          pmd, dump->guid(), bitmap.second->memory->handle());

[ssid, 2017/03/31 00:25:21]

You have updated only the browser side dumps to use the shared_memory edges.
But, the "cc/resource_manager/X" dumps will still create global dump edges which
will match the id given by cc::GetSharedBitmapGUIDForTracing().

I think the right order is first being able to detect if the memory should be
attributes to the current process or not. Then move all the clients of
shared_memory at once. Otherwise, we will end up with a situation where the
memory is attributed equally to all processes sharing the segment. This would
still break telemetry.

+    }
   }
 
   return true;
 }
 
 bool HostSharedBitmapManager::ChildAllocatedSharedBitmap(
     size_t buffer_size,
     const base::SharedMemoryHandle& handle,
     const cc::SharedBitmapId& id) {
   base::AutoLock lock(lock_);
@@ skipping 27 matching lines @@
 
   scoped_refptr<BitmapData> data(new BitmapData(buffer_size));
   data->memory = std::move(shared_memory);
 
   handle_map_[id] = data;
   if (!data->memory->ShareToProcess(process_handle, shared_memory_handle)) {
     LOG(ERROR) << "Cannot share shared memory buffer";
     *shared_memory_handle = base::SharedMemory::NULLHandle();
     return;
   }
- data->memory->Close();
+  data->memory->Close();
 }
 
 void HostSharedBitmapManager::ChildDeletedSharedBitmap(
     const cc::SharedBitmapId& id) {
   base::AutoLock lock(lock_);
   handle_map_.erase(id);
 }
 
 size_t HostSharedBitmapManager::AllocatedBitmapCount() const {
   base::AutoLock lock(lock_);
   return handle_map_.size();
 }
 
 void HostSharedBitmapManager::FreeSharedMemoryFromMap(
     const cc::SharedBitmapId& id) {
   base::AutoLock lock(lock_);
   handle_map_.erase(id);
 }
 
 }  // namespace content