Skip to content

Commit

Permalink
Add SharedMemory::mapped_id()
Browse files Browse the repository at this point in the history
When SharedMemory::Close is called, SharedMemoryHandle is cleared and
losts its ID. This means SharedMemory(Handle)'s ID is not available
after closing.

In DiscardableSharedMemory, shared memory is closed without unmmap-ing
just to save file descriptor resources, and this makes
SharedMemoryTracker confused since SharedmemoryTracker tracks mmap-ed
(and not unmmap-ed) shared memory regions, and tries to get IDs of
those regions. The problem is that sometimes SharedMemoryTarcker fails
to get such IDs.

This CL fixes this problem by adding SharedMemory::mapped_id() to make
the id available even after Close is called.

Bug: 604726, 740781
Change-Id: I5dbf6bd133d8a1e935adad53604f3f40da3c4a6f
Reviewed-on: https://chromium-review.googlesource.com/566353
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Commit-Queue: Hajime Hoshi <hajimehoshi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486375}
  • Loading branch information
Hajime Hoshi authored and Commit Bot committed Jul 13, 2017
1 parent 1c81cac commit 5d64433
Show file tree
Hide file tree
Showing 8 changed files with 46 additions and 15 deletions.
6 changes: 6 additions & 0 deletions base/memory/shared_memory.h
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,11 @@ class BASE_EXPORT SharedMemory {
// failure.
SharedMemoryHandle GetReadOnlyHandle();

// Returns an ID for the mapped region. This is ID of the SharedMemoryHandle
// that was mapped. The ID is valid even after the SharedMemoryHandle is
// Closed, as long as the region is not unmapped.
const UnguessableToken& mapped_id() const { return mapped_id_; }

private:
#if defined(OS_POSIX) && !defined(OS_NACL) && !defined(OS_ANDROID) && \
!defined(OS_FUCHSIA) && (!defined(OS_MACOSX) || defined(OS_IOS))
Expand Down Expand Up @@ -240,6 +245,7 @@ class BASE_EXPORT SharedMemory {
void* memory_ = nullptr;
bool read_only_ = false;
size_t requested_size_ = 0;
base::UnguessableToken mapped_id_;

DISALLOW_COPY_AND_ASSIGN(SharedMemory);
};
Expand Down
2 changes: 2 additions & 0 deletions base/memory/shared_memory_fuchsia.cc
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ bool SharedMemory::MapAt(off_t offset, size_t bytes) {
memory_ = reinterpret_cast<void*>(addr);

mapped_size_ = bytes;
mapped_id_ = shm_.GetGUID();
SharedMemoryTracker::GetInstance()->IncrementMemoryUsage(*this);
return true;
}
Expand All @@ -106,6 +107,7 @@ bool SharedMemory::Unmap() {
}

memory_ = nullptr;
mapped_id_ = UnguessableToken();
return true;
}

Expand Down
2 changes: 2 additions & 0 deletions base/memory/shared_memory_mac.cc
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ bool SharedMemory::MapAt(off_t offset, size_t bytes) {
DCHECK_EQ(0U, reinterpret_cast<uintptr_t>(memory_) &
(SharedMemory::MAP_MINIMUM_ALIGNMENT - 1));
mapped_memory_mechanism_ = shm_.type_;
mapped_id_ = shm_.GetGUID();
SharedMemoryTracker::GetInstance()->IncrementMemoryUsage(*this);
} else {
memory_ = NULL;
Expand All @@ -216,6 +217,7 @@ bool SharedMemory::Unmap() {
}
memory_ = NULL;
mapped_size_ = 0;
mapped_id_ = UnguessableToken();
return true;
}

