Alessio Placitelli [:Dexter] Reporter
	Description • 10 years ago

We should add CPU Vendor, Family, Model and Stepping to the Telemetry Environment as per the Environment Spec (bug 1069880).

I can think of two approaches:

1) Using ctypes in TelemetryEnvironment.jsm, defining a function for each platform.

2) Exposing the data through |Services.sysinfo|, probably by modifying [2].

[1] - https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/tests/unit_aus_update/urlConstruction.js#272
[2] - https://dxr.mozilla.org/mozilla-central/source/xpcom/base/nsSystemInfo.cpp

	Georg Fritzsche [:gfritzsche]
	Comment 1 • 9 years ago

(In reply to Alessio Placitelli [:Dexter] from comment #0)
> We should add CPU Vendor, Family, Model and Stepping to the Telemetry
> Environment as per the Environment Spec (bug 1069880).
> 
> I can think of two approaches:
> 
> 1) Using ctypes in TelemetryEnvironment.jsm, defining a function for each
> platform.
> 
> 2) Exposing the data through |Services.sysinfo|, probably by modifying [2].

Nathan, does 2) sound reasonable for you?
Any specific things to watch out for with this?

	Nathan Froyd [:froydnj]
	Comment 2 • 9 years ago

Exposing the data through Services.sysinfo sounds much more reasonable than trying to do it via ctypes; you'd have to write C++ code for ctypes anyway, I think, so you might as well do it in nsSystemInfo instead.

All that information is really detailed, though...what is it buying you that the current acquisition of SSE/AVX/etc. capabilities isn't providing?  I think it makes sense for crashes (some crashes only occur on particular manufacturers's CPUs), but how would it be useful for general-purpose telemetry?

	Georg Fritzsche [:gfritzsche]
	Comment 3 • 9 years ago

This was requested by Benjamin, moving the ni?.

	Benjamin Smedberg
	Comment 4 • 9 years ago

We're unifying a lot of the analysis infrastructure, and measuring crash counts in telemetry, so having the same aggregating data we're using in crash-stats is important. This may also drive self-support tips about the broken AMD CPUs.

	Nathan Froyd [:froydnj]
	Comment 5 • 9 years ago

Works for me!

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 7 • 9 years ago

Is it enough to just have the whole "info" string like crash report, e.g., "
GenuineIntel family 6 model 15 stepping 13", or do we want this parsed out into separate fields as in the placeholder telemetry code?

	Georg Fritzsche [:gfritzsche]
	Comment 9 • 9 years ago

Comment on attachment 8645172 [details] [diff] [review]
(WIP) Family, model, stepping, cores, cache, cpu speed on Mac on Win, virtual memory max on Win, vendor on Mac.

Review of attachment 8645172 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me.

The test coverage for this to update is here:
https://dxr.mozilla.org/mozilla-central/rev/d6ea652c579992daa9041cc9718bb7c6abefbc91/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js#365

... and the documentation updates would go in here (note ones platform availability and other potential caveats would be great):
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/docs/environment.rst

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +1063,5 @@
> +      vendor: getSysinfoProperty("cpuvendor", null),
> +      family: getSysinfoProperty("cpufamily", null),
> +      model: getSysinfoProperty("cpumodel", null),
> +      stepping: getSysinfoProperty("cpustepping", null),
> +      cacheMB: getSysinfoProperty("cpucache", null),

I'm not sure what the cache sizes in machines currently used in the wild are.
Is MB precision sufficient for our needs?

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 10 • 9 years ago

KB for cache, otherwise the same patch.  Part 1, though I'll probably land this while the rest of the parameters and OSs are getting worked on.

	Georg Fritzsche [:gfritzsche]
	Comment 11 • 9 years ago

Comment on attachment 8650602 [details] [diff] [review]
Part 1. Family, model, stepping, cores, cache, cpu speed on Mac on Win, virtual memory max on Win, vendor on Mac.   r=gfritzsche

Review of attachment 8650602 [details] [diff] [review]:
-----------------------------------------------------------------

Are the documentation & test updates coming up in a separate patch?
We should not land this without these.

Aaron, can you take a quick look over the Windows nsSystemInfo.cpp changes?
It's been a while since i touched those APIs.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 12 • 9 years ago

