Skip to content

Commit

Permalink
Get rid of base::SharedMemory::NULLHandle();
Browse files Browse the repository at this point in the history
This CL is a refactor and has no intended behavior change.

NullHandle() has two use cases:
  * Create an invalid handle.
  * Return an object that can be compared against a handle to check validity.
The former is also the behavior of the default constructor of
base::SharedMemoryHandle, and the latter should be done with the member
IsValid().

Fixing the latter also allows the removal of operator== and operator!=.

BUG=713763
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2849633002
Cr-Commit-Position: refs/heads/master@{#468231}
  • Loading branch information
erikchen authored and Commit bot committed Apr 29, 2017
1 parent e7a3d64 commit a40775d
Show file tree
Hide file tree
Showing 36 changed files with 38 additions and 124 deletions.
8 changes: 3 additions & 5 deletions base/memory/shared_memory.h
Original file line number Diff line number Diff line change
Expand Up @@ -92,17 +92,15 @@ class BASE_EXPORT SharedMemory {
// invalid value; NULL for a HANDLE and -1 for a file descriptor)
static bool IsHandleValid(const SharedMemoryHandle& handle);

// Returns invalid handle (see comment above for exact definition).
static SharedMemoryHandle NULLHandle();

// Closes a shared memory handle.
static void CloseHandle(const SharedMemoryHandle& handle);

// Returns the maximum number of handles that can be open at once per process.
static size_t GetHandleLimit();

// Duplicates The underlying OS primitive. Returns NULLHandle() on failure.
// The caller is responsible for destroying the duplicated OS primitive.
// Duplicates The underlying OS primitive. Returns an invalid handle on
// failure. The caller is responsible for destroying the duplicated OS
// primitive.
static SharedMemoryHandle DuplicateHandle(const SharedMemoryHandle& handle);

#if defined(OS_POSIX)
Expand Down
8 changes: 0 additions & 8 deletions base/memory/shared_memory_handle.h
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,6 @@ class BASE_EXPORT SharedMemoryHandle {
mach_vm_size_t size,
base::ProcessId pid);

// Comparison operators.
bool operator==(const SharedMemoryHandle& handle) const;
bool operator!=(const SharedMemoryHandle& handle) const;

// Duplicates the underlying OS resources.
SharedMemoryHandle Duplicate() const;

Expand All @@ -106,10 +102,6 @@ class BASE_EXPORT SharedMemoryHandle {
#elif defined(OS_WIN)
SharedMemoryHandle(HANDLE h, base::ProcessId pid);

// Comparison operators.
bool operator==(const SharedMemoryHandle& handle) const;
bool operator!=(const SharedMemoryHandle& handle) const;

// Whether |pid_| is the same as the current process's id.
bool BelongsToCurrentProcess() const;

Expand Down
20 changes: 0 additions & 20 deletions base/memory/shared_memory_handle_mac.cc
Original file line number Diff line number Diff line change
Expand Up @@ -91,26 +91,6 @@ SharedMemoryHandle SharedMemoryHandle::Duplicate() const {
}
}

bool SharedMemoryHandle::operator==(const SharedMemoryHandle& handle) const {
if (!IsValid() && !handle.IsValid())
return true;

if (type_ != handle.type_)
return false;

switch (type_) {
case POSIX:
return file_descriptor_.fd == handle.file_descriptor_.fd;
case MACH:
return memory_object_ == handle.memory_object_ && size_ == handle.size_ &&
pid_ == handle.pid_;
}
}

bool SharedMemoryHandle::operator!=(const SharedMemoryHandle& handle) const {
return !(*this == handle);
}

bool SharedMemoryHandle::IsValid() const {
switch (type_) {
case POSIX:
Expand Down
12 changes: 0 additions & 12 deletions base/memory/shared_memory_handle_win.cc
Original file line number Diff line number Diff line change
Expand Up @@ -30,18 +30,6 @@ SharedMemoryHandle& SharedMemoryHandle::operator=(
return *this;
}

bool SharedMemoryHandle::operator==(const SharedMemoryHandle& handle) const {
// Invalid handles are always equal.
if (!IsValid() && !handle.IsValid())
return true;

return handle_ == handle.handle_ && pid_ == handle.pid_;
}

bool SharedMemoryHandle::operator!=(const SharedMemoryHandle& handle) const {
return !(*this == handle);
}

void SharedMemoryHandle::Close() const {
DCHECK(handle_ != nullptr);
DCHECK(BelongsToCurrentProcess());
Expand Down
5 changes: 0 additions & 5 deletions base/memory/shared_memory_mac.cc
Original file line number Diff line number Diff line change
Expand Up @@ -108,11 +108,6 @@ bool SharedMemory::IsHandleValid(const SharedMemoryHandle& handle) {
return handle.IsValid();
}

// static
SharedMemoryHandle SharedMemory::NULLHandle() {
return SharedMemoryHandle();
}

// static
void SharedMemory::CloseHandle(const SharedMemoryHandle& handle) {
handle.Close();
Expand Down
5 changes: 0 additions & 5 deletions base/memory/shared_memory_nacl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,6 @@ bool SharedMemory::IsHandleValid(const SharedMemoryHandle& handle) {
return handle.IsValid();
}

// static
SharedMemoryHandle SharedMemory::NULLHandle() {
return SharedMemoryHandle();
}

// static
void SharedMemory::CloseHandle(const SharedMemoryHandle& handle) {
DCHECK(handle.IsValid());
Expand Down
5 changes: 0 additions & 5 deletions base/memory/shared_memory_posix.cc
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,6 @@ bool SharedMemory::IsHandleValid(const SharedMemoryHandle& handle) {
return handle.IsValid();
}

// static
SharedMemoryHandle SharedMemory::NULLHandle() {
return SharedMemoryHandle();
}

// static
void SharedMemory::CloseHandle(const SharedMemoryHandle& handle) {
DCHECK(handle.IsValid());
Expand Down
5 changes: 0 additions & 5 deletions base/memory/shared_memory_win.cc
Original file line number Diff line number Diff line change
Expand Up @@ -170,11 +170,6 @@ bool SharedMemory::IsHandleValid(const SharedMemoryHandle& handle) {
return handle.IsValid();
}

// static
SharedMemoryHandle SharedMemory::NULLHandle() {
return SharedMemoryHandle();
}

// static
void SharedMemory::CloseHandle(const SharedMemoryHandle& handle) {
handle.Close();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -440,7 +440,7 @@ void DiscardableSharedMemoryManager::AllocateLockedDiscardableSharedMemory(
MemorySegmentMap& client_segments = clients_[client_id];
if (client_segments.find(id) != client_segments.end()) {
LOG(ERROR) << "Invalid discardable shared memory ID";
*shared_memory_handle = base::SharedMemory::NULLHandle();
*shared_memory_handle = base::SharedMemoryHandle();
return;
}

Expand All @@ -461,14 +461,14 @@ void DiscardableSharedMemoryManager::AllocateLockedDiscardableSharedMemory(
std::unique_ptr<base::DiscardableSharedMemory> memory(
new base::DiscardableSharedMemory);
if (!memory->CreateAndMap(size)) {
*shared_memory_handle = base::SharedMemory::NULLHandle();
*shared_memory_handle = base::SharedMemoryHandle();
return;
}

base::CheckedNumeric<size_t> checked_bytes_allocated = bytes_allocated_;
checked_bytes_allocated += memory->mapped_size();
if (!checked_bytes_allocated.IsValid()) {
*shared_memory_handle = base::SharedMemory::NULLHandle();
*shared_memory_handle = base::SharedMemoryHandle();
return;
}

Expand Down
7 changes: 2 additions & 5 deletions components/nacl/common/nacl_types.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,7 @@ NaClStartParams::NaClStartParams()
#endif
validation_cache_enabled(false),
enable_debug_stub(false),
process_type(kUnknownNaClProcessType),
crash_info_shmem_handle(base::SharedMemory::NULLHandle()) {
process_type(kUnknownNaClProcessType) {
}

NaClStartParams::NaClStartParams(const NaClStartParams& other) = default;
Expand Down Expand Up @@ -91,9 +90,7 @@ NaClLaunchResult::NaClLaunchResult()
: ppapi_ipc_channel_handle(),
trusted_ipc_channel_handle(),
plugin_pid(base::kNullProcessId),
plugin_child_id(0),
crash_info_shmem_handle(base::SharedMemory::NULLHandle()) {
}
plugin_child_id(0) {}

NaClLaunchResult::NaClLaunchResult(
const IPC::ChannelHandle& ppapi_ipc_channel_handle,
Expand Down
4 changes: 1 addition & 3 deletions components/nacl/renderer/nexe_load_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,7 @@ std::string LookupAttribute(const std::map<std::string, std::string>& args,

} // namespace

NexeLoadManager::NexeLoadManager(
PP_Instance pp_instance)
NexeLoadManager::NexeLoadManager(PP_Instance pp_instance)
: pp_instance_(pp_instance),
nacl_ready_state_(PP_NACL_READY_STATE_UNSENT),
nexe_error_reported_(false),
Expand All @@ -91,7 +90,6 @@ NexeLoadManager::NexeLoadManager(
nexe_size_(0),
plugin_instance_(content::PepperPluginInstance::Get(pp_instance)),
nonsfi_(false),
crash_info_shmem_handle_(base::SharedMemory::NULLHandle()),
weak_factory_(this) {
set_exit_status(-1);
SetLastError("");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ class StubClient : public media::VideoCaptureDevice::Client {
VideoFrame::WrapExternalSharedMemory(
media::PIXEL_FORMAT_I420, format.frame_size, visible_rect,
format.frame_size, buffer_access->data(),
buffer_access->mapped_size(), base::SharedMemory::NULLHandle(), 0u,
buffer_access->mapped_size(), base::SharedMemoryHandle(), 0u,
base::TimeDelta());
const gfx::Point center = visible_rect.CenterPoint();
const int center_offset_y =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ TEST_F(ClipboardMessageFilterTest, ImageSizeOverflows32BitRowBytes) {
}

TEST_F(ClipboardMessageFilterTest, InvalidSharedMemoryHandle) {
CallWriteImageDirectly(gfx::Size(5, 5), base::SharedMemory::NULLHandle());
CallWriteImageDirectly(gfx::Size(5, 5), base::SharedMemoryHandle());
uint64_t sequence_number =
clipboard()->GetSequenceNumber(ui::CLIPBOARD_TYPE_COPY_PASTE);
CallCommitWrite();
Expand Down
1 change: 0 additions & 1 deletion content/browser/renderer_host/render_message_filter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,6 @@ void RenderMessageFilter::SendLoadFontReply(IPC::Message* reply,
if (result->font_data_size == 0 || result->font_id == 0) {
result->font_data_size = 0;
result->font_id = 0;
handle = base::SharedMemory::NULLHandle();
} else {
result->font_data.GiveToProcess(base::GetCurrentProcessHandle(), &handle);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2869,8 +2869,8 @@ TEST_F(RenderWidgetHostViewAuraTest, DiscardDelegatedFrames) {
display_compositor::HostSharedBitmapManager::current());

for (size_t i = 0; i < (renderer_count - 1) * handles_per_frame; i++) {
bitmap_client.ChildAllocatedSharedBitmap(
1, base::SharedMemory::NULLHandle(), cc::SharedBitmap::GenerateId());
bitmap_client.ChildAllocatedSharedBitmap(1, base::SharedMemoryHandle(),
cc::SharedBitmap::GenerateId());
}

// Hiding this last bitmap should evict all but two frames.
Expand Down
4 changes: 2 additions & 2 deletions content/renderer/media/audio_message_filter_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class MockAudioDelegate : public media::AudioOutputIPCDelegate {
device_status_ = media::OUTPUT_DEVICE_STATUS_ERROR_INTERNAL;

created_received_ = false;
handle_ = base::SharedMemory::NULLHandle();
handle_ = base::SharedMemoryHandle();
length_ = 0;

volume_received_ = false;
Expand Down Expand Up @@ -130,7 +130,7 @@ TEST(AudioMessageFilterTest, Basic) {
const uint32_t kLength = 1024;
EXPECT_FALSE(delegate.created_received());
filter->OnMessageReceived(AudioMsg_NotifyStreamCreated(
kStreamId, base::SharedMemory::NULLHandle(), socket_descriptor, kLength));
kStreamId, base::SharedMemoryHandle(), socket_descriptor, kLength));
EXPECT_TRUE(delegate.created_received());
EXPECT_FALSE(base::SharedMemory::IsHandleValid(delegate.handle()));
EXPECT_EQ(kLength, delegate.length());
Expand Down
2 changes: 1 addition & 1 deletion content/renderer/pepper/mock_renderer_ppapi_host.cc
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ base::SharedMemoryHandle
MockRendererPpapiHost::ShareSharedMemoryHandleWithRemote(
const base::SharedMemoryHandle& handle) {
NOTIMPLEMENTED();
return base::SharedMemory::NULLHandle();
return base::SharedMemoryHandle();
}

bool MockRendererPpapiHost::IsRunningInProcess() const { return false; }
Expand Down
6 changes: 2 additions & 4 deletions content/renderer/pepper/pepper_audio_input_host.cc
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,7 @@ void PepperAudioInputHost::StreamCreated(
}

void PepperAudioInputHost::StreamCreationFailed() {
OnOpenComplete(PP_ERROR_FAILED,
base::SharedMemory::NULLHandle(),
0,
OnOpenComplete(PP_ERROR_FAILED, base::SharedMemoryHandle(), 0,
base::SyncSocket::kInvalidHandle);
}

Expand Down Expand Up @@ -144,7 +142,7 @@ void PepperAudioInputHost::OnOpenComplete(
if (result == PP_OK) {
IPC::PlatformFileForTransit temp_socket =
IPC::InvalidPlatformFileForTransit();
base::SharedMemoryHandle temp_shmem = base::SharedMemory::NULLHandle();
base::SharedMemoryHandle temp_shmem;
result = GetRemoteHandles(
scoped_socket, scoped_shared_memory, &temp_socket, &temp_shmem);

Expand Down
4 changes: 2 additions & 2 deletions content/renderer/pepper/pepper_audio_output_host.cc
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ void PepperAudioOutputHost::StreamCreated(
}

void PepperAudioOutputHost::StreamCreationFailed() {
OnOpenComplete(PP_ERROR_FAILED, base::SharedMemory::NULLHandle(), 0,
OnOpenComplete(PP_ERROR_FAILED, base::SharedMemoryHandle(), 0,
base::SyncSocket::kInvalidHandle);
}

Expand Down Expand Up @@ -182,7 +182,7 @@ void PepperAudioOutputHost::OnOpenComplete(
if (result == PP_OK) {
IPC::PlatformFileForTransit temp_socket =
IPC::InvalidPlatformFileForTransit();
base::SharedMemoryHandle temp_shmem = base::SharedMemory::NULLHandle();
base::SharedMemoryHandle temp_shmem;
result = GetRemoteHandles(scoped_socket, scoped_shared_memory, &temp_socket,
&temp_shmem);

Expand Down
3 changes: 1 addition & 2 deletions content/renderer/renderer_blink_platform_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -546,8 +546,7 @@ bool RendererBlinkPlatformImpl::SandboxSupport::LoadFont(NSFont* src_font,
return false;
}

if (font_data_size == 0 || font_data == base::SharedMemory::NULLHandle() ||
*font_id == 0) {
if (font_data_size == 0 || !font_data.IsValid() || *font_id == 0) {
LOG(ERROR) << "Bad response from RenderProcessHostMsg_LoadFont() for " <<
src_font_descriptor.font_name;
*out = NULL;
Expand Down
2 changes: 1 addition & 1 deletion gpu/ipc/client/gpu_channel_host.cc
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ void GpuChannelHost::RemoveRoute(int route_id) {
base::SharedMemoryHandle GpuChannelHost::ShareToGpuProcess(
const base::SharedMemoryHandle& source_handle) {
if (IsLost())
return base::SharedMemory::NULLHandle();
return base::SharedMemoryHandle();

return base::SharedMemory::DuplicateHandle(source_handle);
}
Expand Down
3 changes: 1 addition & 2 deletions media/base/video_frame.cc
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ scoped_refptr<VideoFrame> VideoFrame::WrapExternalData(
base::TimeDelta timestamp) {
return WrapExternalStorage(format, STORAGE_UNOWNED_MEMORY, coded_size,
visible_rect, natural_size, data, data_size,
timestamp, base::SharedMemory::NULLHandle(), 0);
timestamp, base::SharedMemoryHandle(), 0);
}

// static
Expand Down Expand Up @@ -925,7 +925,6 @@ VideoFrame::VideoFrame(VideoPixelFormat format,
coded_size_(coded_size),
visible_rect_(visible_rect),
natural_size_(natural_size),
shared_memory_handle_(base::SharedMemory::NULLHandle()),
shared_memory_offset_(0),
timestamp_(timestamp),
unique_id_(g_unique_id_generator.GetNext()) {
Expand Down
2 changes: 1 addition & 1 deletion media/capture/content/thread_safe_capture_oracle.cc
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ bool ThreadSafeCaptureOracle::ObserveEventAndDecideCapture(
*storage = VideoFrame::WrapExternalSharedMemory(
params_.requested_format.pixel_format, coded_size,
gfx::Rect(visible_size), visible_size, output_buffer_access->data(),
output_buffer_access->mapped_size(), base::SharedMemory::NULLHandle(), 0u,
output_buffer_access->mapped_size(), base::SharedMemoryHandle(), 0u,
base::TimeDelta());
// If creating the VideoFrame wrapper failed, call DidCaptureFrame() with
// !success to execute the required post-capture steps (tracing, notification
Expand Down
4 changes: 2 additions & 2 deletions mojo/edk/embedder/embedder.h
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,8 @@ CreateSharedBufferWrapper(base::SharedMemoryHandle shared_memory_handle,
// |MojoHandle| and closes the handle. If successful, |num_bytes| will contain
// the size of the shared memory buffer and |read_only| will contain whether the
// buffer handle is read-only. Both |num_bytes| and |read_only| may be null.
// Note: The value of |shared_memory_handle| may be
// base::SharedMemory::NULLHandle(), even if this function returns success.
// Note: |shared_memory_handle| may be invalid even if this function returns
// success.
// Callers should perform appropriate checks.
MOJO_SYSTEM_IMPL_EXPORT MojoResult
PassSharedMemoryHandle(MojoHandle mojo_handle,
Expand Down
2 changes: 1 addition & 1 deletion ppapi/nacl_irt/ppapi_dispatcher.cc
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ IPC::PlatformFileForTransit PpapiDispatcher::ShareHandleWithRemote(
base::SharedMemoryHandle PpapiDispatcher::ShareSharedMemoryHandleWithRemote(
const base::SharedMemoryHandle& handle,
base::ProcessId remote_pid) {
return base::SharedMemory::NULLHandle();
return base::SharedMemoryHandle();
}

std::set<PP_Instance>* PpapiDispatcher::GetGloballySeenInstanceIDSet() {
Expand Down
2 changes: 1 addition & 1 deletion ppapi/proxy/gamepad_resource.cc
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ void GamepadResource::Sample(PP_Instance /* instance */,
void GamepadResource::OnPluginMsgSendMemory(
const ResourceMessageReplyParams& params) {
// On failure, the handle will be null and the CHECK below will be tripped.
base::SharedMemoryHandle handle = base::SharedMemory::NULLHandle();
base::SharedMemoryHandle handle;
params.TakeSharedMemoryHandleAtIndex(0, &handle);

shared_memory_.reset(new base::SharedMemory(handle, true));
Expand Down
2 changes: 1 addition & 1 deletion ppapi/proxy/media_stream_track_resource_base.cc
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ void MediaStreamTrackResourceBase::OnPluginMsgInitBuffers(
int32_t number_of_buffers,
int32_t buffer_size,
bool readonly) {
base::SharedMemoryHandle shm_handle = base::SharedMemory::NULLHandle();
base::SharedMemoryHandle shm_handle;
params.TakeSharedMemoryHandleAtIndex(0, &shm_handle);
buffer_manager_.SetBuffers(number_of_buffers, buffer_size,
std::unique_ptr<base::SharedMemory>(
Expand Down
Loading

0 comments on commit a40775d

Please sign in to comment.