1255472 - Collect actual major and minor version data on Windows 10

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Ryan VanderMeulen [:RyanVM]]

Windows 10 has moved away from traditional service packs in its release model. Microsoft now uses major updates once or twice a year (the Build 1511 November Update, for example) and monthly cumulative updates that deliver bug fixes and security updates at the same time.

These updates follow a predictable version numbering system, where the OS Build (as shown by the winver command) uses an X.Y system that denotes which major and minor updates have been installed. For example, 10240.0 represents an unpatched RTM build, and 10586.164 represents a fully patched build as of today. The X value is incremented for the major releases and the Y value is incremented for each monthly cumulative update.

Right now, we have the os.version, servicePackMajor, and servicePackMinor fields for collecting information about which version of Windows a user is on. For a fully-patched Windows 10 build, we get os.version = 10.0, servicePackMajor = 0, and servicePackMinor = 0, giving us no visibility into the effect any Windows updates might have caused (or usage statistics across the different releases).

I propose that we update servicePackMajor and servicePackMinor on Windows 10 to reflect the various patch levels, i.e. servicePackMajor = 10586 & servicePackMinor = 164 as that will give us more useful data that just treating Windows 10 as a single population like we're currently doing. For example, we'll be able to see if a recent cumulative update is causing a crash spike with better accuracy than relying on the date alone. It was also inform our decision making process about which version of Windows 10 to test in automation.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Can we safely expect servicePackMajor and servicePackMinor to stay 0 in the future?

Comment 2

[Benjamin Smedberg]

No, we should not assume that SP won't be used, even though that's the current plan. Let's record the X,Y values for win10 in new fields.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Relevant-looking Windows registry values:
> [HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows NT\CurrentVersion]
> "BuildLab"="10586.th2_release_sec.160223-1728"
> "BuildLabEx"="10586.162.amd64fre.th2_release_sec.160223-1728"
> "CurrentBuild"="10586"
> "CurrentBuildNumber"="10586"
> "ReleaseId"="1511"
> "UBR"=dword:000000a4

The UBR value is 164, which correlates to the current monthly patch level. I'm going to ni? myself to verify next week that the value increments as expected after the next monthly cumulative update.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Diving into the client API side quickly:

The relevant data is collected here:
https://dxr.mozilla.org/mozilla-central/rev/b6683e141c47c022598c0caac3ea8ba8c6236d42/toolkit/components/telemetry/TelemetryEnvironment.jsm#1234

We currently submit "$dwMajorVersion.$dwMinorVersion" which comes from GetVersionEx():
https://dxr.mozilla.org/mozilla-central/rev/b6683e141c47c022598c0caac3ea8ba8c6236d42/nsprpub/pr/src/md/windows/ntmisc.c#830

I see that GetVersionEx() is actually suspect to the manifest version lie shim:
https://blogs.msdn.microsoft.com/chuckw/2013/09/10/manifest-madness/

GetVersionEx() would presumably get us the build number, but i'm not sure yet where we can get the X.Y numbers from.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Aaron, do you happen to know if there is an API that would give us the Windows 10 version details (described in comment 0)?

Comment 6

[Georg Fritzsche [:gfritzsche]]

Looking at windows 10 version history here:
https://en.wikipedia.org/wiki/Windows_10_version_history

... i see that "Treshold 2" versions follow the 10.0.X.Y pattern, but Redstone 1 uses 10.0.X.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

That will presumably change when Redstone (Anniversary Update) ships and they freeze on X.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

The "Pre-release versions of Windows 10 Threshold 2" section shows the same.

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

GetVersionEx() gives you the X as dwBuildNumber provided that we include Windows 10 compat in our manifest (which I believe we do).

I haven't found any better way to access Y other than to read the "UBR" value from the registry. FWIW it appears as though the UBR is incremented by "cumulative update" patches.

The "ReleaseId" value in the registry is also used in version strings.

