Fixed check return warnings from Coverity.

Fixed the following warnings:

CID=104268,104216,104131,104058


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

[Danh Nguyen, 2012-07-25 23:47:27]

Hi Lei, Mattias,

I need your help to review these changes from today's chrome-lax Coverity
fixlet.  Since they're small changes, I hope this CL won't take much of your
time.

Mattias,

Please give me an owner review for the changes under chrome/browser/policy.

Thanks a lot for your help.

Danh

[Lei Zhang, 2012-07-25 23:50:29]

LGTM with mnissler's approval.

Also, please change the CL description to something other than "coverity
fixlet".

[Mattias Nissler (ping if slow), 2012-07-26 09:37:06]

I'm pretty sure the DCHECKS are wrong here, but adding people who wrote this
code to take a look as well :)

http://codereview.chromium.org/10830022/diff/1/chrome/browser/policy/managed_...
File chrome/browser/policy/managed_mode_policy_provider.cc (right):

http://codereview.chromium.org/10830022/diff/1/chrome/browser/policy/managed_...
chrome/browser/policy/managed_mode_policy_provider.cc:47: DCHECK(rv);
I think this is wrong, the intention here is to return NULL if the key doesn't
exist (i.e. !rv), which now DCHECKS.

http://codereview.chromium.org/10830022/diff/1/chrome/browser/policy/policy_l...
File chrome/browser/policy/policy_loader_win.cc (right):

