Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(286)

Issue 194923004: Revert of Eliminate a potential race in IPC::ChannelProxy (Closed)

Created:
6 years, 9 months ago by johnme
Modified:
6 years, 9 months ago
CC:
chromium-reviews, darin-cc_chromium.org, jam
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

Revert of Eliminate a potential race in IPC::ChannelProxy (https://codereview.chromium.org/183553004/) Reason for revert: Since this has landed, testsuite content_browsertests is failing on bot Android Tests (dbg) on the chromium.linux waterfall. Specifically, the WebContentsImplBrowserTest.OpenURLSubframe test is consistently crashing, with the following DCHECK: [FATAL:device_orientation_message_filter.cc(18)] Check failed: BrowserThread::CurrentlyOn(BrowserThread::IO). This corresponds the the following DCHECK about being on the IO thread: DeviceOrientationMessageFilter::~DeviceOrientationMessageFilter() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); if (is_started_) DeviceInertialSensorService::GetInstance()->RemoveConsumer( CONSUMER_TYPE_ORIENTATION); } This same DCHECK failed in one of the try jobs on this CL: http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered_tests/builds/133648 Since this DCHECK has never been observed to fail before (certainly not in the last 200 builds), and has failed both in this CL's try job and twice in a row on the waterfall since landing this CL, it seems very likely that this CL is the cause. At a guess, the changes to ChannelProxy::Context::OnRemoveFilter seem quite relevant here. Original issue's description: > Eliminate a potential race in IPC::ChannelProxy > > Doing the following steps with ChannelProxy leads to a data race: > 1) Create the ChannelProxy, but don't initialize it. > 2) Add a filter. > 3) Init the ChannelProxy. > > The problem is, AddFilter() posts a task from the Listener thread to the IPC task runner to do OnAddFilter. Prior to this patch, OnAddFilter will try to read channel_ even though channel_ may not have been initialized, and it's accessed without any synchronization. > > This patch only really adds the filter if peer_pid_ has been set on the IPC::Channel thread; otherwise, it waits until the connection has been established to really add filters. > > See the bug for more detail. > > BUG=244383 > > Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256188 TBR=jam@chromium.org,dmichael@chromium.org NOTREECHECKS=true NOTRY=true BUG=244383 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256221

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+20 lines, -54 lines) Patch
M ipc/ipc_channel_nacl.h View 2 chunks +0 lines, -2 lines 0 comments Download
M ipc/ipc_channel_nacl.cc View 5 chunks +4 lines, -17 lines 0 comments Download
M ipc/ipc_channel_proxy.h View 1 chunk +0 lines, -4 lines 0 comments Download
M ipc/ipc_channel_proxy.cc View 5 chunks +16 lines, -31 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
johnme
Created Revert of Eliminate a potential race in IPC::ChannelProxy
6 years, 9 months ago (2014-03-11 13:53:42 UTC) #1
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/johnme@chromium.org/194923004/1
6 years, 9 months ago (2014-03-11 13:54:04 UTC) #2
commit-bot: I haz the power
6 years, 9 months ago (2014-03-11 13:54:55 UTC) #3
Message was sent while issue was closed.
Change committed as 256221

Powered by Google App Engine
This is Rietveld 408576698