(In reply to Georg Fritzsche [:gfritzsche] from comment #11)
> Comment on attachment 8650602 [details] [diff] [review]
> Part 1. Family, model, stepping, cores, cache, cpu speed on Mac on Win,
> virtual memory max on Win, vendor on Mac.   r=gfritzsche
> 
> Review of attachment 8650602 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Are the documentation & test updates coming up in a separate patch?
> We should not land this without these.

Right, sorry, I'll get the documentation and test updates in a separate patch and not land anything without them.

	Georg Fritzsche [:gfritzsche]
	Comment 13 • 9 years ago

Testing this on my MBP i get:
  cpu.count 	8
  cpu.cores 	4
  cpu.vendor 	GenuineIntel
  cpu.family 	6
  cpu.model 	70
  cpu.stepping 	1
  cpu.cacheKB 	8192
  cpu.speedMHz 	2600

... vs. this info from the "system information" tool:
  Model Name:	MacBook Pro
  Model Identifier:	MacBookPro11,3
  Processor Name:	Intel Core i7
  Processor Speed:	2,6 GHz
  Number of Processors:	1
  Total Number of Cores:	4
  L2 Cache (per Core):	256 KB
  L3 Cache:	6 MB
  Memory:	16 GB

So, apparently we fail to retrieve the L3 cache and report the L2 cache instead?
Should we maybe just report both L3 & L2 cache where available?

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 15 • 9 years ago

(In reply to Georg Fritzsche [:gfritzsche] from comment #13)
> ...
> 
> So, apparently we fail to retrieve the L3 cache and report the L2 cache
> instead?
> Should we maybe just report both L3 & L2 cache where available?

That's a good point.  Yes, we try L3, if we don't find the information, we take L2.  I don't have a strong opinion on which number to report here (L2 only, L3 ony, sum of the two, L2 and L3 separately.)  I'm also not sure we're going to make serious decisions based on that piece of telemetry information anyway :)

Any strong feelings one way or the other?  There doesn't seem to be information at that level of detail in this or bug 1069880, or any of the documents attached to it.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 16 • 9 years ago

For example, on Windows we just report the largest cache.

	Georg Fritzsche [:gfritzsche]
	Comment 17 • 9 years ago

Comment on attachment 8653558 [details] [diff] [review]
Part 1B. Tests and documentation for Part 1.

Review of attachment 8653558 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: toolkit/components/telemetry/docs/environment.rst
@@ +79,5 @@
>          cpu: {
> +            count: <number>,  // e.g. 8, or null on failure - logical cpus
> +            cores: <number>, // e.g., 4, or null on failure - physical cores, windows, mac only
> +            vendor: <string>, // e.g. "GenuineIntel", or null on failure, mac only
> +            family: <string>, // null on failure, windows, mac only

Nit: "windows & mac only"? I read it as "mac only" on first look.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +369,5 @@
> +  if (gIsWindows || gIsMac) {
> +    if (gIsWindows) {
> +      EXTRA_CPU_FIELDS = ["cores", "model", "family", "stepping", "cacheKB", "speedMHz"];
> +    } else {
> +      // This is gIsMac:

Nit: If we write it here, might as well explicitly use |else if (gIsMac)|?

	Vladan Djeric (:vladan)
	Comment 18 • 9 years ago

I don't have a strong opinion on how we report caches, as long as what we report is consistent. Always L2, or always L3, or always L2+L3 where some of these can be null.

Note that if we have the cpumodel, we can obtain the full cache info from Intel's and AMD's sites and use these in analyses.

	Georg Fritzsche [:gfritzsche]
	Comment 19 • 9 years ago

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #18)
> I don't have a strong opinion on how we report caches, as long as what we
> report is consistent. Always L2, or always L3, or always L2+L3 where some of
> these can be null.

Ok, i'd say lets go for the last option, then we are covered either way.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 20 • 9 years ago

Just reporting the sum gets us into interesting situations; for example about:telemetry would show 9472KB for the system I have with Windows (8 * 32KB + 4 * 256KB + 8MB), on a Xeon E3-1270 v2 that shows "Intel Smart Cache 8 MB".  L1 would show up per logical core, L2 per physical core, L3 once.  What would be the right value to show there?

	Georg Fritzsche [:gfritzsche]
	Comment 21 • 9 years ago

Sorry, that means both separately, with a value of null where they are not present.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 22 • 9 years ago

(In reply to Georg Fritzsche [:gfritzsche] from comment #13)
> Testing this on my MBP i get:
> ...
> So, apparently we fail to retrieve the L3 cache...

That's a bug in the patch above.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 23 • 9 years ago

Compared to the previous version of the patch:

The test is the same as the r+'d version, with nits addressed and accounting for L2 + L3 caches.
Added L2 and L3 cache separately.
Fixed some 32-bit/64-bit mixups on the Mac side, which is why L3 cache didn't work.  Put in a bunch of asserts to catch similar problems.

	Georg Fritzsche [:gfritzsche]
	Comment 24 • 9 years ago

Comment on attachment 8653664 [details] [diff] [review]
Part 1. Family, model, stepping, cores, cache, cpu speed on Mac on Win, virtual memory max on Win, vendor on Mac.   r=gfritzsche

Review of attachment 8653664 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks good to me.
I'll defer the Windows-specific nsSystemInfo.cpp changes to aklotz.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +1089,5 @@
> +      family: getSysinfoProperty("cpufamily", null),
> +      model: getSysinfoProperty("cpumodel", null),
> +      stepping: getSysinfoProperty("cpustepping", null),
> +      L2cacheKB: getSysinfoProperty("cpucachel2", null),
> +      L3cacheKB: getSysinfoProperty("cpucachel3", null),

Lets use consistent camel-case, i.e. l2CacheKB, l3CacheKB?

::: xpcom/base/nsSystemInfo.cpp
@@ +246,5 @@
> +    PDWORD);
> +static void GetProcessorInformation(int* physical_cpus, int* cache_size_L2, int* cache_size_L3) {
> +  *physical_cpus = 0;
> +  *cache_size_L2 = 0; // This will be in kbytes
> +  *cache_size_L3 = 0; // This will be in kbytes

Style nit: The surrounding code uses camel-case.

@@ +259,5 @@
> +  // Determine buffer size, allocate and get processor information.
> +  // Size can change between calls (unlikely), so a loop is done.
> +  SYSTEM_LOGICAL_PROCESSOR_INFORMATION infoBuffer[32];
> +  SYSTEM_LOGICAL_PROCESSOR_INFORMATION* infos = &infoBuffer[0];
> +  DWORD return_length = sizeof(infoBuffer);

Style nit: The surrounding code uses camel-case.

	(No longer employed by Mozilla) Aaron Klotz
	Comment 25 • 9 years ago

Comment on attachment 8653664 [details] [diff] [review]
Part 1. Family, model, stepping, cores, cache, cpu speed on Mac on Win, virtual memory max on Win, vendor on Mac.   r=gfritzsche

Review of attachment 8653664 [details] [diff] [review]:
-----------------------------------------------------------------

OK with our nits addressed.

::: xpcom/base/nsSystemInfo.cpp
@@ +243,5 @@
> +// Based on media/webrtc/trunk/webrtc/base/systeminfo.cc
> +typedef BOOL (WINAPI *LPFN_GLPI)(
> +    PSYSTEM_LOGICAL_PROCESSOR_INFORMATION,
> +    PDWORD);
> +static void GetProcessorInformation(int* physical_cpus, int* cache_size_L2, int* cache_size_L3) {

style nit: static void on separate line from the rest of the signature. Open curly brace on its own line.

@@ +244,5 @@
> +typedef BOOL (WINAPI *LPFN_GLPI)(
> +    PSYSTEM_LOGICAL_PROCESSOR_INFORMATION,
> +    PDWORD);
> +static void GetProcessorInformation(int* physical_cpus, int* cache_size_L2, int* cache_size_L3) {
> +  *physical_cpus = 0;

Let's add some assertions to the top of this function to verify that a caller hasn't passed nullptr to any of these paramaters.

@@ +252,5 @@
> +  // GetLogicalProcessorInformation() is available on Windows XP SP3 and beyond.
> +  LPFN_GLPI glpi = reinterpret_cast<LPFN_GLPI>(GetProcAddress(
> +      GetModuleHandle(L"kernel32"),
> +      "GetLogicalProcessorInformation"));
> +  if (NULL == glpi) {

s/NULL/nullptr/

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 26 • 9 years ago

(In reply to Georg Fritzsche [:gfritzsche] from comment #24)
> Comment on attachment 8653664 [details] [diff] [review]
> Part 1. Family, model, stepping, cores, cache, cpu speed on Mac on Win,
> virtual memory max on Win, vendor on Mac.   r=gfritzsche
> 
> Review of attachment 8653664 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, this looks good to me.
> I'll defer the Windows-specific nsSystemInfo.cpp changes to aklotz.
> 
> ::: toolkit/components/telemetry/TelemetryEnvironment.jsm
> @@ +1089,5 @@
> > +      family: getSysinfoProperty("cpufamily", null),
> > +      model: getSysinfoProperty("cpumodel", null),
> > +      stepping: getSysinfoProperty("cpustepping", null),
> > +      L2cacheKB: getSysinfoProperty("cpucachel2", null),
> > +      L3cacheKB: getSysinfoProperty("cpucachel3", null),
> 
> Lets use consistent camel-case, i.e. l2CacheKB, l3CacheKB?

I'll make that change.

> 
> ::: xpcom/base/nsSystemInfo.cpp
> @@ +246,5 @@
> > +    PDWORD);
> > +static void GetProcessorInformation(int* physical_cpus, int* cache_size_L2, int* cache_size_L3) {
> > +  *physical_cpus = 0;
> > +  *cache_size_L2 = 0; // This will be in kbytes
> > +  *cache_size_L3 = 0; // This will be in kbytes
> 
> Style nit: The surrounding code uses camel-case.
> 
> @@ +259,5 @@
> > +  // Determine buffer size, allocate and get processor information.
> > +  // Size can change between calls (unlikely), so a loop is done.
> > +  SYSTEM_LOGICAL_PROCESSOR_INFORMATION infoBuffer[32];
> > +  SYSTEM_LOGICAL_PROCESSOR_INFORMATION* infos = &infoBuffer[0];
> > +  DWORD return_length = sizeof(infoBuffer);
> 
> Style nit: The surrounding code uses camel-case.

I'll fix the infoBuffer to be info_buffer, but I'll leave the other things with underscores - since this function pretty much unchanged came from webrtc code, I wanted to leave the code looking like the original just to make it easier to notice if anything changes.  Not a very solid argument though.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 27 • 9 years ago

With (most, except the ones explained away in above comment) nits addressed.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 30 • 9 years ago

Tweak the test a bit to cover scenarios where we don't have L2 and L3 cache.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 31 • 9 years ago

Windows wants explicit <string> include.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 33 • 9 years ago

Passing try, minor changes, carrying r+.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 36 • 9 years ago

Make it linux GTK specific, that was the intent.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 37 • 9 years ago

Don't know where to find cpu frequency on all Linux systems, so leave it optional when unit testing.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 38 • 9 years ago

Comment on attachment 8656183 [details] [diff] [review]
Part 2. Linux support for the items from Part 1.

Lee, is this "linux-e" enough?

	Lee Salzman [:lsalzman]
	Comment 39 • 9 years ago

Comment on attachment 8656183 [details] [diff] [review]
Part 2. Linux support for the items from Part 1.

Review of attachment 8656183 [details] [diff] [review]:
-----------------------------------------------------------------

Looks tolerabl-e linux-e.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 40 • 9 years ago

Comment on attachment 8656183 [details] [diff] [review]
Part 2. Linux support for the items from Part 1.

Same as the first patch was doing for windows + mac.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 41 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c50193ba6c1
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eef62aa7176e - with the modified patch 2.

	Georg Fritzsche [:gfritzsche]
	Comment 42 • 9 years ago

Comment on attachment 8656183 [details] [diff] [review]
Part 2. Linux support for the items from Part 1.

Review of attachment 8656183 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the Telemetry changes with the comments addressed.
I'm not sure if we should use that - apparently abandoned - Tokenizer given the other implementation that is actually used.

::: toolkit/components/telemetry/docs/environment.rst
@@ +78,5 @@
>          virtualMaxMB: <number>, // windows-only
>          isWow64: <bool>, // windows-only
>          cpu: {
>              count: <number>,  // e.g. 8, or null on failure - logical cpus
> +            cores: <number>, // e.g., 4, or null on failure - physical cores

"desktop-only"
Ditto for the others below.

::: toolkit/components/telemetry/tests/unit/head.js
@@ +13,5 @@
>  const gIsMac = ("@mozilla.org/xpcom/mac-utils;1" in Cc);
>  const gIsAndroid =  ("@mozilla.org/android/bridge;1" in Cc);
>  const gIsGonk = ("@mozilla.org/cellbroadcast/gonkservice;1" in Cc);
> +const gIsLinux = ("@mozilla.org/gnome-gconf-service;1" in Cc) ||
> +                 ("@mozilla.org/gio-service;1" in Cc);

The changes here don't seem Gnome specific - i think we should instead use the newish AppConstants.jsm and check AppConstants.platform=="linux".

::: xpcom/ds/Tokenizer.h
@@ +323,5 @@
> +  // and value is separated with a colon (by default.)  The separator should
> +  // not be used elsewhere in the key or value entry.
> +  // Same functionality and limitations as the Tokenizer it uses.
> +  //
> +  static bool ParseConfigFile(const std::string& aFilename,

* (ConfigFile seems a bit generic, ParseKeyValueFile?)
* Tokenizer.h isn't included anywhere AFAICT, might be unused?
* we have nsCharSeparatedTokenizer, which is actually used across the tree
* maybe we can just use nsCharSeparatedTokenizer and wrap its usage in a helper function in nsSystemInfo.cpp?

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 43 • 9 years ago

(In reply to Georg Fritzsche [:gfritzsche] [away Sep 5 - 13] from comment #42)
> ...
> * Tokenizer.h isn't included anywhere AFAICT, might be unused?
> * we have nsCharSeparatedTokenizer, which is actually used across the tree
> * maybe we can just use nsCharSeparatedTokenizer and wrap its usage in a
> helper function in nsSystemInfo.cpp?

I'll take a look at nsCharSeparatedTokenizer, thanks for the pointer.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 44 • 9 years ago

(In reply to Milan Sreckovic [:milan] from comment #43)
> (In reply to Georg Fritzsche [:gfritzsche] [away Sep 5 - 13] from comment
> #42)
> > ...
> > * Tokenizer.h isn't included anywhere AFAICT, might be unused?
> > * we have nsCharSeparatedTokenizer, which is actually used across the tree
> > * maybe we can just use nsCharSeparatedTokenizer and wrap its usage in a
> > helper function in nsSystemInfo.cpp?
> 
> I'll take a look at nsCharSeparatedTokenizer, thanks for the pointer.

By the way, the "unused" part of the Tokenizer - it only showed up about 6 weeks ago, so it isn't surprising.  This blogpost describes it: http://www.janbambas.cz/string-parsing-made-simple-with-mozillatokenizer/

	Georg Fritzsche [:gfritzsche]
	Comment 45 • 9 years ago

(In reply to Milan Sreckovic [:milan] from comment #44)
> (In reply to Milan Sreckovic [:milan] from comment #43)
> > (In reply to Georg Fritzsche [:gfritzsche] [away Sep 5 - 13] from comment
> > #42)
> > > ...
> > > * Tokenizer.h isn't included anywhere AFAICT, might be unused?
> > > * we have nsCharSeparatedTokenizer, which is actually used across the tree
> > > * maybe we can just use nsCharSeparatedTokenizer and wrap its usage in a
> > > helper function in nsSystemInfo.cpp?
> > 
> > I'll take a look at nsCharSeparatedTokenizer, thanks for the pointer.
> 
> By the way, the "unused" part of the Tokenizer - it only showed up about 6
> weeks ago, so it isn't surprising.  This blogpost describes it:
> http://www.janbambas.cz/string-parsing-made-simple-with-mozillatokenizer/

Ah, i see - maybe we should use the cleaner of the two then.

Note that i'll be out from tomorrow afternoon on - :vladan or :dexter should be good for Telemetry related questions/flags.

	(No longer employed by Mozilla) Aaron Klotz
	Comment 46 • 9 years ago

Comment on attachment 8655664 [details] [diff] [review]
Part 3. Vendor string on windows.

Review of attachment 8655664 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with fix applied.

::: xpcom/base/nsSystemInfo.cpp
@@ +389,5 @@
> +                         reinterpret_cast<LPBYTE>(cpuVendorStr),
> +                         &len) == ERROR_SUCCESS &&
> +        vtype == REG_SZ && len % 2 == 0 && len > 1) {
> +      cpuVendorStr[len/2] = 0; // In case it isn't null terminated
> +      CopyUTF16toUTF8(nsAutoString(cpuVendorStr), cpuVendor);

s/nsAutoString/nsDependentString/

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 47 • 9 years ago

Nits, plus, used the nsCharSeparatedTokenizer for the overall parsing, made the code simpler, and no changes to the (new and unrelated) Tokenizer class, though the later came in handy for some simple parsing within the values.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 48 • 9 years ago

Use the right string type.

	Georg Fritzsche [:gfritzsche]
	Comment 49 • 9 years ago

Comment on attachment 8656770 [details] [diff] [review]
Part 2. Linux support for the items from Part 1.  Carry r=gfritzsche

Review of attachment 8656770 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/base/nsSystemInfo.cpp
@@ +85,5 @@
> +      key = tokens.nextToken();
> +      if (tokens.hasMoreTokens()) {
> +        value = tokens.nextToken();
> +      }
> +      aKeyValuePairs[key] = value;

Isn't it enough to only insert if we found two tokens?

@@ +485,5 @@
> +    // cpuVendor from "vendor_id"
> +    cpuVendor.Assign(keyValuePairs[NS_LITERAL_CSTRING("vendor_id")]);
> +
> +    // cpuFamily from "cpu family"
> +    cpuFamily = atoi(keyValuePairs[NS_LITERAL_CSTRING("cpu family")].get());

Theoretical failure scenario because of missing conversion failure checks:
If the converted value for atoi is outside the target types range, the return value is undefined.

Ditto the others below.

@@ +501,5 @@
> +      // cacheSizeL3 from "cache size"
> +      Tokenizer::Token t;
> +      Tokenizer p(keyValuePairs[NS_LITERAL_CSTRING("cache size")]);
> +      if (p.Next(t) && t.Type() == Tokenizer::TOKEN_INTEGER) {
> +        cacheSizeL3 = static_cast<int>(t.AsInteger());

No range checks?

@@ +503,5 @@
> +      Tokenizer p(keyValuePairs[NS_LITERAL_CSTRING("cache size")]);
> +      if (p.Next(t) && t.Type() == Tokenizer::TOKEN_INTEGER) {
> +        cacheSizeL3 = static_cast<int>(t.AsInteger());
> +        if (p.Next(t) && t.Type() == Tokenizer::TOKEN_WORD &&
> +            t.AsString() != NS_LITERAL_CSTRING("KB")) {

Can you add a comment on what this does?

@@ +504,5 @@
> +      if (p.Next(t) && t.Type() == Tokenizer::TOKEN_INTEGER) {
> +        cacheSizeL3 = static_cast<int>(t.AsInteger());
> +        if (p.Next(t) && t.Type() == Tokenizer::TOKEN_WORD &&
> +            t.AsString() != NS_LITERAL_CSTRING("KB")) {
> +          cacheSizeL3 = 13;

Whats up with the specific value 13 here?

@@ +518,5 @@
> +    if (getline(input, line)) {
> +      Tokenizer::Token t;
> +      Tokenizer p(line.c_str());
> +      if (p.Next(t) && t.Type() == Tokenizer::TOKEN_INTEGER) {
> +        cpuSpeed = static_cast<int>(t.AsInteger()/1000);

No range check?

@@ +531,5 @@
> +    if (getline(input, line)) {
> +      Tokenizer::Token t;
> +      Tokenizer p(line.c_str(), nullptr, "K");
> +      if (p.Next(t) && t.Type() == Tokenizer::TOKEN_INTEGER) {
> +        cacheSizeL2 = static_cast<int>(t.AsInteger());

No range check?

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 50 • 9 years ago

(In reply to Georg Fritzsche [:gfritzsche] [away Sep 5 - 13] from comment #49)
> Comment on attachment 8656770 [details] [diff] [review]
> Part 2. Linux support for the items from Part 1.  Carry r=gfritzsche
> 
> Review of attachment 8656770 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/base/nsSystemInfo.cpp
> @@ +85,5 @@
> > +      key = tokens.nextToken();
> > +      if (tokens.hasMoreTokens()) {
> > +        value = tokens.nextToken();
> > +      }
> > +      aKeyValuePairs[key] = value;
> 
> Isn't it enough to only insert if we found two tokens?

No, there are cases where we have the key, and the value is blank, and we want to record the blank.

> 
> @@ +485,5 @@
> > +    // cpuVendor from "vendor_id"
> > +    cpuVendor.Assign(keyValuePairs[NS_LITERAL_CSTRING("vendor_id")]);
> > +
> > +    // cpuFamily from "cpu family"
> > +    cpuFamily = atoi(keyValuePairs[NS_LITERAL_CSTRING("cpu family")].get());
> 
> Theoretical failure scenario because of missing conversion failure checks:
> If the converted value for atoi is outside the target types range, the
> return value is undefined.
> 
> Ditto the others below.

Right, those would just get returned as 0.  We can see if the telemetry than has any of those values, and worry about it at that point?

> 
> @@ +501,5 @@
> > +      // cacheSizeL3 from "cache size"
> > +      Tokenizer::Token t;
> > +      Tokenizer p(keyValuePairs[NS_LITERAL_CSTRING("cache size")]);
> > +      if (p.Next(t) && t.Type() == Tokenizer::TOKEN_INTEGER) {
> > +        cacheSizeL3 = static_cast<int>(t.AsInteger());
> 
> No range checks?

As in, if the cache returned is outside of the range of 32-bit integer?  Given that we're just using this value to display the cache, I'm not horribly worried about it.


> 
> @@ +503,5 @@
> > +      Tokenizer p(keyValuePairs[NS_LITERAL_CSTRING("cache size")]);
> > +      if (p.Next(t) && t.Type() == Tokenizer::TOKEN_INTEGER) {
> > +        cacheSizeL3 = static_cast<int>(t.AsInteger());
> > +        if (p.Next(t) && t.Type() == Tokenizer::TOKEN_WORD &&
> > +            t.AsString() != NS_LITERAL_CSTRING("KB")) {
> 
> Can you add a comment on what this does?

Will do.

> 
> @@ +504,5 @@
> > +      if (p.Next(t) && t.Type() == Tokenizer::TOKEN_INTEGER) {
> > +        cacheSizeL3 = static_cast<int>(t.AsInteger());
> > +        if (p.Next(t) && t.Type() == Tokenizer::TOKEN_WORD &&
> > +            t.AsString() != NS_LITERAL_CSTRING("KB")) {
> > +          cacheSizeL3 = 13;
> 
> Whats up with the specific value 13 here?

Nothing that makes any sense, just a leftover from debugging.  I'll clean it up.

> 
> @@ +518,5 @@
> > +    if (getline(input, line)) {
> > +      Tokenizer::Token t;
> > +      Tokenizer p(line.c_str());
> > +      if (p.Next(t) && t.Type() == Tokenizer::TOKEN_INTEGER) {
> > +        cpuSpeed = static_cast<int>(t.AsInteger()/1000);
> 
> No range check?
> 
> @@ +531,5 @@
> > +    if (getline(input, line)) {
> > +      Tokenizer::Token t;
> > +      Tokenizer p(line.c_str(), nullptr, "K");
> > +      if (p.Next(t) && t.Type() == Tokenizer::TOKEN_INTEGER) {
> > +        cacheSizeL2 = static_cast<int>(t.AsInteger());
> 
> No range check?


How strongly do you feel about the int64->int32 range checks for this code?  I can comment that we don't care, or I can put the range check in.  I'm leaning towards the former, but it's your r+ on it, so I'm happy to add the range check if you think we need it.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 51 • 9 years ago

Address the review comments - now with range checks, tokenizer to avoid atoi direct calls, comments about some of the potentially confusing code.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 52 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=adbc063e8edf

(In reply to Milan Sreckovic [:milan] from comment #50)
> ...
> > No range check?
> 
> How strongly do you feel about the int64->int32 range checks for this code? 

Never mind the question, added the range checks.

	Pulsebot
	Comment 53 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/95b43173a47b
https://hg.mozilla.org/integration/fx-team/rev/912d328e8ac0
https://hg.mozilla.org/integration/fx-team/rev/6d67e5c0e73b

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 54 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/95b43173a47b
https://hg.mozilla.org/mozilla-central/rev/912d328e8ac0
https://hg.mozilla.org/mozilla-central/rev/6d67e5c0e73b

	Khalid Syfullah Zaman [:khalid32]
	Comment 55 • 9 years ago

I have reproduced this bug on Nightly 38.0a1 (2015-02-02) on ubuntu 14.04 LTS, 32 bit!

The bug's fix is now verified on Latest Beta 43.0b3!

Build ID: 20151112144305
User Agent: Mozilla/5.0 (X11; Linux i686; rv:43.0) Gecko/20100101 Firefox/43.0

[testday-20151113]

	Saheda Reza [:Antora]
	Comment 56 • 9 years ago

I have reproduced this on Firefox nightly 38.0a1 according to (2015-2-2)

It's verified on Latest Firefox Beta
Build ID 	20151123113812
User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:43.0) Gecko/20100101 Firefox/43.0

Tested OS- 32bit windows8.1