Add Null check for kExtensionsPref dictionary...

Add Null check for kExtensionsPref dictionary

Do nothing if the preferences dic do not exist.

CID 14614

BUG=none
TEST=none

[cpu_(ooo_6.6-7.5), 2011-01-28 03:33:09]

coverity meat-bot

[Mattias Nissler (ping if slow), 2011-01-28 09:27:03]

http://codereview.chromium.org/6271025/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/6271025/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs.cc:1148: return;
Hm. I'd say that the problem is really with the PrefService code which shouldn't
return NULL in any case here. Could we resolve the report also by replacing this
code in PrefService::GetDictionary():

  const Value* value = pref->GetValue();
  if (value->GetType() == Value::TYPE_NULL)
    return NULL;
  return static_cast<const DictionaryValue*>(value);

by 

  const Value* value = pref->GetValue();
  CHECK(value->IsType(Value::TYPE_DICTIONARY))
  return static_cast<const DictionaryValue*>(value);

[battre, 2011-01-28 09:35:10]

The same holds of course for PrefService::GetList

[cpu_(ooo_6.6-7.5), 2011-01-29 02:31:50]

Ok, no problem, I withdraw this fix and let you fix it the right way. Please do
not forget or else covertity meat-bot will strike again :)


On 2011/01/28 09:35:10, battre wrote:
> The same holds of course for PrefService::GetList

[cpu_(ooo_6.6-7.5), 2011-01-29 02:33:02]

Just to be clear I am not a fan of CHECK() in these cases.

On 2011/01/29 02:31:50, cpu wrote:
> Ok, no problem, I withdraw this fix and let you fix it the right way. Please
do
> not forget or else covertity meat-bot will strike again :)
> 
> 
> On 2011/01/28 09:35:10, battre wrote:
> > The same holds of course for PrefService::GetList

[Mattias Nissler (ping if slow), 2011-01-29 13:30:21]

I'm not big fan of CHECK either. However, it's an invariant of the
PrefService that all prefs that were registered have at least a default
value, so I think the CHECK() is fair in this case.

Btw. how can I learn more about that coverity meat-bot? This is the first
time I hear about it.

On Sat, Jan 29, 2011 at 3:33 AM, <cpu@chromium.org> wrote:

> Just to be clear I am not a fan of CHECK() in these cases.
>
>
> On 2011/01/29 02:31:50, cpu wrote:
>
>> Ok, no problem, I withdraw this fix and let you fix it the right way.
>> Please
>>
> do
>
>> not forget or else covertity meat-bot will strike again :)
>>
>
>
>  On 2011/01/28 09:35:10, battre wrote:
>> > The same holds of course for PrefService::GetList
>>
>
>
>
> http://codereview.chromium.org/6271025/
>

[Aaron Boodman, 2011-01-29 19:24:03]

http://codereview.chromium.org/6271025/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/6271025/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs.cc:1148: return;
On 2011/01/28 09:27:03, Mattias Nissler wrote:
> Hm. I'd say that the problem is really with the PrefService code which
shouldn't
> return NULL in any case here. Could we resolve the report also by replacing
this
> code in PrefService::GetDictionary():
> 
>   const Value* value = pref->GetValue();
>   if (value->GetType() == Value::TYPE_NULL)
>     return NULL;
>   return static_cast<const DictionaryValue*>(value);
> 
> by 
> 
>   const Value* value = pref->GetValue();
>   CHECK(value->IsType(Value::TYPE_DICTIONARY))
>   return static_cast<const DictionaryValue*>(value);

cpu, did you find a case of this actually returning NULL, or did coverity flag
this?

Mattias, what happens if the Preferences file gets changed by hand and the
extensions key gets replaced with some other type? Will PrefService return a
default DictionaryValue or will it return NULL?

[Mattias Nissler (ping if slow), 2011-01-29 19:34:32]

Relevant to the pref system is the type when the preference gets registered.
That means changing the type in the file will result in the pref system
ignoring that pref and returning the default value, which is an empty
dictionary in this case.

On Sat, Jan 29, 2011 at 8:24 PM, <aa@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6271025/diff/1/chrome/browser/extensions/exten...
> File chrome/browser/extensions/extension_prefs.cc (right):
>
>
>
http://codereview.chromium.org/6271025/diff/1/chrome/browser/extensions/exten...
> chrome/browser/extensions/extension_prefs.cc:1148: return;
> On 2011/01/28 09:27:03, Mattias Nissler wrote:
>
>> Hm. I'd say that the problem is really with the PrefService code which
>>
> shouldn't
>
>> return NULL in any case here. Could we resolve the report also by
>>
> replacing this
>
>> code in PrefService::GetDictionary():
>>
>
>    const Value* value = pref->GetValue();
>>   if (value->GetType() == Value::TYPE_NULL)
>>     return NULL;
>>   return static_cast<const DictionaryValue*>(value);
>>
>
>  by
>>
>
>    const Value* value = pref->GetValue();
>>   CHECK(value->IsType(Value::TYPE_DICTIONARY))
>>   return static_cast<const DictionaryValue*>(value);
>>
>
> cpu, did you find a case of this actually returning NULL, or did
> coverity flag this?
>
> Mattias, what happens if the Preferences file gets changed by hand and
> the extensions key gets replaced with some other type? Will PrefService
> return a default DictionaryValue or will it return NULL?
>
>
> http://codereview.chromium.org/6271025/
>

[cpu_(ooo_6.6-7.5), 2011-01-31 21:34:09]

On 2011/01/29 19:24:03, Aaron Boodman wrote:
>
http://codereview.chromium.org/6271025/diff/1/chrome/browser/extensions/exten...
> File chrome/browser/extensions/extension_prefs.cc (right):
> 
>
http://codereview.chromium.org/6271025/diff/1/chrome/browser/extensions/exten...
> chrome/browser/extensions/extension_prefs.cc:1148: return;
> On 2011/01/28 09:27:03, Mattias Nissler wrote:
> > Hm. I'd say that the problem is really with the PrefService code which
> shouldn't
> > return NULL in any case here. Could we resolve the report also by replacing
> this
> > code in PrefService::GetDictionary():
> > 
> >   const Value* value = pref->GetValue();
> >   if (value->GetType() == Value::TYPE_NULL)
> >     return NULL;
> >   return static_cast<const DictionaryValue*>(value);
> > 
> > by 
> > 
> >   const Value* value = pref->GetValue();
> >   CHECK(value->IsType(Value::TYPE_DICTIONARY))
> >   return static_cast<const DictionaryValue*>(value);
> 
> cpu, did you find a case of this actually returning NULL, or did coverity flag

coverity found it, because in other cases we do check null.


> this?
> 
> Mattias, what happens if the Preferences file gets changed by hand and the
> extensions key gets replaced with some other type? Will PrefService return a
> default DictionaryValue or will it return NULL?