Add more CHECK in GPU and NPAPI process on invalid file descriptors

Add more CHECK in GPU and NPAPI process on invalid file descriptors

Goes with r112647, where we added CHECKs for invalid file descriptors.

Regarding the GPU process, we already have a crash in GpuChannelManager::OnEstablishChannel() function due to invalid renderer fd.  But it's not clear whether this is due to channel reuse or failure in creating channel in server mode.  Talked to apatrick@ and the reuse one should never happen in normal case.  Adding check on that case so we can hopefully rule out that case.

Regarding the NPAPI process, we already have a lot of crashes due to invalid file descriptors in the rederer.  In NPAPI process, add a CHECK right after the channel is created in server mode to see where this invalid fd originates from.

BUG=none
TEST=passed unit tests


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

[xhwang, 2011-12-08 05:14:32]

Hello jam and apatrick:

Please do ONWERS review.  See description for details.

jam: content/common/np_channel_base.cc
apatrick: content/common/gpu/*.cc

ddorwin, piman and brettw:

Please also let me know if you are comfortable with this change as we are close
to Beta.

xhwang

[piman, 2011-12-08 07:09:11]

You may want to talk to kerz@ in terms of whether or not to get this over to the
branch - that's where dev channels are going to get cut for a little while.

http://codereview.chromium.org/8877001/diff/2001/content/common/gpu/gpu_chann...
File content/common/gpu/gpu_channel.cc (right):

http://codereview.chromium.org/8877001/diff/2001/content/common/gpu/gpu_chann...
content/common/gpu/gpu_channel.cc:421: NOTREACHED();
I think you meant something like LOG(FATAL) or CHECK(false).
NOTREACHED() is DCHECK(false) -- only in debug.

http://codereview.chromium.org/8877001/diff/2001/content/common/gpu/gpu_chann...
File content/common/gpu/gpu_channel_manager.cc (right):

http://codereview.chromium.org/8877001/diff/2001/content/common/gpu/gpu_chann...
content/common/gpu/gpu_channel_manager.cc:82: else {
nit: braces on both sides of the else.

[ddorwin, 2011-12-08 19:17:27]

http://codereview.chromium.org/8877001/diff/8001/content/common/gpu/gpu_chann...
File content/common/gpu/gpu_channel.cc (right):

http://codereview.chromium.org/8877001/diff/8001/content/common/gpu/gpu_chann...
content/common/gpu/gpu_channel.cc:421: CHECK(false);
Replace the if statement with a CHECK? The return will never execute anyway.

http://codereview.chromium.org/8877001/diff/8001/content/common/gpu/gpu_chann...
File content/common/gpu/gpu_channel_manager.cc (right):

http://codereview.chromium.org/8877001/diff/8001/content/common/gpu/gpu_chann...
content/common/gpu/gpu_channel_manager.cc:83: CHECK(false);
If this is temporary, there should be a TODO to clean this up. Otherwise, we
should just CHECK line 80 and remove new lines 82-85.

http://codereview.chromium.org/8877001/diff/8001/content/common/np_channel_ba...
File content/common/np_channel_base.cc (right):

http://codereview.chromium.org/8877001/diff/8001/content/common/np_channel_ba...
content/common/np_channel_base.cc:138: if (mode_ == IPC::Channel::MODE_SERVER)
Do we have a separate check that covers the client case or otherwise know that
it's no the client case that's causing a problem?

[xhwang, 2011-12-08 19:37:49]

Uploaded new patchs according to piman and ddorwin's comments.

http://codereview.chromium.org/8877001/diff/2001/content/common/gpu/gpu_chann...
File content/common/gpu/gpu_channel.cc (right):

http://codereview.chromium.org/8877001/diff/2001/content/common/gpu/gpu_chann...
content/common/gpu/gpu_channel.cc:421: NOTREACHED();
On 2011/12/08 07:09:11, piman wrote:
> I think you meant something like LOG(FATAL) or CHECK(false).
> NOTREACHED() is DCHECK(false) -- only in debug.

Done.

http://codereview.chromium.org/8877001/diff/2001/content/common/gpu/gpu_chann...
File content/common/gpu/gpu_channel_manager.cc (right):

http://codereview.chromium.org/8877001/diff/2001/content/common/gpu/gpu_chann...
content/common/gpu/gpu_channel_manager.cc:82: else {
On 2011/12/08 07:09:11, piman wrote:
> nit: braces on both sides of the else.

Done.

http://codereview.chromium.org/8877001/diff/8001/content/common/gpu/gpu_chann...
File content/common/gpu/gpu_channel.cc (right):

http://codereview.chromium.org/8877001/diff/8001/content/common/gpu/gpu_chann...
content/common/gpu/gpu_channel.cc:421: CHECK(false);
On 2011/12/08 19:17:27, ddorwin wrote:
> Replace the if statement with a CHECK? The return will never execute anyway.

This will be a temporary change.

http://codereview.chromium.org/8877001/diff/8001/content/common/gpu/gpu_chann...
File content/common/gpu/gpu_channel_manager.cc (right):

http://codereview.chromium.org/8877001/diff/8001/content/common/gpu/gpu_chann...
content/common/gpu/gpu_channel_manager.cc:83: CHECK(false);
On 2011/12/08 19:17:27, ddorwin wrote:
> If this is temporary, there should be a TODO to clean this up. Otherwise, we
> should just CHECK line 80 and remove new lines 82-85.

This should be temporary as I am not confident that this CHECK is totally safe
in reality (as per piman's comment in
http://code.google.com/p/chromium/issues/detail?id=95732#c41).  It may be
improved later but is out of the scope of this investigation.

http://codereview.chromium.org/8877001/diff/8001/content/common/np_channel_ba...
File content/common/np_channel_base.cc (right):

http://codereview.chromium.org/8877001/diff/8001/content/common/np_channel_ba...
content/common/np_channel_base.cc:138: if (mode_ == IPC::Channel::MODE_SERVER)
On 2011/12/08 19:17:27, ddorwin wrote:
> Do we have a separate check that covers the client case or otherwise know that
> it's no the client case that's causing a problem?

This same function is shared by both server and client sides.  Line 124-129
covers the check in the client case.

[apatrick_chromium, 2011-12-08 19:57:02]

lgtm

[brettw, 2011-12-08 20:39:55]

lgtm

[ddorwin, 2011-12-08 21:56:41]

lgtm

[commit-bot: I haz the power, 2011-12-09 03:59:32]

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

[commit-bot: I haz the power, 2011-12-09 05:18:30]

Change committed as 113767