For example, run Notepad and click Help->About. The version string is shown as
Version <ReleaseId> (OS Build <CurrentBuild>.<UBR>)

It wouldn't surprise me if API access to this additional information was intentionally omitted. Microsoft has been deprecating version APIs due to application developers consistently writing incorrect code to check version numbers for feature detection purposes.

Comment 10

[Georg Fritzsche [:gfritzsche]]

Thanks Aaron.

We can add two optional properties:
(1) windowsBuildNumber
(2) windowsUBR

For (1), we can change [0], also returning the build number and renaming the function.
For (2), we can read the registry key from JS [1].
We can add both data points at [2]. Test coverage can be added at [3] and tests run with |mach test toolkit/components/telemetry/tests/unit|.

0: https://dxr.mozilla.org/mozilla-central/rev/29d5a4175c8b74f45482276a53985cf2568b4be2/toolkit/components/telemetry/TelemetryEnvironment.jsm#324
1: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Accessing_the_Windows_Registry_Using_XPCOm
2: https://dxr.mozilla.org/mozilla-central/rev/29d5a4175c8b74f45482276a53985cf2568b4be2/toolkit/components/telemetry/TelemetryEnvironment.jsm#1244
3: https://dxr.mozilla.org/mozilla-central/rev/29d5a4175c8b74f45482276a53985cf2568b4be2/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js#509

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Just wanted to confirm that the UBR value was changed to 218 as expected with today's released cumulative update.

Comment 12

[Alessio Placitelli [:Dexter]]

Attachment: bug1255472.patch

This WIP patch adds the fields and a minimal test coverage.

Comment 13

[Alessio Placitelli [:Dexter]]

Attachment: bug1255472.patch

This patch adds the windows build number and UBR value. We force the 64-bit registry view when reading the UBR value.

Comment 14

[Alessio Placitelli [:Dexter]]

Attachment: Part 2 - Add the WOW64_64 flag support to WindowsRegistry

There's an interesting problem when querying the UBR registry key from a 32 bit process: reads to "HKLM\SOFTWARE\Microsoft\Windows NT\CurrentVersion\UBR" gets automagically transalted to "HKLM\SOFTWARE\WOW6432Node\Microsoft\Windows NT\CurrentVersion\UBR".

This patch adds the 64-bit registry view support to WindowsRegistry.