http://codereview.chromium.org/10830022/diff/1/chrome/browser/policy/policy_l...
chrome/browser/policy/policy_loader_win.cc:92: DCHECK_EQ(rc, ERROR_SUCCESS);
I think this is also wrong. Keys may not exist, so it's perfectly fine for this
to fail (and we shouldn't DCHECK). That's why we have the key->Valid() check
below.

http://codereview.chromium.org/10830022/diff/1/chrome/browser/value_store/tes...
File chrome/browser/value_store/testing_value_store.cc (right):

http://codereview.chromium.org/10830022/diff/1/chrome/browser/value_store/tes...
chrome/browser/value_store/testing_value_store.cc:102: DCHECK(rv);
Same thing, this may return false, in which case old_value is NULL, which is
handled properly in line 103.

[Joao da Silva, 2012-07-26 10:04:33]

As Mattias mentioned, DCHECKing is not correct in these cases, as failure is an
expected result some of the times.

I guess coverity complains about ignored return values, but sometimes they're
alright to ignore; we have a WARN_UNUSED_RESULT macro in
base/compiler_specific.h for the cases where ignoring the result is usually a
bug. Maybe coverity should ignore functions without that attribute when their
return value is ignored?

http://codereview.chromium.org/10830022/diff/1/chrome/browser/policy/policy_l...
File chrome/browser/policy/policy_loader_win.cc (right):

http://codereview.chromium.org/10830022/diff/1/chrome/browser/policy/policy_l...
chrome/browser/policy/policy_loader_win.cc:92: DCHECK_EQ(rc, ERROR_SUCCESS);
On 2012/07/26 09:37:06, Mattias Nissler wrote:
> I think this is also wrong. Keys may not exist, so it's perfectly fine for
this
> to fail (and we shouldn't DCHECK). That's why we have the key->Valid() check
> below.

Yep. Rewrite as:

if (key->Open(...) != ERROR_SUCCESS || !key->Valid())
  continue;

http://codereview.chromium.org/10830022/diff/1/chrome/browser/value_store/tes...
File chrome/browser/value_store/testing_value_store.cc (right):

http://codereview.chromium.org/10830022/diff/1/chrome/browser/value_store/tes...
chrome/browser/value_store/testing_value_store.cc:103: if (!old_value ||
!old_value->Equals(&it.value())) {
As Mattias mentioned, the DCHECK is wrong. Maybe rewrite this as:

if (!storage_.GetWithoutPathExpansion(it.key(), &old_value) ||
old_value->Equals(...)) { ... }

[Danh Nguyen, 2012-07-26 15:46:27]

Thank you Lei, Mattias and Joao for your reviews and your advices.

I should have done better investigation.  I'm pretty new to Chrome and this is a
good learning experience.

Could you take a look at the changes again?

Thanks,
Danh

http://codereview.chromium.org/10830022/diff/1/chrome/browser/policy/managed_...
File chrome/browser/policy/managed_mode_policy_provider.cc (right):

http://codereview.chromium.org/10830022/diff/1/chrome/browser/policy/managed_...
chrome/browser/policy/managed_mode_policy_provider.cc:47: DCHECK(rv);
On 2012/07/26 09:37:06, Mattias Nissler wrote:
> I think this is wrong, the intention here is to return NULL if the key doesn't
> exist (i.e. !rv), which now DCHECKS.

Reverted. I'll remove the DCHECK and add a note here.

http://codereview.chromium.org/10830022/diff/1/chrome/browser/policy/policy_l...
File chrome/browser/policy/policy_loader_win.cc (right):

http://codereview.chromium.org/10830022/diff/1/chrome/browser/policy/policy_l...
chrome/browser/policy/policy_loader_win.cc:92: DCHECK_EQ(rc, ERROR_SUCCESS);
On 2012/07/26 10:04:33, Joao da Silva wrote:
> On 2012/07/26 09:37:06, Mattias Nissler wrote:
> > I think this is also wrong. Keys may not exist, so it's perfectly fine for
> this
> > to fail (and we shouldn't DCHECK). That's why we have the key->Valid() check
> > below.
> 
> Yep. Rewrite as:
> 
> if (key->Open(...) != ERROR_SUCCESS || !key->Valid())
>   continue;

Fixed per Joao's advice.  Thanks.

http://codereview.chromium.org/10830022/diff/1/chrome/browser/value_store/tes...
File chrome/browser/value_store/testing_value_store.cc (right):

http://codereview.chromium.org/10830022/diff/1/chrome/browser/value_store/tes...
chrome/browser/value_store/testing_value_store.cc:103: if (!old_value ||
!old_value->Equals(&it.value())) {
On 2012/07/26 10:04:33, Joao da Silva wrote:
> As Mattias mentioned, the DCHECK is wrong. Maybe rewrite this as:
> 
> if (!storage_.GetWithoutPathExpansion(it.key(), &old_value) ||
> old_value->Equals(...)) { ... }

Fixed.  Thanks Joao for your suggestion.

[Joao da Silva, 2012-07-26 16:19:02]

policy/ lgtm with nit

http://codereview.chromium.org/10830022/diff/6002/chrome/browser/policy/polic...
File chrome/browser/policy/policy_loader_win.cc (right):

http://codereview.chromium.org/10830022/diff/6002/chrome/browser/policy/polic...
chrome/browser/policy/policy_loader_win.cc:92: !key->Valid())
Nit: add braces when the test or the body spans more than 1 one

[Danh Nguyen, 2012-07-26 17:08:04]

Thanks Joao for your quick response.  I appreciate that very much.

Nit fixed.

Thanks,
Danh

http://codereview.chromium.org/10830022/diff/6002/chrome/browser/policy/polic...
File chrome/browser/policy/policy_loader_win.cc (right):

http://codereview.chromium.org/10830022/diff/6002/chrome/browser/policy/polic...
chrome/browser/policy/policy_loader_win.cc:92: !key->Valid())
On 2012/07/26 16:19:02, Joao da Silva wrote:
> Nit: add braces when the test or the body spans more than 1 one

Done.

[commit-bot: I haz the power, 2012-07-26 17:26:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danhn@chromium.org/10830022/5002

[commit-bot: I haz the power, 2012-07-26 21:30:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danhn@chromium.org/10830022/5002

[commit-bot: I haz the power, 2012-07-26 22:40:25]

Change committed as 148638