Parse /etc/lsb-release only once on ChromeOS

Parse /etc/lsb-release only once on ChromeOS
This moves all parsing and caching of the contents of /etc/lsb-release
to base::SysInfo. This eliminates redundant parsing from:
* LsbReleaseLogSource
* VersionLoader
* StatisticsProvider

BUG=266903
For chrome_main_deleagate.cc (code removal only)

R=brettw@chromium.org, derat@chromium.org, pneubeck@chromium.org, xiyuan@chromium.org
TBR=cpu@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=225581

[stevenjb, 2013-09-23 23:38:04]

derat@ - overall review
brettw@ - base/
pneubeck@ - c/b/policy/
xiyuan@ - c/b/u/webui

https://codereview.chromium.org/23588009/diff/5001/chrome/browser/chromeos/ch...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/23588009/diff/5001/chrome/browser/chromeos/ch...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:396:
chrome::VersionInfo::SetChannel(channel);
This was moved from StatisticsProviderImpl::LoadMachineOSInfoFile(). It is now
safe to call this at any point. I put it here so that it gets called at roughly
the same point during startup that it was getting called before.

https://codereview.chromium.org/23588009/diff/5001/chrome/browser/chromeos/sy...
File chrome/browser/chromeos/system/statistics_provider.cc (left):

https://codereview.chromium.org/23588009/diff/5001/chrome/browser/chromeos/sy...
chrome/browser/chromeos/system/statistics_provider.cc:24: // TODO(phajdan.jr):
Drop that dependency, http://crbug.com/180711 .
I'll be moving this to src/chromeos/system in a follow-up CL.

[xiyuan, 2013-09-23 23:59:23]

c/b/u/webui LGTM

[pneubeck (no reviews), 2013-09-24 07:44:51]

nice cleanup.
chrome/browser/policy/cloud/device_management_service.cc lgtm.

https://codereview.chromium.org/23588009/diff/12001/base/sys_info.h
File base/sys_info.h (right):

https://codereview.chromium.org/23588009/diff/12001/base/sys_info.h#newcode85
base/sys_info.h:85: // Returns a the contents of /etc/lsb-release as a map.
a the -> the

https://codereview.chromium.org/23588009/diff/12001/base/sys_info_chromeos.cc
File base/sys_info_chromeos.cc (right):

https://codereview.chromium.org/23588009/diff/12001/base/sys_info_chromeos.cc...
base/sys_info_chromeos.cc:127: SysInfo::LsbReleaseMap lsb_release_map_;
FWIW, before this change only a fixed amount of memory was consumed (3 x int32
and 1 x bool). It's not clear how much memory lsb_release_map_ might consume.
For all other platforms SysInfo doesn't cache any data.

