Gpu: Unregister channel name in the gpu process when a channel closes.

Gpu: Unregister channel name in the gpu process when a channel closes.

The Gpu channel name is derived from the GPU pid and the renderer id. When a render view crashes and is reloaded, a channel with the same pid and renderer id will be created in the gpu process, which causes this check to fail:

[36009:263:2890157912966265:FATAL:/Volumes/Data/thakis/chrome/src/ipc/ipc_channel_posix.cc(108)] Check failed: i == map_.end(). Creating second IPC server (fd 15) for '36009.r2' while first (fd 14) still exists
Backtrace:
	0   Chromium Framework                  0x00cafe62 base::debug::StackTrace::StackTrace() + 32
	1   Chromium Framework                  0x00cc6b20 logging::LogMessage::~LogMessage() + 64
	2   Chromium Framework                  0x017f6834 IPC::(anonymous namespace)::PipeMap::Insert(std::string const&, int) + 644
	3   Chromium Framework                  0x017f6887 IPC::AddChannelSocket(std::string const&, int) + 25
	4   Chromium Framework                  0x00b3efcb GpuChannel::Init() + 103
	5   Chromium Framework                  0x00b470ff GpuThread::OnEstablishChannel(int) + 407
	6   Chromium Framework                  0x00b4778d void DispatchToMethod<GpuThread, void (GpuThread::*)(int), int>(GpuThread*, void (GpuThread::*)(int), Tuple1<int> const&) + 65
	7   Chromium Framework                  0x00b48c52 bool IPC::MessageWithTuple<Tuple1<int> >::Dispatch<GpuThread, void (GpuThread::*)(int)>(IPC::Message const*, GpuThread*, void (GpuThread::*)(int)) + 71
	8   Chromium Framework                  0x00b472f8 GpuThread::OnControlMessageReceived(IPC::Message const&) + 146
	9   Chromium Framework                  0x001696f6 ChildThread::OnMessageReceived(IPC::Message const&) + 452
	10  Chromium Framework                  0x017f9bcc IPC::ChannelProxy::Context::OnDispatchMessage(IPC::Message const&) + 144
	11  Chromium Framework                  0x017fab64 void DispatchToMethod<IPC::ChannelProxy::Context, void (IPC::ChannelProxy::Context::*)(IPC::Message const&), IPC::Message>(IPC::ChannelProxy::Context*, void (IPC::ChannelProxy::Context::*)(IPC::Message const&), Tuple1<IPC::Message> const&) + 63
	12  Chromium Framework                  0x017fab9f RunnableMethod<IPC::ChannelProxy::Context, void (IPC::ChannelProxy::Context::*)(IPC::Message const&), Tuple1<IPC::Message> >::Run() + 57
	13  Chromium Framework                  0x00ccdc39 MessageLoop::RunTask(Task*) + 303
	14  Chromium Framework                  0x00ccdcf1 MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) + 53
	15  Chromium Framework                  0x00ccdfcf MessageLoop::DoWork() + 253
	16  Chromium Framework                  0x00d42024 base::MessagePumpCFRunLoopBase::RunWork() + 74
	17  Chromium Framework                  0x00d42069 base::MessagePumpCFRunLoopBase::RunWorkSource(void*) + 23
	18  CoreFoundation                      0x96f260fb __CFRunLoopDoSources0 + 1563
	19  CoreFoundation                      0x96f23bbf __CFRunLoopRun + 1071
	20  CoreFoundation                      0x96f23094 CFRunLoopRunSpecific + 452
	21  CoreFoundation                      0x96f22ec1 CFRunLoopRunInMode + 97
	22  HIToolbox                           0x976c2f9c RunCurrentEventLoopInMode + 392
	23  HIToolbox                           0x976c2d51 ReceiveNextEventCommon + 354
	24  HIToolbox                           0x976c2bd6 BlockUntilNextEventMatchingListInMode + 81
	25  AppKit                              0x920b5a89 _DPSNextEvent + 847
	26  AppKit                              0x920b52ca -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 156
	27  AppKit                              0x9207755b -[NSApplication run] + 821
	28  Chromium Framework                  0x00d41b66 base::MessagePumpNSApplication::DoRun(base::MessagePump::Delegate*) + 130
	29  Chromium Framework                  0x00d42155 base::MessagePumpCFRunLoopBase::Run(base::MessagePump::Delegate*) + 175
	30  Chromium Framework                  0x00cce7db MessageLoop::RunInternal() + 209
	31  Chromium Framework                  0x00cce7f5 MessageLoop::RunHandler() + 17
	32  Chromium Framework                  0x00cce859 MessageLoop::Run() + 35
	33  Chromium Framework                  0x00b466e6 GpuMain(MainFunctionParams const&) + 1123
	34  Chromium Framework                  0x00008d67 ChromeMain + 4921
	35  Chromium Helper                     0x00001f52 main + 24
	36  Chromium Helper                     0x00001f0e start + 54
	37  ???                                 0x00000003 0x0 + 3

