Handles path absence instead of CHECKing.

Handles path absence instead of CHECKing.

In Chromium OS there is no 'extensions' directory, so this case should
be handled correctly.

BUG=chromium-os:11107, 70197
TEST=login to CrOS, no crashing

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

[Sam Kerner (Chrome), 2011-01-20 14:22:43]

LGTM.  Thanks for the quick fix.

On 2011/01/20 14:05:55, altimofeev wrote:

[Erik does not do reviews, 2011-01-20 14:58:50]

This doesn't look like the correct fix to me.  It should first be addressed
in app_paths.cc.  There, the question is whether it should point to another
location or explicitly fail (I don't see why we wouldn't want to support an
alternate location since there was a specific ask from ChromeOS for the
ability to support OEM bundling).  There's also at least one other
place that CHECKs the same value in this file.  Perhaps this will never be
called in this case, but then it seems like this particular CHECK isn't what
you want.

Erik


On Thu, Jan 20, 2011 at 6:05 AM, <altimofeev@chromium.org> wrote:

> Reviewers: Sam Kerner (Chrome),
>
> Description:
> Handles path absence instead of CHECKing.
>
> In Chromium OS there is no 'extensions' directory, so this case should
> be handled correctly.
>
> BUG=chromium-os:11107
> TEST=login to CrOS, no crashing
>
> Please review this at http://codereview.chromium.org/6265014/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  M chrome/browser/extensions/external_pref_extension_loader.cc
>
>
> Index: chrome/browser/extensions/external_pref_extension_loader.cc
> diff --git a/chrome/browser/extensions/external_pref_extension_loader.cc
> b/chrome/browser/extensions/external_pref_extension_loader.cc
> index
>
fc84b1ebf6df8b9023c67fba18696c0fd0069688..0d246b01c50735cfaca21b472616272377238262
> 100644
> --- a/chrome/browser/extensions/external_pref_extension_loader.cc
> +++ b/chrome/browser/extensions/external_pref_extension_loader.cc
> @@ -60,19 +60,22 @@ void ExternalPrefExtensionLoader::StartLoading() {
>  void ExternalPrefExtensionLoader::LoadOnFileThread() {
>   CHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
>
> -  CHECK(PathService::Get(base_path_key_, &base_path_));
> +  scoped_ptr<DictionaryValue> prefs;
>
> -  FilePath json_file;
> -  json_file =
> base_path_.Append(FILE_PATH_LITERAL("external_extensions.json"));
> +  if (PathService::Get(base_path_key_, &base_path_)) {
> +    FilePath json_file;
> +    json_file =
> +        base_path_.Append(FILE_PATH_LITERAL("external_extensions.json"));
>
> -  scoped_ptr<DictionaryValue> prefs;
> -  if (file_util::PathExists(json_file)) {
> -    JSONFileValueSerializer serializer(json_file);
> -    prefs.reset(ExtractPrefs(&serializer));
> -  } else {
> -    prefs.reset(new DictionaryValue());
> +    if (file_util::PathExists(json_file)) {
> +      JSONFileValueSerializer serializer(json_file);
> +      prefs.reset(ExtractPrefs(&serializer));
> +    }
>   }
>
> +  if (!prefs.get())
> +    prefs.reset(new DictionaryValue());
> +
>   prefs_.reset(prefs.release());
>   BrowserThread::PostTask(
>        BrowserThread::UI, FROM_HERE,
>
>
>

[altimofeev, 2011-01-20 15:28:27]

I consider this fix as "smooth" revert of the recently added CHECK, which leads
to the crashes. The issues you are talking about probably will be implemented.
But I think that issues tracker is the better place for the discussions about
features.

Also, I don't see the big difference between not finding the json file and not
finding directory which should contain this file.

-- Alexey

On 2011/01/20 14:58:50, Erik Kay wrote:
> This doesn't look like the correct fix to me.  It should first be addressed
> in app_paths.cc.  There, the question is whether it should point to another
> location or explicitly fail (I don't see why we wouldn't want to support an
> alternate location since there was a specific ask from ChromeOS for the
> ability to support OEM bundling).  There's also at least one other
> place that CHECKs the same value in this file.  Perhaps this will never be
> called in this case, but then it seems like this particular CHECK isn't what
> you want.
> 
> Erik
> 
> 
> On Thu, Jan 20, 2011 at 6:05 AM, <mailto:altimofeev@chromium.org> wrote:
> 
> > Reviewers: Sam Kerner (Chrome),
> >
> > Description:
> > Handles path absence instead of CHECKing.
> >
> > In Chromium OS there is no 'extensions' directory, so this case should
> > be handled correctly.
> >
> > BUG=chromium-os:11107
> > TEST=login to CrOS, no crashing
> >
> > Please review this at http://codereview.chromium.org/6265014/
> >
> > SVN Base: svn://svn.chromium.org/chrome/trunk/src
> >
> > Affected files:
> >  M chrome/browser/extensions/external_pref_extension_loader.cc
> >
> >
> > Index: chrome/browser/extensions/external_pref_extension_loader.cc
> > diff --git a/chrome/browser/extensions/external_pref_extension_loader.cc
> > b/chrome/browser/extensions/external_pref_extension_loader.cc
> > index
> >
>
fc84b1ebf6df8b9023c67fba18696c0fd0069688..0d246b01c50735cfaca21b472616272377238262
> > 100644
> > --- a/chrome/browser/extensions/external_pref_extension_loader.cc
> > +++ b/chrome/browser/extensions/external_pref_extension_loader.cc
> > @@ -60,19 +60,22 @@ void ExternalPrefExtensionLoader::StartLoading() {
> >  void ExternalPrefExtensionLoader::LoadOnFileThread() {
> >   CHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
> >
> > -  CHECK(PathService::Get(base_path_key_, &base_path_));
> > +  scoped_ptr<DictionaryValue> prefs;
> >
> > -  FilePath json_file;
> > -  json_file =
> > base_path_.Append(FILE_PATH_LITERAL("external_extensions.json"));
> > +  if (PathService::Get(base_path_key_, &base_path_)) {
> > +    FilePath json_file;
> > +    json_file =
> > +        base_path_.Append(FILE_PATH_LITERAL("external_extensions.json"));
> >
> > -  scoped_ptr<DictionaryValue> prefs;
> > -  if (file_util::PathExists(json_file)) {
> > -    JSONFileValueSerializer serializer(json_file);
> > -    prefs.reset(ExtractPrefs(&serializer));
> > -  } else {
> > -    prefs.reset(new DictionaryValue());
> > +    if (file_util::PathExists(json_file)) {
> > +      JSONFileValueSerializer serializer(json_file);
> > +      prefs.reset(ExtractPrefs(&serializer));
> > +    }
> >   }
> >
> > +  if (!prefs.get())
> > +    prefs.reset(new DictionaryValue());
> > +
> >   prefs_.reset(prefs.release());
> >   BrowserThread::PostTask(
> >        BrowserThread::UI, FROM_HERE,
> >
> >
> >

[Sam Kerner (Chrome), 2011-01-20 15:31:29]

On 2011/01/20 14:58:50, Erik Kay wrote:
> This doesn't look like the correct fix to me.  It should first be addressed
> in app_paths.cc.

In app_paths.cc, creating the directory fails because the path is on a read only
volume.

Linux users are reporting the same problem, because non-root users can't create
the directory.  crbug.com/70197

>  There, the question is whether it should point to another
> location or explicitly fail (I don't see why we wouldn't want to support an
> alternate location since there was a specific ask from ChromeOS for the
> ability to support OEM bundling).

Chrome OS does use the existing path to bundle some extensions, so we should not
change the path.  The change in which the CHECK was added makes it easy to
support multiple paths, and the next step is to add a path for OEM extension
lists.

>  There's also at least one other
> place that CHECKs the same value in this file.  Perhaps this will never be
> called in this case, but then it seems like this particular CHECK isn't what
> you want.

I don't see it.  Do you mean  CHECK(!base_path_.empty()); ?  That check is
intended to catch a request for a base path when LoadOnFileThread*() errored
out.  In that case, the code that should use the base path should never run.

Sam

> Erik
> 
> 
> On Thu, Jan 20, 2011 at 6:05 AM, <mailto:altimofeev@chromium.org> wrote:
> 
> > Reviewers: Sam Kerner (Chrome),
> >
> > Description:
> > Handles path absence instead of CHECKing.
> >
> > In Chromium OS there is no 'extensions' directory, so this case should
> > be handled correctly.
> >
> > BUG=chromium-os:11107
> > TEST=login to CrOS, no crashing
> >
> > Please review this at http://codereview.chromium.org/6265014/
> >
> > SVN Base: svn://svn.chromium.org/chrome/trunk/src
> >
> > Affected files:
> >  M chrome/browser/extensions/external_pref_extension_loader.cc
> >
> >
> > Index: chrome/browser/extensions/external_pref_extension_loader.cc
> > diff --git a/chrome/browser/extensions/external_pref_extension_loader.cc
> > b/chrome/browser/extensions/external_pref_extension_loader.cc
> > index
> >
>
fc84b1ebf6df8b9023c67fba18696c0fd0069688..0d246b01c50735cfaca21b472616272377238262
> > 100644
> > --- a/chrome/browser/extensions/external_pref_extension_loader.cc
> > +++ b/chrome/browser/extensions/external_pref_extension_loader.cc
> > @@ -60,19 +60,22 @@ void ExternalPrefExtensionLoader::StartLoading() {
> >  void ExternalPrefExtensionLoader::LoadOnFileThread() {
> >   CHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
> >
> > -  CHECK(PathService::Get(base_path_key_, &base_path_));
> > +  scoped_ptr<DictionaryValue> prefs;
> >
> > -  FilePath json_file;
> > -  json_file =
> > base_path_.Append(FILE_PATH_LITERAL("external_extensions.json"));
> > +  if (PathService::Get(base_path_key_, &base_path_)) {
> > +    FilePath json_file;
> > +    json_file =
> > +        base_path_.Append(FILE_PATH_LITERAL("external_extensions.json"));
> >
> > -  scoped_ptr<DictionaryValue> prefs;
> > -  if (file_util::PathExists(json_file)) {
> > -    JSONFileValueSerializer serializer(json_file);
> > -    prefs.reset(ExtractPrefs(&serializer));
> > -  } else {
> > -    prefs.reset(new DictionaryValue());
> > +    if (file_util::PathExists(json_file)) {
> > +      JSONFileValueSerializer serializer(json_file);
> > +      prefs.reset(ExtractPrefs(&serializer));
> > +    }
> >   }
> >
> > +  if (!prefs.get())
> > +    prefs.reset(new DictionaryValue());
> > +
> >   prefs_.reset(prefs.release());
> >   BrowserThread::PostTask(
> >        BrowserThread::UI, FROM_HERE,
> >
> >
> >

[Erik does not do reviews, 2011-01-20 19:11:12]

On Thu, Jan 20, 2011 at 7:28 AM, <altimofeev@chromium.org> wrote:

> I consider this fix as "smooth" revert of the recently added CHECK, which
> leads
> to the crashes. The issues you are talking about probably will be
> implemented.
> But I think that issues tracker is the better place for the discussions
> about
> features.
>

Thanks.  I didn't realize that this was a recently added CHECK.


> Also, I don't see the big difference between not finding the json file and
> not
> finding directory which should contain this file.
>

The difference is that others may try to use the same path key.  If it's not
intended to work in ChromeOS, that's where it should fail.


On Thu, Jan 20, 2011 at 7:31 AM, <skerner@chromium.org> wrote:

> On 2011/01/20 14:58:50, Erik Kay wrote:
>
>> This doesn't look like the correct fix to me.  It should first be
>> addressed
>> in app_paths.cc.
>>
>
> In app_paths.cc, creating the directory fails because the path is on a read
> only
> volume.
>
> Linux users are reporting the same problem, because non-root users can't
> create
> the directory.  crbug.com/70197


It seems odd that the code is trying to create the directory ever.
 Machine-level mac and windows installs would have this same problem.  The
directory should already exist as part of our install image.



>  There, the question is whether it should point to another
>> location or explicitly fail (I don't see why we wouldn't want to support
>> an
>> alternate location since there was a specific ask from ChromeOS for the
>> ability to support OEM bundling).
>>
>
> Chrome OS does use the existing path to bundle some extensions, so we
> should not
> change the path.  The change in which the CHECK was added makes it easy to
> support multiple paths, and the next step is to add a path for OEM
> extension
> lists.


I'm confused.  If it's using the path, then how can the path not exist in
some cases?



>  There's also at least one other
>> place that CHECKs the same value in this file.  Perhaps this will never be
>> called in this case, but then it seems like this particular CHECK isn't
>> what
>> you want.
>>
>
> I don't see it.  Do you mean  CHECK(!base_path_.empty()); ?  That check is
> intended to catch a request for a base path when LoadOnFileThread*()
> errored
> out.  In that case, the code that should use the base path should never
> run.
>

Yes.  That's the one.  My point is that it sounds like base_path_ isn't the
thing to check then.  For example, LoadOnFileThread might fail to read the
prefs file as well.

Erik

[Sam Kerner (Chrome), 2011-01-20 19:53:48]

On 2011/01/20 19:11:12, Erik Kay wrote:
> On Thu, Jan 20, 2011 at 7:28 AM, <mailto:altimofeev@chromium.org> wrote:
> 
> > I consider this fix as "smooth" revert of the recently added CHECK, which
> > leads
> > to the crashes. The issues you are talking about probably will be
> > implemented.
> > But I think that issues tracker is the better place for the discussions
> > about
> > features.
> >
> 
> Thanks.  I didn't realize that this was a recently added CHECK.
> 
> 
> > Also, I don't see the big difference between not finding the json file and
> > not
> > finding directory which should contain this file.
> >
> 
> The difference is that others may try to use the same path key.  If it's not
> intended to work in ChromeOS, that's where it should fail.
> 
> 
> On Thu, Jan 20, 2011 at 7:31 AM, <mailto:skerner@chromium.org> wrote:
> 
> > On 2011/01/20 14:58:50, Erik Kay wrote:
> >
> >> This doesn't look like the correct fix to me.  It should first be
> >> addressed
> >> in app_paths.cc.
> >>
> >
> > In app_paths.cc, creating the directory fails because the path is on a read
> > only
> > volume.
> >
> > Linux users are reporting the same problem, because non-root users can't
> > create
> > the directory.  crbug.com/70197
> 
> 
> It seems odd that the code is trying to create the directory ever.
>  Machine-level mac and windows installs would have this same problem.  The
> directory should already exist as part of our install image.
> 
> 
> 
> >  There, the question is whether it should point to another
> >> location or explicitly fail (I don't see why we wouldn't want to support
> >> an
> >> alternate location since there was a specific ask from ChromeOS for the
> >> ability to support OEM bundling).
> >>
> >
> > Chrome OS does use the existing path to bundle some extensions, so we
> > should not
> > change the path.  The change in which the CHECK was added makes it easy to
> > support multiple paths, and the next step is to add a path for OEM
> > extension
> > lists.
> 
> 
> I'm confused.  If it's using the path, then how can the path not exist in
> some cases?

  My understanding from looking at Dmitry's setup script is that release install
script puts external_extensions.json in the right directory.  A developer who
makes a build without running the full release script will not have it unless
they create it.


> >  There's also at least one other
> >> place that CHECKs the same value in this file.  Perhaps this will never be
> >> called in this case, but then it seems like this particular CHECK isn't
> >> what
> >> you want.
> >>
> >
> > I don't see it.  Do you mean  CHECK(!base_path_.empty()); ?  That check is
> > intended to catch a request for a base path when LoadOnFileThread*()
> > errored
> > out.  In that case, the code that should use the base path should never
> > run.
> >
> 
> Yes.  That's the one.  My point is that it sounds like base_path_ isn't the
> thing to check then.  For example, LoadOnFileThread might fail to read the
> prefs file as well.

base_path_ is initialized to empty when an ExternalPrefExtensionLoader is
constructed.  The user of an ExternalPrefExtensionLoader object calls
StartLoading(), which starts LoadOnFileThread().  If the json file is not
found/read, no external extension records are around to be processed.

GetBaseCrxFilePath() gives the path from which relative paths in
external_extensions.json should be resolved from.    If you didn't read the
.json file, you have no paths to .crx files, so there is no reason to call
GetBaseCrxFilePath().  Therefor if the path is still empty when
GetBaseCrxFilePath() is run, something is wrong.  I could make this more
explicit by adding a boolean did_successfully_read_json_file_, and CHECK()ing it
in GetBaseCrxFilePath().  Would that address your concern?

Sam

[Erik does not do reviews, 2011-01-20 20:38:37]

On Thu, Jan 20, 2011 at 11:53 AM, <skerner@chromium.org> wrote:

> On 2011/01/20 19:11:12, Erik Kay wrote:
>
>  On Thu, Jan 20, 2011 at 7:28 AM, <mailto:altimofeev@chromium.org> wrote:
>>
>
>  > I consider this fix as "smooth" revert of the recently added CHECK,
>> which
>> > leads
>> > to the crashes. The issues you are talking about probably will be
>> > implemented.
>> > But I think that issues tracker is the better place for the discussions
>> > about
>> > features.
>> >
>>
>
>  Thanks.  I didn't realize that this was a recently added CHECK.
>>
>
>
>  > Also, I don't see the big difference between not finding the json file
>> and
>> > not
>> > finding directory which should contain this file.
>> >
>>
>
>  The difference is that others may try to use the same path key.  If it's
>> not
>> intended to work in ChromeOS, that's where it should fail.
>>
>
>
>  On Thu, Jan 20, 2011 at 7:31 AM, <mailto:skerner@chromium.org> wrote:
>>
>
>  > On 2011/01/20 14:58:50, Erik Kay wrote:
>> >
>> >> This doesn't look like the correct fix to me.  It should first be
>> >> addressed
>> >> in app_paths.cc.
>> >>
>> >
>> > In app_paths.cc, creating the directory fails because the path is on a
>> read
>> > only
>> > volume.
>> >
>> > Linux users are reporting the same problem, because non-root users can't
>> > create
>> > the directory.  crbug.com/70197
>>
>
>
>  It seems odd that the code is trying to create the directory ever.
>>  Machine-level mac and windows installs would have this same problem.  The
>> directory should already exist as part of our install image.
>>
>
>
>
>  >  There, the question is whether it should point to another
>> >> location or explicitly fail (I don't see why we wouldn't want to
>> support
>> >> an
>> >> alternate location since there was a specific ask from ChromeOS for the
>> >> ability to support OEM bundling).
>> >>
>> >
>> > Chrome OS does use the existing path to bundle some extensions, so we
>> > should not
>> > change the path.  The change in which the CHECK was added makes it easy
>> to
>> > support multiple paths, and the next step is to add a path for OEM
>> > extension
>> > lists.
>>
>
>
>  I'm confused.  If it's using the path, then how can the path not exist in
>> some cases?
>>
>
>  My understanding from looking at Dmitry's setup script is that release
> install
> script puts external_extensions.json in the right directory.  A developer
> who
> makes a build without running the full release script will not have it
> unless
> they create it.


So maybe this is the core issue.  If it's a directory that's always going to
be present in the shipping build, then it should be present for developer
builds as well.



> >  There's also at least one other
>> >> place that CHECKs the same value in this file.  Perhaps this will never
>> be
>> >> called in this case, but then it seems like this particular CHECK isn't
>> >> what
>> >> you want.
>> >>
>> >
>> > I don't see it.  Do you mean  CHECK(!base_path_.empty()); ?  That check
>> is
>> > intended to catch a request for a base path when LoadOnFileThread*()
>> > errored
>> > out.  In that case, the code that should use the base path should never
>> > run.
>> >
>>
>
>  Yes.  That's the one.  My point is that it sounds like base_path_ isn't
>> the
>> thing to check then.  For example, LoadOnFileThread might fail to read the
>> prefs file as well.
>>
>
> base_path_ is initialized to empty when an ExternalPrefExtensionLoader is
> constructed.  The user of an ExternalPrefExtensionLoader object calls
> StartLoading(), which starts LoadOnFileThread().  If the json file is not
> found/read, no external extension records are around to be processed.
>
> GetBaseCrxFilePath() gives the path from which relative paths in
> external_extensions.json should be resolved from.    If you didn't read the
> .json file, you have no paths to .crx files, so there is no reason to call
> GetBaseCrxFilePath().  Therefor if the path is still empty when
> GetBaseCrxFilePath() is run, something is wrong.  I could make this more
> explicit by adding a boolean did_successfully_read_json_file_, and
> CHECK()ing it
> in GetBaseCrxFilePath().  Would that address your concern?
>

If the point is that it shouldn't ever be called unless there was something
in the json file, then the right assertion would be:

CHECK(!prefs_.empty());

Also, the comment isn't accurate since it says that base_path_ is always
valid after LoadOnFileThread.





>
> Sam
>
>
>
> http://codereview.chromium.org/6265014/
>

[Sam Kerner (Chrome), 2011-01-20 21:50:19]

On 2011/01/20 20:38:37, Erik Kay wrote:
> On Thu, Jan 20, 2011 at 11:53 AM, <mailto:skerner@chromium.org> wrote:
> 
> > On 2011/01/20 19:11:12, Erik Kay wrote:
> >
> >  On Thu, Jan 20, 2011 at 7:28 AM, <mailto:altimofeev@chromium.org> wrote:
> >>
> >
> >  > I consider this fix as "smooth" revert of the recently added CHECK,
> >> which
> >> > leads
> >> > to the crashes. The issues you are talking about probably will be
> >> > implemented.
> >> > But I think that issues tracker is the better place for the discussions
> >> > about
> >> > features.
> >> >
> >>
> >
> >  Thanks.  I didn't realize that this was a recently added CHECK.
> >>
> >
> >
> >  > Also, I don't see the big difference between not finding the json file
> >> and
> >> > not
> >> > finding directory which should contain this file.
> >> >
> >>
> >
> >  The difference is that others may try to use the same path key.  If it's
> >> not
> >> intended to work in ChromeOS, that's where it should fail.
> >>
> >
> >
> >  On Thu, Jan 20, 2011 at 7:31 AM, <mailto:skerner@chromium.org> wrote:
> >>
> >
> >  > On 2011/01/20 14:58:50, Erik Kay wrote:
> >> >
> >> >> This doesn't look like the correct fix to me.  It should first be
> >> >> addressed
> >> >> in app_paths.cc.
> >> >>
> >> >
> >> > In app_paths.cc, creating the directory fails because the path is on a
> >> read
> >> > only
> >> > volume.
> >> >
> >> > Linux users are reporting the same problem, because non-root users can't
> >> > create
> >> > the directory.  crbug.com/70197
> >>
> >
> >
> >  It seems odd that the code is trying to create the directory ever.
> >>  Machine-level mac and windows installs would have this same problem.  The
> >> directory should already exist as part of our install image.
> >>
> >
> >
> >
> >  >  There, the question is whether it should point to another
> >> >> location or explicitly fail (I don't see why we wouldn't want to
> >> support
> >> >> an
> >> >> alternate location since there was a specific ask from ChromeOS for the
> >> >> ability to support OEM bundling).
> >> >>
> >> >
> >> > Chrome OS does use the existing path to bundle some extensions, so we
> >> > should not
> >> > change the path.  The change in which the CHECK was added makes it easy
> >> to
> >> > support multiple paths, and the next step is to add a path for OEM
> >> > extension
> >> > lists.
> >>
> >
> >
> >  I'm confused.  If it's using the path, then how can the path not exist in
> >> some cases?
> >>
> >
> >  My understanding from looking at Dmitry's setup script is that release
> > install
> > script puts external_extensions.json in the right directory.  A developer
> > who
> > makes a build without running the full release script will not have it
> > unless
> > they create it.
> 
> 
> So maybe this is the core issue.  If it's a directory that's always going to
> be present in the shipping build, then it should be present for developer
> builds as well.

Alexey, do you know what script we need to update on Chrome OS?

Based on the bug report from linux users, it is not present on existing linux
installs.  The package on my linux machine doesn't have it either.  We can't fix
this by creating it if it is missing without having superuser privileges.  So, I
don't see how we can avoid tolerating the lack of this directory on linux.


> > >  There's also at least one other
> >> >> place that CHECKs the same value in this file.  Perhaps this will never
> >> be
> >> >> called in this case, but then it seems like this particular CHECK isn't
> >> >> what
> >> >> you want.
> >> >>
> >> >
> >> > I don't see it.  Do you mean  CHECK(!base_path_.empty()); ?  That check
> >> is
> >> > intended to catch a request for a base path when LoadOnFileThread*()
> >> > errored
> >> > out.  In that case, the code that should use the base path should never
> >> > run.
> >> >
> >>
> >
> >  Yes.  That's the one.  My point is that it sounds like base_path_ isn't
> >> the
> >> thing to check then.  For example, LoadOnFileThread might fail to read the
> >> prefs file as well.
> >>
> >
> > base_path_ is initialized to empty when an ExternalPrefExtensionLoader is
> > constructed.  The user of an ExternalPrefExtensionLoader object calls
> > StartLoading(), which starts LoadOnFileThread().  If the json file is not
> > found/read, no external extension records are around to be processed.
> >
> > GetBaseCrxFilePath() gives the path from which relative paths in
> > external_extensions.json should be resolved from.    If you didn't read the
> > .json file, you have no paths to .crx files, so there is no reason to call
> > GetBaseCrxFilePath().  Therefor if the path is still empty when
> > GetBaseCrxFilePath() is run, something is wrong.  I could make this more
> > explicit by adding a boolean did_successfully_read_json_file_, and
> > CHECK()ing it
> > in GetBaseCrxFilePath().  Would that address your concern?
> >
> 
> If the point is that it shouldn't ever be called unless there was something
> in the json file, then the right assertion would be:
> 
> CHECK(!prefs_.empty());

Okay, that is clearer.

> 
> Also, the comment isn't accurate since it says that base_path_ is always
> valid after LoadOnFileThread.

I am about to send you another CL in this area of the code.  Will address this
in that CL.

Sam

[altimofeev, 2011-01-21 13:58:59]

On 2011/01/20 21:50:19, Sam Kerner (Chrome) wrote:

> ...
> Alexey, do you know what script we need to update on Chrome OS?

Not exactly. Furthermore, I do think that having empty read-only folder makes
not so much sense.