Add CHECK on file descriptor in various IPC::ChannelHandle passed in.

Add CHECK on file descriptor in various IPC::ChannelHandle passed in.

Regarding Chromium issues 73355, 95129, 95732, 97285, 103957 and Chromium-os issue 18437, 22372, we suspect the channel handles passed to the renderer have invalid file descriptors (fd). This is supported by the fact that using a channel handle with a valid name but an invalid fd will produce crashes with exactly the same stack trace as reported in these issues. Running out of fd in either the renderer, browser or the other process (GPU, broker, etc) could cause this to happen, but we are not sure if that's the real cause.

Adding check for the fd in various places to help investigate these issues further. We will be able to tell if invalid fd is passed in and if yes, which process generates it. Browser side check is only added for the broker case to limit the scale of bad user experience, while providing enough cases for investigation.

BUG=none
TEST=passed unit tests

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

[xhwang, 2011-11-30 19:28:09]

Hello ddorwin, piman and jam,

This is a small change to add check on the file descriptor in channel handle.  I
suspect this is why we have crashes in CreatePipe in Chromium issues 95732,
97285, 95129 and Chromium-os issue 22372, some of which are top chromium crashes
right now.  If this works, we can go ahead to investigate why the fd is invalid.
 If not, we can at least rule out some possibilities.

-xhwang

[piman, 2011-11-30 19:39:25]

Could we CHECK() instead of silently failing? That way we'd get more accurate
crash reports and it'd be more obvious that this is what's happening.
I would also add CHECK() on the sender side. It sounded like the sender may be
sending a bad fd.

[xhwang, 2011-11-30 20:12:52]

On 2011/11/30 19:39:25, piman wrote:
> Could we CHECK() instead of silently failing? That way we'd get more accurate
> crash reports and it'd be more obvious that this is what's happening.
> I would also add CHECK() on the sender side. It sounded like the sender may be
> sending a bad fd.

Adding CHECK() would be good for tracing bug but bad for user experience. Do you
agree we could add CHECK() to keep tracing these issues but they should be
removed in the future?

I don't know how GPU process works.  But will adding CHECK() on the sender side
cause the GPU process to die and affect all tabs that depend on the GPU process?

If these issues are really caused by running out of fds. Do we have better way
to deal with the situation rather than declaring failure and not crashing? Or we
should actually work towards reducing fd usage and avoid exhausting fds from
happening in the first place?

[piman, 2011-11-30 21:11:01]

On Wed, Nov 30, 2011 at 12:12 PM, <xhwang@chromium.org> wrote:

> On 2011/11/30 19:39:25, piman wrote:
>
>> Could we CHECK() instead of silently failing? That way we'd get more
>> accurate
>> crash reports and it'd be more obvious that this is what's happening.
>> I would also add CHECK() on the sender side. It sounded like the sender
>> may be
>> sending a bad fd.
>>
>
> Adding CHECK() would be good for tracing bug but bad for user experience.
> Do you
> agree we could add CHECK() to keep tracing these issues but they should be
> removed in the future?
>

Yes, of course.


> I don't know how GPU process works.  But will adding CHECK() on the sender
> side
> cause the GPU process to die and affect all tabs that depend on the GPU
> process?
>

Renderers are supposed to handle it.

Note: the browser is involved in sending the channel, and I'm debating
CHECKing there too. It's a worse experience because the whole browser
crashes. But it would pinpoint to whether or not the browser is causing
this.

If these issues are really caused by running out of fds. Do we have better
> way
> to deal with the situation rather than declaring failure and not crashing?


Well first we need to properly transmit the accurate information.
If the gpu process is running out of fds, maybe the browser process could
create another one, but the gpu process would need to tell it so (by e.g.
returning failure from the message creating the channel instead of
succeeding with an invalid channel).
If the browser process is the one running out of fds, we should look at why
and fix that. I don't think it's supposed to have this many long lived open
fds. Maybe there's a leak.
If the renderer process is the one running out, there's not much we can do,
but it would be good to track down and see if there's a leak too.


> Or we
> should actually work towards reducing fd usage and avoid exhausting fds
> from
> happening in the first place?
>

We certainly should work towards that too, if that is indeed the problem.

>
>
http://codereview.chromium.**org/8735015/<http://codereview.chromium.org/8735...
>

[xhwang, 2011-12-01 00:05:35]

On 2011/11/30 21:11:01, piman wrote:
> On Wed, Nov 30, 2011 at 12:12 PM, <mailto:xhwang@chromium.org> wrote:
> 
> > On 2011/11/30 19:39:25, piman wrote:
> >
> >> Could we CHECK() instead of silently failing? That way we'd get more
> >> accurate
> >> crash reports and it'd be more obvious that this is what's happening.
> >> I would also add CHECK() on the sender side. It sounded like the sender
> >> may be
> >> sending a bad fd.
> >>
> >
> > Adding CHECK() would be good for tracing bug but bad for user experience.
> > Do you
> > agree we could add CHECK() to keep tracing these issues but they should be
> > removed in the future?
> >
> 
> Yes, of course.
> 
> 
> > I don't know how GPU process works.  But will adding CHECK() on the sender
> > side
> > cause the GPU process to die and affect all tabs that depend on the GPU
> > process?
> >
> 
> Renderers are supposed to handle it.

I experimented with this a bit.  I have the GPU process crashed on purpose when
more than 5 renderer tries to connect to it.  The previous four opened tabs are
all playing HTML5 video.  After the GPU process crashes, a new GPU process is
loaded quickly, which is good.  But the previous opened tabs cannot
continue/resume to play the video normally.  They all show a white/blank window
with the progress bar still moving.  So I am not sure how well the renderer can
handle this situation.