The fix is to unregister the channel-name mapping when the GPU channel goes away.

BUG=55641
TEST=Go to a gpu-accelerated page. Enter about:crash in omnibox. Reload page. No CHECK should be printed, and the page should reload fine.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=66304

[Nico, 2010-11-16 15:13:05]

This probably fixes the same problem for linux.

[Nico, 2010-11-16 15:15:26]

By "This probably fixes the same problem for linux." I mean that I set
out to fix a mac bug with this CL, but from reading the code I think
that the same bug exists on linux, and will be fixed by this CL as
well.

On Tue, Nov 16, 2010 at 4:13 PM,  <thakis@chromium.org> wrote:
> Reviewers: piman, kbr,
>
> Message:
> This probably fixes the same problem for linux.
>
> Description:
> Gpu: Unregister channel name in the gpu process when a channel closes.
>
> The Gpu channel name is derived from the GPU pid and the renderer id. When a
> render view crashes and is reloaded, a channel with the same pid and
> renderer id
> will be created in the gpu process, which causes this check to fail:
>
>
[36009:263:2890157912966265:FATAL:/Volumes/Data/thakis/chrome/src/ipc/ipc_channel_posix.cc(108)]
> Check failed: i == map_.end(). Creating second IPC server (fd 15) for
> '36009.r2'
> while first (fd 14) still exists
> Backtrace:
>        0   Chromium Framework                  0x00cafe62
> base::debug::StackTrace::StackTrace() + 32
>        1   Chromium Framework                  0x00cc6b20
> logging::LogMessage::~LogMessage() + 64
>        2   Chromium Framework                  0x017f6834 IPC::(anonymous
> namespace)::PipeMap::Insert(std::string const&, int) + 644
>        3   Chromium Framework                  0x017f6887
> IPC::AddChannelSocket(std::string const&, int) + 25
>        4   Chromium Framework                  0x00b3efcb GpuChannel::Init()
> + 103
>        5   Chromium Framework                  0x00b470ff
> GpuThread::OnEstablishChannel(int) + 407
>        6   Chromium Framework                  0x00b4778d void
> DispatchToMethod<GpuThread, void (GpuThread::*)(int), int>(GpuThread*, void
> (GpuThread::*)(int), Tuple1<int> const&) + 65
>        7   Chromium Framework                  0x00b48c52 bool
> IPC::MessageWithTuple<Tuple1<int> >::Dispatch<GpuThread, void
> (GpuThread::*)(int)>(IPC::Message const*, GpuThread*, void
> (GpuThread::*)(int))
> + 71
>        8   Chromium Framework                  0x00b472f8
> GpuThread::OnControlMessageReceived(IPC::Message const&) + 146
>        9   Chromium Framework                  0x001696f6
> ChildThread::OnMessageReceived(IPC::Message const&) + 452
>        10  Chromium Framework                  0x017f9bcc
> IPC::ChannelProxy::Context::OnDispatchMessage(IPC::Message const&) + 144
>        11  Chromium Framework                  0x017fab64 void
> DispatchToMethod<IPC::ChannelProxy::Context, void
> (IPC::ChannelProxy::Context::*)(IPC::Message const&),
> IPC::Message>(IPC::ChannelProxy::Context*, void
> (IPC::ChannelProxy::Context::*)(IPC::Message const&), Tuple1<IPC::Message>
> const&) + 63
>        12  Chromium Framework                  0x017fab9f
> RunnableMethod<IPC::ChannelProxy::Context, void
> (IPC::ChannelProxy::Context::*)(IPC::Message const&), Tuple1<IPC::Message>
>>
>> ::Run() + 57
>
>        13  Chromium Framework                  0x00ccdc39
> MessageLoop::RunTask(Task*)
> + 303
>        14  Chromium Framework                  0x00ccdcf1
> MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) + 53
>        15  Chromium Framework                  0x00ccdfcf
> MessageLoop::DoWork() + 253
>        16  Chromium Framework                  0x00d42024
> base::MessagePumpCFRunLoopBase::RunWork() + 74
>        17  Chromium Framework                  0x00d42069
> base::MessagePumpCFRunLoopBase::RunWorkSource(void*) + 23
>        18  CoreFoundation                      0x96f260fb
> __CFRunLoopDoSources0 + 1563
>        19  CoreFoundation                      0x96f23bbf __CFRunLoopRun +
> 1071
>        20  CoreFoundation                      0x96f23094
> CFRunLoopRunSpecific + 452
>        21  CoreFoundation                      0x96f22ec1 CFRunLoopRunInMode
> + 97
>        22  HIToolbox                           0x976c2f9c
> RunCurrentEventLoopInMode +
> 392
>        23  HIToolbox                           0x976c2d51
> ReceiveNextEventCommon + 354
>        24  HIToolbox                           0x976c2bd6
> BlockUntilNextEventMatchingListInMode + 81
>        25  AppKit                              0x920b5a89 _DPSNextEvent +
> 847
>        26  AppKit                              0x920b52ca -[NSApplication
> nextEventMatchingMask:untilDate:inMode:dequeue:] + 156
>        27  AppKit                              0x9207755b -[NSApplication
> run] + 821
>        28  Chromium Framework                  0x00d41b66
> base::MessagePumpNSApplication::DoRun(base::MessagePump::Delegate*) + 130
>        29  Chromium Framework                  0x00d42155
> base::MessagePumpCFRunLoopBase::Run(base::MessagePump::Delegate*) + 175
>        30  Chromium Framework                  0x00cce7db
> MessageLoop::RunInternal() +
> 209
>        31  Chromium Framework                  0x00cce7f5
> MessageLoop::RunHandler() +
> 17
>        32  Chromium Framework                  0x00cce859 MessageLoop::Run()
> + 35
>        33  Chromium Framework                  0x00b466e6
> GpuMain(MainFunctionParams
> const&) + 1123
>        34  Chromium Framework                  0x00008d67 ChromeMain + 4921
>        35  Chromium Helper                     0x00001f52 main + 24
>        36  Chromium Helper                     0x00001f0e start + 54
>        37  ???                                 0x00000003 0x0 + 3
>
> The fix is to unregister the channel-name mapping when the GPU channel goes
> away.
>
> BUG=55641
> TEST=Go to a gpu-accelerated page. Enter about:crash in omnibox. Reload
> page. No
> CHECK should be printed, and the page should reload fine.
>
> Please review this at http://codereview.chromium.org/4979005/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  M chrome/gpu/gpu_channel.cc
>
>
> Index: chrome/gpu/gpu_channel.cc
> diff --git a/chrome/gpu/gpu_channel.cc b/chrome/gpu/gpu_channel.cc
> index
>
d5525ed0d3230f920979a07ed1a991e1c6c8cefa..8d0387433930e0e672118bd935a48e248e049315
> 100644
> --- a/chrome/gpu/gpu_channel.cc
> +++ b/chrome/gpu/gpu_channel.cc
> @@ -65,6 +65,9 @@ void GpuChannel::OnMessageReceived(const IPC::Message&
> message) {
>
>  void GpuChannel::OnChannelError() {
>
> static_cast<GpuThread*>(ChildThread::current())->RemoveChannel(renderer_id_);
> +#if defined(OS_POSIX)
> +  IPC::RemoveAndCloseChannelSocket(GetChannelName());
> +#endif
>  }
>
>  bool GpuChannel::Send(IPC::Message* message) {
>
>
>

[apatrick, 2010-11-16 17:48:54]

http://codereview.chromium.org/4979005/diff/1/chrome/gpu/gpu_channel.cc
File chrome/gpu/gpu_channel.cc (right):

http://codereview.chromium.org/4979005/diff/1/chrome/gpu/gpu_channel.cc#newco...
chrome/gpu/gpu_channel.cc:69:
IPC::RemoveAndCloseChannelSocket(GetChannelName());
I think this is the right thing to do but I wonder if calling
RemoveAndCloseChannelCocket should have symmetry with AddChannelSocket, which is
called in Init. Did you try putting it in the GpuChannel destructor? I'm
concerned about the channel being closed for reasons other than an error.

[Nico, 2010-11-16 18:54:27]

On Tue, Nov 16, 2010 at 6:48 PM,  <apatrick@google.com> wrote:
>
> http://codereview.chromium.org/4979005/diff/1/chrome/gpu/gpu_channel.cc
> File chrome/gpu/gpu_channel.cc (right):
>
>
http://codereview.chromium.org/4979005/diff/1/chrome/gpu/gpu_channel.cc#newco...
> chrome/gpu/gpu_channel.cc:69:
> IPC::RemoveAndCloseChannelSocket(GetChannelName());
> I think this is the right thing to do but I wonder if calling
> RemoveAndCloseChannelCocket should have symmetry with AddChannelSocket,
> which is called in Init. Did you try putting it in the GpuChannel
> destructor? I'm concerned about the channel being closed for reasons
> other than an error.

That probably works as well. I've put it there because that's where
chrome/plugin/plugin_channel_base.cc put it.

Done.

>
> http://codereview.chromium.org/4979005/
>

[Nico, 2010-11-16 18:55:14]

On Tue, Nov 16, 2010 at 7:54 PM, Nico Weber <thakis@chromium.org> wrote:
> On Tue, Nov 16, 2010 at 6:48 PM,  <apatrick@google.com> wrote:
>>
>> http://codereview.chromium.org/4979005/diff/1/chrome/gpu/gpu_channel.cc
>> File chrome/gpu/gpu_channel.cc (right):
>>
>>
http://codereview.chromium.org/4979005/diff/1/chrome/gpu/gpu_channel.cc#newco...
>> chrome/gpu/gpu_channel.cc:69:
>> IPC::RemoveAndCloseChannelSocket(GetChannelName());
>> I think this is the right thing to do but I wonder if calling
>> RemoveAndCloseChannelCocket should have symmetry with AddChannelSocket,
>> which is called in Init. Did you try putting it in the GpuChannel
>> destructor? I'm concerned about the channel being closed for reasons
>> other than an error.
>
> That probably works as well. I've put it there because that's where
> chrome/plugin/plugin_channel_base.cc put it.

(And by "probably works as well" I mean I tried it, and it did in fact work.)

>
> Done.
>
>>
>> http://codereview.chromium.org/4979005/
>>
>

[Ken Russell (switch to Gerrit), 2010-11-16 19:01:39]

LGTM