Expand Down
2 changes: 2 additions & 0 deletions base/memory/shared_memory_nacl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ bool SharedMemory::MapAt(off_t offset, size_t bytes) {
mapped_size_ = bytes;
DCHECK_EQ(0U, reinterpret_cast<uintptr_t>(memory_) &
(SharedMemory::MAP_MINIMUM_ALIGNMENT - 1));
mapped_id_ = shm_.GetGUID();
SharedMemoryTracker::GetInstance()->IncrementMemoryUsage(*this);
} else {
memory_ = NULL;
Expand All @@ -103,6 +104,7 @@ bool SharedMemory::Unmap() {
DPLOG(ERROR) << "munmap";
memory_ = NULL;
mapped_size_ = 0;
mapped_id_ = UnguessableToken();
return true;
}

Expand Down
2 changes: 2 additions & 0 deletions base/memory/shared_memory_posix.cc
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,7 @@ bool SharedMemory::MapAt(off_t offset, size_t bytes) {
bool mmap_succeeded = memory_ != (void*)-1 && memory_ != NULL;
if (mmap_succeeded) {
mapped_size_ = bytes;
mapped_id_ = shm_.GetGUID();
DCHECK_EQ(0U,
reinterpret_cast<uintptr_t>(memory_) &
(SharedMemory::MAP_MINIMUM_ALIGNMENT - 1));
Expand All @@ -300,6 +301,7 @@ bool SharedMemory::Unmap() {
munmap(memory_, mapped_size_);
memory_ = NULL;
mapped_size_ = 0;
mapped_id_ = UnguessableToken();
return true;
}

Expand Down
20 changes: 5 additions & 15 deletions base/memory/shared_memory_tracker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ namespace base {
namespace {

std::string GetDumpNameForTracing(const UnguessableToken& id) {
DCHECK(!id.is_empty());
return "shared_memory/" + id.ToString();
}

Expand Down Expand Up @@ -57,29 +58,18 @@ void SharedMemoryTracker::DecrementMemoryUsage(

bool SharedMemoryTracker::OnMemoryDump(const trace_event::MemoryDumpArgs& args,
trace_event::ProcessMemoryDump* pmd) {
std::vector<std::tuple<UnguessableToken, uintptr_t, size_t>> usages;
std::vector<std::tuple<UnguessableToken, size_t>> usages;
{
AutoLock hold(usages_lock_);
usages.reserve(usages_.size());
for (const auto& usage : usages_) {
usages.emplace_back(usage.first->handle().GetGUID(),
reinterpret_cast<uintptr_t>(usage.first->memory()),
usage.second);
usages.emplace_back(usage.first->mapped_id(), usage.second);
}
}
for (const auto& usage : usages) {
const UnguessableToken& memory_guid = std::get<0>(usage);
uintptr_t address = std::get<1>(usage);
size_t size = std::get<2>(usage);
std::string dump_name;
if (memory_guid.is_empty()) {
// TODO(hajimehoshi): As passing ID across mojo is not implemented yet
// (crbug/713763), ID can be empty. For such case, use an address instead
// of GUID so that approximate memory usages are available.
dump_name = "shared_memory/" + Uint64ToString(address);
} else {
dump_name = GetDumpNameForTracing(memory_guid);
}
size_t size = std::get<1>(usage);
std::string dump_name = GetDumpNameForTracing(memory_guid);
// Discard duplicates that might be seen in single-process mode.
if (pmd->GetAllocatorDump(dump_name))
continue;
Expand Down
25 changes: 25 additions & 0 deletions base/memory/shared_memory_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -734,4 +734,29 @@ MULTIPROCESS_TEST_MAIN(SharedMemoryTestMain) {
#endif // !defined(OS_IOS) && !defined(OS_ANDROID) && !defined(OS_MACOSX) &&
// !defined(OS_FUCHSIA)

TEST(SharedMemoryTest, MappedId) {
const uint32_t kDataSize = 1024;
SharedMemory memory;
SharedMemoryCreateOptions options;
options.size = kDataSize;
#if defined(OS_MACOSX) && !defined(OS_IOS)
// The Mach functionality is tested in shared_memory_mac_unittest.cc.
options.type = SharedMemoryHandle::POSIX;
#endif

EXPECT_TRUE(memory.Create(options));
base::UnguessableToken id = memory.handle().GetGUID();
EXPECT_FALSE(id.is_empty());
EXPECT_TRUE(memory.mapped_id().is_empty());

EXPECT_TRUE(memory.Map(kDataSize));
EXPECT_EQ(id, memory.mapped_id());

memory.Close();
EXPECT_EQ(id, memory.mapped_id());

memory.Unmap();
EXPECT_TRUE(memory.mapped_id().is_empty());
}

} // namespace base
2 changes: 2 additions & 0 deletions base/memory/shared_memory_win.cc
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,7 @@ bool SharedMemory::MapAt(off_t offset, size_t bytes) {
DCHECK_EQ(0U, reinterpret_cast<uintptr_t>(memory_) &
(SharedMemory::MAP_MINIMUM_ALIGNMENT - 1));
mapped_size_ = GetMemorySectionSize(memory_);
mapped_id_ = shm_.GetGUID();
SharedMemoryTracker::GetInstance()->IncrementMemoryUsage(*this);
return true;
}
Expand All @@ -323,6 +324,7 @@ bool SharedMemory::Unmap() {
SharedMemoryTracker::GetInstance()->DecrementMemoryUsage(*this);
UnmapViewOfFile(memory_);
memory_ = NULL;
mapped_id_ = UnguessableToken();
return true;
}

Expand Down

0 comments on commit 5d64433

Please sign in to comment.