My question is mostly: do we want to optimize here for performance (parse once)
or for memory consumption (don't cache)? The former makes IMO only sense if
access to SysInfo is time critical or occurs frequently and parsing takes too
much time. However it seems that caching is required because of the sandbox. Is
that right?

To mitigate the issue, either document how many entries /etc/lsb-release has or
maybe whitelist the keys. As I don't expect this to be a huge problem, feel free
to ignore this comment.

https://codereview.chromium.org/23588009/diff/12001/chrome/app/chrome_main_de...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/23588009/diff/12001/chrome/app/chrome_main_de...
chrome/app/chrome_main_delegate.cc:752:
base::SysInfo::OperatingSystemVersionNumbers(
This call assumes more about the SysInfo implementation than the SysInfo
interface declares.
Instead there should be a call base::SysInfo::CacheVersionNumbers() (or more
scalable CacheInfo() ).

[Daniel Erat, 2013-09-24 15:37:52]

Thanks for cleaning this up!

https://codereview.chromium.org/23588009/diff/12001/base/sys_info.h
File base/sys_info.h (right):

https://codereview.chromium.org/23588009/diff/12001/base/sys_info.h#newcode96
base/sys_info.h:96: // time of the ChromeOS build).
nit: s/ChromeOS/Chrome OS/, our product's name is two words, pass it on

:-P

https://codereview.chromium.org/23588009/diff/12001/base/sys_info.h#newcode101
base/sys_info.h:101: double lsb_release_time);
it'd be best to make this take a Time instead of a double, but if that's
unfeasible for some reason, document what the double is (seconds since epoch?)

https://codereview.chromium.org/23588009/diff/12001/base/sys_info_chromeos.cc
File base/sys_info_chromeos.cc (right):

https://codereview.chromium.org/23588009/diff/12001/base/sys_info_chromeos.cc...
base/sys_info_chromeos.cc:49: env->GetVar("LSB_RELEASE_TIME",
&lsb_release_time_str);
nit: move both of these to constants so you can share them with
SetChromeOSVersionInfoForTest()

https://codereview.chromium.org/23588009/diff/12001/base/sys_info_chromeos.cc...
base/sys_info_chromeos.cc:52: StringToDouble(lsb_release_time_str, &us);
check the return value and maybe log an error

https://codereview.chromium.org/23588009/diff/12001/base/sys_info_chromeos.cc...
base/sys_info_chromeos.cc:57: // only happen in non-chromeos environments.
nit: s/chromeos/Chrome OS/

also, worthwhile calling IsRunningOnChromeOS() here and logging an error if the
environment variables aren't set?

https://codereview.chromium.org/23588009/diff/12001/base/sys_info_chromeos.cc...
base/sys_info_chromeos.cc:67: lsb_release_map_["lsb-source"] = parsed_from_env ?
"env" : "file";
nit: move these strings to constants (assuming that you'll eventually want to
access them from somewhere)

https://codereview.chromium.org/23588009/diff/12001/base/sys_info_chromeos.cc...
base/sys_info_chromeos.cc:96: for (size_t i = 0; i  < pairs.size(); ++i) {
nit: remove extra space before '<'

https://codereview.chromium.org/23588009/diff/12001/base/sys_info_chromeos.cc...
base/sys_info_chromeos.cc:127: SysInfo::LsbReleaseMap lsb_release_map_;
On 2013/09/24 07:44:51, pneubeck wrote:
> FWIW, before this change only a fixed amount of memory was consumed (3 x int32
> and 1 x bool). It's not clear how much memory lsb_release_map_ might consume.
> For all other platforms SysInfo doesn't cache any data.
> 
> My question is mostly: do we want to optimize here for performance (parse
once)
> or for memory consumption (don't cache)? The former makes IMO only sense if
> access to SysInfo is time critical or occurs frequently and parsing takes too
> much time. However it seems that caching is required because of the sandbox.
Is
> that right?
> 
> To mitigate the issue, either document how many entries /etc/lsb-release has
or
> maybe whitelist the keys. As I don't expect this to be a huge problem, feel
free
> to ignore this comment.

fwiw, i don't think there's any harm in keeping all of lsb-release in memory.

https://codereview.chromium.org/23588009/diff/12001/chrome/app/chrome_main_de...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/23588009/diff/12001/chrome/app/chrome_main_de...
chrome/app/chrome_main_delegate.cc:759: base::SysInfo::GetLsbReleaseTime());
nit: indenting one space seems strange -- indent four spaces beyond the previous
line's "base", perhaps?

https://codereview.chromium.org/23588009/diff/12001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/external_metrics.cc (right):

https://codereview.chromium.org/23588009/diff/12001/chrome/browser/chromeos/e...
chrome/browser/chromeos/external_metrics.cc:103: // mess.  At least this code is
temporary.
nit: delete this outdated comment

https://codereview.chromium.org/23588009/diff/12001/chrome/browser/chromeos/e...
chrome/browser/chromeos/external_metrics.cc:106: if
(base::SysInfo::GetLsbReleaseBoard() != "parrot")
this code makes me sad.

(probably not your fault.)

that is all.

[stevenjb, 2013-09-24 17:18:33]

https://codereview.chromium.org/23588009/diff/12001/base/sys_info.h
File base/sys_info.h (right):

https://codereview.chromium.org/23588009/diff/12001/base/sys_info.h#newcode85
base/sys_info.h:85: // Returns a the contents of /etc/lsb-release as a map.
On 2013/09/24 07:44:51, pneubeck wrote:
> a the -> the

Done.

https://codereview.chromium.org/23588009/diff/12001/base/sys_info.h#newcode96
base/sys_info.h:96: // time of the ChromeOS build).
On 2013/09/24 15:37:53, Daniel Erat wrote:
> nit: s/ChromeOS/Chrome OS/, our product's name is two words, pass it on
> 
> :-P

Done.

https://codereview.chromium.org/23588009/diff/12001/base/sys_info.h#newcode101
base/sys_info.h:101: double lsb_release_time);
On 2013/09/24 15:37:53, Daniel Erat wrote:
> it'd be best to make this take a Time instead of a double, but if that's
> unfeasible for some reason, document what the double is (seconds since epoch?)

Done.

https://codereview.chromium.org/23588009/diff/12001/base/sys_info_chromeos.cc
File base/sys_info_chromeos.cc (right):

https://codereview.chromium.org/23588009/diff/12001/base/sys_info_chromeos.cc...
base/sys_info_chromeos.cc:49: env->GetVar("LSB_RELEASE_TIME",
&lsb_release_time_str);
On 2013/09/24 15:37:53, Daniel Erat wrote:
> nit: move both of these to constants so you can share them with
> SetChromeOSVersionInfoForTest()

Done.

https://codereview.chromium.org/23588009/diff/12001/base/sys_info_chromeos.cc...
base/sys_info_chromeos.cc:52: StringToDouble(lsb_release_time_str, &us);
On 2013/09/24 15:37:53, Daniel Erat wrote:
> check the return value and maybe log an error

Added a test, but this occurs before logging is initialized, so that won't be
useful in practice. However, these variables show up in chrome://system so we
should be able to debug any strange behavior.

https://codereview.chromium.org/23588009/diff/12001/base/sys_info_chromeos.cc...
base/sys_info_chromeos.cc:57: // only happen in non-chromeos environments.
On 2013/09/24 15:37:53, Daniel Erat wrote:
> nit: s/chromeos/Chrome OS/
> 
> also, worthwhile calling IsRunningOnChromeOS() here and logging an error if
the
> environment variables aren't set?

Again, logging won't help, but we do set lsb-source below for debugging in
chrome://system.

https://codereview.chromium.org/23588009/diff/12001/base/sys_info_chromeos.cc...
base/sys_info_chromeos.cc:67: lsb_release_map_["lsb-source"] = parsed_from_env ?
"env" : "file";
On 2013/09/24 15:37:53, Daniel Erat wrote:
> nit: move these strings to constants (assuming that you'll eventually want to
> access them from somewhere)

Made them constants, but we only display these in chrome://system, we never
compare them in the code, so I didn't expose them anywhere.

https://codereview.chromium.org/23588009/diff/12001/base/sys_info_chromeos.cc...
base/sys_info_chromeos.cc:96: for (size_t i = 0; i  < pairs.size(); ++i) {
On 2013/09/24 15:37:53, Daniel Erat wrote:
> nit: remove extra space before '<'

Done.

https://codereview.chromium.org/23588009/diff/12001/base/sys_info_chromeos.cc...
base/sys_info_chromeos.cc:127: SysInfo::LsbReleaseMap lsb_release_map_;
On 2013/09/24 15:37:53, Daniel Erat wrote:
> On 2013/09/24 07:44:51, pneubeck wrote:
> > FWIW, before this change only a fixed amount of memory was consumed (3 x
int32
> > and 1 x bool). It's not clear how much memory lsb_release_map_ might
consume.
> > For all other platforms SysInfo doesn't cache any data.
> > 
> > My question is mostly: do we want to optimize here for performance (parse
> once)
> > or for memory consumption (don't cache)? The former makes IMO only sense if
> > access to SysInfo is time critical or occurs frequently and parsing takes
too
> > much time. However it seems that caching is required because of the sandbox.
> Is
> > that right?
> > 
> > To mitigate the issue, either document how many entries /etc/lsb-release has
> or
> > maybe whitelist the keys. As I don't expect this to be a huge problem, feel
> free
> > to ignore this comment.
> 
> fwiw, i don't think there's any harm in keeping all of lsb-release in memory.

This is tiny, all things considered, but I added a comment.

https://codereview.chromium.org/23588009/diff/12001/chrome/app/chrome_main_de...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/23588009/diff/12001/chrome/app/chrome_main_de...
chrome/app/chrome_main_delegate.cc:752:
base::SysInfo::OperatingSystemVersionNumbers(
On 2013/09/24 07:44:51, pneubeck wrote:
> This call assumes more about the SysInfo implementation than the SysInfo
> interface declares.
> Instead there should be a call base::SysInfo::CacheVersionNumbers() (or more
> scalable CacheInfo() ).

I actually don't know why this was put here and didn't notice that these weren't
being used. I'm going to eliminate this block entirely, I don't think it is
useful. We'll see if the tests agree with me.

https://codereview.chromium.org/23588009/diff/12001/chrome/app/chrome_main_de...
chrome/app/chrome_main_delegate.cc:759: base::SysInfo::GetLsbReleaseTime());
On 2013/09/24 15:37:53, Daniel Erat wrote:
> nit: indenting one space seems strange -- indent four spaces beyond the
previous
> line's "base", perhaps?
Removed.

https://codereview.chromium.org/23588009/diff/12001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/external_metrics.cc (right):

https://codereview.chromium.org/23588009/diff/12001/chrome/browser/chromeos/e...
chrome/browser/chromeos/external_metrics.cc:103: // mess.  At least this code is
temporary.
On 2013/09/24 15:37:53, Daniel Erat wrote:
> nit: delete this outdated comment

Done.

https://codereview.chromium.org/23588009/diff/12001/chrome/browser/chromeos/e...
chrome/browser/chromeos/external_metrics.cc:106: if
(base::SysInfo::GetLsbReleaseBoard() != "parrot")
On 2013/09/24 15:37:53, Daniel Erat wrote:
> this code makes me sad.
> 
> (probably not your fault.)
> 
> that is all.

Yeah... at least we're not doing an extra file read now (even if this is on the
IO thread).

[Daniel Erat, 2013-09-24 18:09:08]

lgtm

https://codereview.chromium.org/23588009/diff/20001/base/sys_info_chromeos.cc
File base/sys_info_chromeos.cc (right):

https://codereview.chromium.org/23588009/diff/20001/base/sys_info_chromeos.cc...
base/sys_info_chromeos.cc:73: lsb_release_map_["lsb-source"] =
nit: for consistency, the key string probably be in the same place as the value
strings. i'm fine with them all staying inline -- thanks for the explanation
that these don't get read in code anywhere but do still show up in
chrome://system (presumably it's dumping the whole map)

[stevenjb, 2013-09-24 18:31:33]

https://codereview.chromium.org/23588009/diff/20001/base/sys_info_chromeos.cc
File base/sys_info_chromeos.cc (right):

https://codereview.chromium.org/23588009/diff/20001/base/sys_info_chromeos.cc...
base/sys_info_chromeos.cc:73: lsb_release_map_["lsb-source"] =
On 2013/09/24 18:09:09, Daniel Erat wrote:
> nit: for consistency, the key string probably be in the same place as the
value
> strings. i'm fine with them all staying inline -- thanks for the explanation
> that these don't get read in code anywhere but do still show up in
> chrome://system (presumably it's dumping the whole map)
Good point. Done. (And yes, we just dump the whole map).

[brettw, 2013-09-25 17:36:46]

base owners lgtm, didn't check the details.

[commit-bot: I haz the power, 2013-09-25 19:34:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/23588009/37001

[commit-bot: I haz the power, 2013-09-25 21:27:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/23588009/37001

[stevenjb, 2013-09-26 21:51:30]

Committed patchset #10 manually as r225581.