> 
> Note: the browser is involved in sending the channel, and I'm debating
> CHECKing there too. It's a worse experience because the whole browser
> crashes. But it would pinpoint to whether or not the browser is causing
> this.

I don't know enough about if it's possible for browser to run out of fd.  But in
Chromium issue 103957, we probably have the same invalid fd issue.  I don't
believe there are a lot of process using the broker, so where the invalid fd is
coming from still intrigues me.  If the browser could run out of fd, that
explains it.  But again, if that happens, won't the browser go terribly wrong?
We should also see other crashes related to this to justify this hypothesis.

In terms of if we should add a CHECK in the browser.  It should depend on what's
the chance the browser run out of fd.  I don't have enough knowledge on this and
someone needs to make the call.

> 
> If these issues are really caused by running out of fds. Do we have better
> > way
> > to deal with the situation rather than declaring failure and not crashing?
> 
> 
> Well first we need to properly transmit the accurate information.
> If the gpu process is running out of fds, maybe the browser process could
> create another one, but the gpu process would need to tell it so (by e.g.
> returning failure from the message creating the channel instead of
> succeeding with an invalid channel).

I suppose providing an invalid channel is the way to signal failure (at least in
the broker case).

> If the browser process is the one running out of fds, we should look at why
> and fix that. I don't think it's supposed to have this many long lived open
> fds. Maybe there's a leak.
> If the renderer process is the one running out, there's not much we can do,
> but it would be good to track down and see if there's a leak too.
> 

To avoid bad user experience, why don't we add check in the browser only for the
broker.  The broker crash is happening almost everyday but on average only for
<10 cases.  So we will have enough reports everyday but don't affect too many
users.  See:
http://crash/search?query=product%3A%22Chrome_ChromeOS%22+function_name%3A%22....

> 
> > Or we
> > should actually work towards reducing fd usage and avoid exhausting fds
> > from
> > happening in the first place?
> >
> 
> We certainly should work towards that too, if that is indeed the problem.
> 
> >
> >
>
http://codereview.chromium.**org/8735015/%3Chttp://codereview.chromium.org/87...>
> >

[jam, 2011-12-01 03:56:34]

I defer to Antoine who's an owner of content\renderer

In general, we prefer to CHECK. we'll get reports early (before it even gets to
stable channel, i.e. from dev/beta users) and fix it.

[xhwang, 2011-12-01 18:09:41]

Hello ddorwin and piman,

As discussed, added CHECK on fd for all gpu, npapi plugin and broker cases. See
updated change description for details. Please review.

-xhwang

[piman, 2011-12-01 18:25:31]

LGTM. Please make sure the referenced bugs are marked as ReleaseBlock-Beta, so
that we don't forget the CHECKs and accidentally ship with them :)

[xhwang, 2011-12-01 19:25:16]

hello jam,

Could you please do an OWNERS review on the following two files:

content/browser/renderer_host/render_message_filter.cc
content/common/gpu/gpu_channel_manager.cc

thanks,
xhwang

[xhwang, 2011-12-01 23:45:23]

Hello piman,

ddorwin and I investigated the NPAPI case a little more and found that we cannot
check in the NPAPI plugin process.

There's a map (see g_channels in NPChannelBase::GetChannel) storing exist client
channels at the renderer side.  So if the channel already exists the renderer
will reuse it instead of creating a new one.  However, the renderer blindly asks
the browser to create a channel even if it already exists.  This appears to be
necessary so that the browser can check whether policy allows plugins for the
URL, etc.  However, this also introduces unnecessary IPC from the browser to
plugin since the browser does not know that the renderer already has a channel. 
In this case, the plugin returns a channel with fd being -1 since the client
pipe has already been taken.  The renderer ignores this invalid fd anyway since
an existing channel can be reused (by map looking up).

So I am going to remove the check in the plugin and unfortunately we will loose
the ability to monitor the fd in plugin process.  At the renderer side, set the
check right before the channel will be really created to avoid false alarm.

Please review again.

BTW, where is the URL policy check for Pepper plugins?

xhwang

[piman, 2011-12-01 23:53:04]

On 2011/12/01 23:45:23, xhwang wrote:
> Hello piman,
> 
> ddorwin and I investigated the NPAPI case a little more and found that we
cannot
> check in the NPAPI plugin process.
> 
> There's a map (see g_channels in NPChannelBase::GetChannel) storing exist
client
> channels at the renderer side.  So if the channel already exists the renderer
> will reuse it instead of creating a new one.  However, the renderer blindly
asks
> the browser to create a channel even if it already exists.  This appears to be
> necessary so that the browser can check whether policy allows plugins for the
> URL, etc.  However, this also introduces unnecessary IPC from the browser to
> plugin since the browser does not know that the renderer already has a
channel. 
> In this case, the plugin returns a channel with fd being -1 since the client
> pipe has already been taken.  The renderer ignores this invalid fd anyway
since
> an existing channel can be reused (by map looking up).
> 
> So I am going to remove the check in the plugin and unfortunately we will
loose
> the ability to monitor the fd in plugin process.  At the renderer side, set
the
> check right before the channel will be really created to avoid false alarm.
> 
> Please review again.

LGTM

> 
> BTW, where is the URL policy check for Pepper plugins?

I don't think there is such a thing.

> 
> xhwang

[jam, 2011-12-02 00:16:11]

lgtm

[commit-bot: I haz the power, 2011-12-02 04:51:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/8735015/12014

[commit-bot: I haz the power, 2011-12-02 07:27:05]

Change committed as 112647