Comment 15

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8740901 [details] [diff] [review]
bug1255472.patch

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

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +326,4 @@
>   *
>   * @return An object containing the service pack major and minor versions.
>   */
> +function getBuildAndServicePack() {

`GetWindowsVersionInfo()`?

@@ +330,1 @@
>    const UNKNOWN_SERVICE_PACK = {major: null, minor: null};

Add buildNumber here.

@@ +1247,5 @@
>  
>      if (["gonk", "android"].includes(AppConstants.platform)) {
>        data.kernelVersion = getSysinfoProperty("kernel_version", null);
>      } else if (AppConstants.platform === "win") {
> +      let servicePack = getBuildAndServicePack();

`let versionInfo ...`?

@@ +1252,3 @@
>        data.servicePackMajor = servicePack.major;
>        data.servicePackMinor = servicePack.minor;
> +      data.windowsBuildNumber = servicePack.buildNumber;

We don't intend to use build number or UBR on Windows < 10.
Let's only add it for >=10 (and not pay the bandwidth/storage cost elsewhere).

@@ +1255,5 @@
> +      // Query the UBR key and only add it to the environment if it's available.
> +      // |readRegKey| doesn't throw, but rather returns 'undefined' on error.
> +      let ubr = WindowsRegistry.readRegKey(Ci.nsIWindowsRegKey.ROOT_KEY_LOCAL_MACHINE,
> +                                           WINDOWS_UBR_KEY_PATH, "UBR");
> +      if (typeof(ubr) != 'undefined') {

Why not just `ubr != undefined`?

Comment 16

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8740902 [details] [diff] [review]
Part 2 - Add the WOW64_64 flag support to WindowsRegistry

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

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +1255,5 @@
>        // Query the UBR key and only add it to the environment if it's available.
>        // |readRegKey| doesn't throw, but rather returns 'undefined' on error.
>        let ubr = WindowsRegistry.readRegKey(Ci.nsIWindowsRegKey.ROOT_KEY_LOCAL_MACHINE,
> +                                           WINDOWS_UBR_KEY_PATH, "UBR",
> +                                           Ci.nsIWindowsRegKey.WOW64_64);

Does this still work fine in x64 builds?

Comment 17

[Benjamin Smedberg]

Nit, it would have to be `ubr !== undefined`.

Comment 18

[Alessio Placitelli [:Dexter]]

Attachment: Part 1 - Add the build number and UBR for Windows >= 10

This also checks for the Windows major version to be >= 10.

Comment 19

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #16)
> Comment on attachment 8740902 [details] [diff] [review]
> Part 2 - Add the WOW64_64 flag support to WindowsRegistry
> 
> Review of attachment 8740902 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/TelemetryEnvironment.jsm
> @@ +1255,5 @@
> >        // Query the UBR key and only add it to the environment if it's available.
> >        // |readRegKey| doesn't throw, but rather returns 'undefined' on error.
> >        let ubr = WindowsRegistry.readRegKey(Ci.nsIWindowsRegKey.ROOT_KEY_LOCAL_MACHINE,
> > +                                           WINDOWS_UBR_KEY_PATH, "UBR",
> > +                                           Ci.nsIWindowsRegKey.WOW64_64);
> 
> Does this still work fine in x64 builds?

That's a good point. Given [1], this should not be a problem: we're saying that we want to access the 64 bit registry which is the default registry for x64 builds. Again, to be 100% sure, I guess we should manually test drive this.

[1] - https://msdn.microsoft.com/it-it/library/windows/desktop/aa384129%28v=vs.85%29.aspx

Comment 20

[Georg Fritzsche [:gfritzsche]]

(In reply to Alessio Placitelli [:Dexter] from comment #19)
> That's a good point. Given [1], this should not be a problem: we're saying
> that we want to access the 64 bit registry which is the default registry for
> x64 builds. Again, to be 100% sure, I guess we should manually test drive
> this.

Yes, let's verify that before landing.

Comment 21

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8740978 [details] [diff] [review]
Part 1 - Add the build number and UBR for Windows >= 10

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

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +43,5 @@
>  // The maximum length of a string (e.g. description) in the addons section.
>  const MAX_ADDON_STRING_LENGTH = 100;
>  
> +// The path to the "UBR" key, queried to get additional version details on Windows.
> +const WINDOWS_UBR_KEY_PATH = "SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion";

It's only used in one place below, let's move it there.

@@ +329,3 @@
>   */
> +function getWindowsVersionInfo() {
> +  const UNKNOWN_SERVICE_PACK = {major: null, minor: null, buildNumber: null};

* UNKNOWN_VERSION_INFO
* with the function renaming, we should rename major/minor to servicePackMajor/Minor

@@ +398,5 @@
> +  const majorVersionNumber = parseInt(aVersionString.split(".")[0]);
> +  if (!Number.isFinite(majorVersionNumber)) {
> +    return false;
> +  }
> +  // Only return true if the major version number is > 10.

>= 10... not sure that needs a comment though.

@@ +1272,5 @@
> +      let versionInfo = getWindowsVersionInfo();
> +      data.servicePackMajor = versionInfo.major;
> +      data.servicePackMinor = versionInfo.minor;
> +      // We only need the build number and UBR if we're at or above Windows 10.
> +      if (isVersionGreaterWindows10(data.version)) {

Let's use the nsIVersionComparator.

Comment 22

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8740902 [details] [diff] [review]
Part 2 - Add the WOW64_64 flag support to WindowsRegistry

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

::: toolkit/modules/WindowsRegistry.jsm
@@ +18,5 @@
>     * @param aKey
>     *        The key name.
> +   * @param [aRegistryView=0]
> +   *        Optionally set to nsIWindowsRegKey.WOW64_64 (or nsIWindowsRegKey.WOW64_32)
> +   *        to access a 64-bit (32-bit) key from either a 32-bit or 64-bit application.

The documentation seems to talk about "nodes", not "views" - lets use that too.
Ditto below.

Comment 23

[Alessio Placitelli [:Dexter]]

Attachment: Part 1 - Add the build number and UBR for Windows >= 10

Ah, I didn't know about the version comparator. Good to know.

Comment 24

[Alessio Placitelli [:Dexter]]

Attachment: Part 2 - Add the WOW64_64 flag support to WindowsRegistry

Comment 25

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8741432 [details] [diff] [review]
Part 1 - Add the build number and UBR for Windows >= 10

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

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +1237,5 @@
>     * @return Object containing the OS data.
>     */
>    _getOSData: function () {
> +    // The path to the "UBR" key, queried to get additional version details on Windows.
> +    const WINDOWS_UBR_KEY_PATH = "SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion";

Let's move this into the Windows specific branch.

Comment 26

[Alessio Placitelli [:Dexter]]

Attachment: Part 1 - Add the build number and UBR for Windows >= 10

Comment 27

[Alessio Placitelli [:Dexter]]

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

Comment 28

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #20)
> (In reply to Alessio Placitelli [:Dexter] from comment #19)
> > That's a good point. Given [1], this should not be a problem: we're saying
> > that we want to access the 64 bit registry which is the default registry for
> > x64 builds. Again, to be 100% sure, I guess we should manually test drive
> > this.
> 
> Yes, let's verify that before landing.

I just verified that it still works on FF x64, using the build produced by the try push: http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-a4fe5d5945396c7987f5ff5a84f86b8b7895368b/try-win64/

Both the UBR and the build number are correct.

Comment 29

[Alessio Placitelli [:Dexter]]

Attachment: Part 3 - Update the docs.

Comment 30

[Alessio Placitelli [:Dexter]]

Attachment: Part 3 - Update the docs.

Comment 31

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8741777 [details] [diff] [review]
Part 3 - Update the docs.

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

::: toolkit/components/telemetry/docs/environment.rst
@@ +108,5 @@
>              kernelVersion: <string>, // android/b2g only or null on failure
>              servicePackMajor: <number>, // windows only or null on failure
>              servicePackMinor: <number>, // windows only or null on failure
> +            windowsBuildNumber: <number>, // windows 10 only or null on failure
> +            windowsUBR: <number>, // windows 10 only or non-present on failure

This is a bit inconsistent.
Lets change this (and the code) to be "null on failure" too.

@@ +333,5 @@
> +~~
> +
> +This object contains operating system information.
> +
> +- ``name``: the name of the os.

Nit: Upper-case "OS" here and below.

@@ +335,5 @@
> +This object contains operating system information.
> +
> +- ``name``: the name of the os.
> +- ``version``: a string representing the os version.
> +- ``kernelVersion``: an android/b2g only string representing the kernel version.

"Android/B2G"

Comment 32

[Alessio Placitelli [:Dexter]]

Attachment: Part 3 - Update the docs.

Comment 33

[Alessio Placitelli [:Dexter]]

Attachment: Part 1 - Add the build number and UBR for Windows >= 10

|windowsUBR| is now null on failure.

Comment 34

[Alessio Placitelli [:Dexter]]

Attachment: Part 2 - Add the WOW64_64 flag support to WindowsRegistry

Slightly changed the test to account for |windowsUBR| being null.

Comment 35

[Alessio Placitelli [:Dexter]]

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

Comment 36

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/fx-team/rev/508d2f80ae5ec4e169825a15632a59f8c46b2171
Bug 1255472 - Collect actual major and minor version data on Windows 10. r=gfritzsche

https://hg.mozilla.org/integration/fx-team/rev/e49ca5f6fef2089584cba4b9959bc08f7313ae42
Bug 1255472 - Add support for the WOW64 mode to the WindowsRegistry. r=gfritzsche

https://hg.mozilla.org/integration/fx-team/rev/abe15212891c03d58bee30a6ec38e89bcb4abef3
Bug 1255472 - Updates the documentation. r=gfritzsche

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/508d2f80ae5e
https://hg.mozilla.org/mozilla-central/rev/e49ca5f6fef2
https://hg.mozilla.org/mozilla-central/rev/abe15212891c

Comment 38

[Ryan VanderMeulen [:RyanVM]]

(Very limited) initial data is coming in and showing sane-looking results \m/. Is this something we could safely uplift to Aurora as well?

Comment 39

[Alessio Placitelli [:Dexter]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #38)
> (Very limited) initial data is coming in and showing sane-looking results
> \m/. Is this something we could safely uplift to Aurora as well?

Awesome! I'll do some validity checks on the incoming data and proceed with the aurora uplifts tomorrow.

Comment 40

[Alessio Placitelli [:Dexter]]

I ran a notebook to sanity check the data [1], everything looks good to me. Georg, is there any other check you think we should do? If not, I'd move on with the aurora uplift.

[1] - https://gist.github.com/Dexterp37/2813698cf14ab611d980e1264449e8e3

Comment 41

[Georg Fritzsche [:gfritzsche]]

Lets also check the distribution of actual build number & UBR values (to avoid surprises later) before uplifting - those could be broken for a variety of reasons.
Otherwise this looks good.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

I had Anthony put together a crude dashboard for this. So far, so good.
nbviewer.jupyter.org/urls/s3-us-west-2.amazonaws.com/telemetry-public-analysis-2/windows-distribution/data/windows-build-distribution.ipynb

Comment 43

[Alessio Placitelli [:Dexter]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #42)
> I had Anthony put together a crude dashboard for this. So far, so good.
> nbviewer.jupyter.org/urls/s3-us-west-2.amazonaws.com/telemetry-public-
> analysis-2/windows-distribution/data/windows-build-distribution.ipynb

Oh, whoops. I updated the gist in comment 40 before reading your comment. Everything looks good in that gist as well.

Comment 44

[Alessio Placitelli [:Dexter]]

Comment on attachment 8741877 [details] [diff] [review]
Part 1 - Add the build number and UBR for Windows >= 10

Approval Request Comment
[Feature/regressing bug #]: None
[User impact if declined]: No impact, but it would be nice to have win10 version data from aurora on as it could be used to see if a cumulative update is causing a crash spike with better accuracy and to inform the decision making process about which version of Windows 10 to test in automation.
[Describe test coverage new/current, TreeHerder]: This stack of patches adds some sanity checks for the new fields to the existing xpcshell tests.
[Risks and why]: Low, the code has been out for two nightlies and was manually tested. We also performed server-side validation on the incoming nightly data to make sure nothing weird was happening.
[String/UUID change made/needed]: None

Comment 45

[Alessio Placitelli [:Dexter]]

Comment on attachment 8741878 [details] [diff] [review]
Part 2 - Add the WOW64_64 flag support to WindowsRegistry

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

Comment 46

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8741877 [details] [diff] [review]
Part 1 - Add the build number and UBR for Windows >= 10

More telemetry data on win10 builds/versions, Awesome job by the team verifying the data on Nightly (thanks!!!), Aurora47+

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/79b66a95e5ba
https://hg.mozilla.org/releases/mozilla-aurora/rev/a63292ca8432
https://hg.mozilla.org/releases/mozilla-aurora/rev/e636f660d797