Skip to content

Commit

Permalink
Fix std::map usage in IPC GetTaskRunner
Browse files Browse the repository at this point in the history
This CL fixes the incorrect usage of std::map.

The lookup in the map is using the [] operator which is creating a
default entry when the key is not present.

etienne@ first tried to land this fix but had to revert because some
tests became flaky (which was supposed to happen when we first
associated IPCs with per-frame task runners here
https://chromium-review.googlesource.com/c/chromium/src/+/1526067 ,
but was hidden because of this bug).
Test failure example:
https://ci.chromium.org/p/chromium/builders/ci/Linux%20ChromiumOS%20MSan%20Tests/13652

This is a second try to fix the incorrect map usage.

-- update --

The flaky tests were because of scheduling experiments that set the
background tasks to low priority, and this fix just surfaced the
issue because now tasks are posted to per-frame task runners.
CL to disable the experiments:
https://chromium-review.googlesource.com/c/chromium/src/+/1692577

Now that the experiments are disabled, this memory leak fix should
not make the tests flaky any more.

Bug: 973200, 980523
Change-Id: I1eaf0af67f1bcbd582a510d70174666251b8b4ef
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1686979
Commit-Queue: Yuzu Saijo <yuzus@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#676283}
  • Loading branch information
Yuzu Saijo authored and Commit Bot committed Jul 11, 2019
1 parent c22c65b commit 3b63300
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 11 deletions.
19 changes: 9 additions & 10 deletions ipc/ipc_channel_proxy.cc
Original file line number Diff line number Diff line change
Expand Up @@ -345,23 +345,22 @@ void ChannelProxy::Context::AddListenerTaskRunner(
void ChannelProxy::Context::RemoveListenerTaskRunner(int32_t routing_id) {
DCHECK(default_listener_task_runner_->BelongsToCurrentThread());
base::AutoLock lock(listener_thread_task_runners_lock_);
if (base::Contains(listener_thread_task_runners_, routing_id))
listener_thread_task_runners_.erase(routing_id);
listener_thread_task_runners_.erase(routing_id);
}

// Called on the IPC::Channel thread.
base::SingleThreadTaskRunner* ChannelProxy::Context::GetTaskRunner(
int32_t routing_id) {
scoped_refptr<base::SingleThreadTaskRunner>
ChannelProxy::Context::GetTaskRunner(int32_t routing_id) {
DCHECK(ipc_task_runner_->BelongsToCurrentThread());
if (routing_id == MSG_ROUTING_NONE)
return default_listener_task_runner_.get();
return default_listener_task_runner_;

base::AutoLock lock(listener_thread_task_runners_lock_);
base::SingleThreadTaskRunner* task_runner =
listener_thread_task_runners_[routing_id].get();
if (task_runner)
return task_runner;
return default_listener_task_runner_.get();
auto task_runner = listener_thread_task_runners_.find(routing_id);
if (task_runner == listener_thread_task_runners_.end())
return default_listener_task_runner_;
DCHECK(task_runner->second);
return task_runner->second;
}

// Called on the listener's thread
Expand Down
3 changes: 2 additions & 1 deletion ipc/ipc_channel_proxy.h
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,8 @@ class COMPONENT_EXPORT(IPC) ChannelProxy : public Sender {

// Called on the IPC::Channel thread.
// Returns the task runner associated with |routing_id|.
base::SingleThreadTaskRunner* GetTaskRunner(int32_t routing_id);
scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner(
int32_t routing_id);

protected:
friend class base::RefCountedThreadSafe<Context>;
Expand Down

0 comments on commit 3b63300

Please sign in to comment.