Closed
Bug 1255472
Opened 9 years ago
Closed 9 years ago
Collect actual major and minor version data on Windows 10
Categories
(Toolkit :: Telemetry, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: RyanVM, Assigned: Dexter)
References
Details
(Whiteboard: [measurement:client])
Attachments
(3 files, 9 obsolete files)
(deleted),
patch
|
Dexter
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Dexter
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Dexter
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
Can we safely expect servicePackMajor and servicePackMinor to stay 0 in the future?
Comment 2•9 years ago
|
||
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.
Reporter | ||
Comment 3•9 years ago
|
||
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.
Flags: needinfo?(ryanvm)
Comment 4•9 years ago
|
||
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.
Updated•9 years ago
|
Summary: Make servicePackMajor and servicePackMinor collect useful information on Windows 10 → Collect actual major and minor version data on Windows 10
Comment 5•9 years ago
|
||
Aaron, do you happen to know if there is an API that would give us the Windows 10 version details (described in comment 0)?
Flags: needinfo?(aklotz)
Updated•9 years ago
|
Priority: P3 → P2
Comment 6•9 years ago
|
||
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.
Reporter | ||
Comment 7•9 years ago
|
||
That will presumably change when Redstone (Anniversary Update) ships and they freeze on X.
Flags: needinfo?(ryanvm)
Reporter | ||
Comment 8•9 years ago
|
||
The "Pre-release versions of Windows 10 Threshold 2" section shows the same.
Comment 9•9 years ago
|
||
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.
Flags: needinfo?(aklotz)
Comment 10•9 years ago
|
||
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
Whiteboard: [measurement:client] → [measurement:client] [lang=js]
Updated•9 years ago
|
Mentor: gfritzsche
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → alessio.placitelli
Reporter | ||
Comment 11•9 years ago
|
||
Just wanted to confirm that the UBR value was changed to 218 as expected with today's released cumulative update.
Assignee | ||
Comment 12•9 years ago
|
||
This WIP patch adds the fields and a minimal test coverage.
Assignee | ||
Comment 13•9 years ago
|
||
This patch adds the windows build number and UBR value. We force the 64-bit registry view when reading the UBR value.
Attachment #8740802 -
Attachment is obsolete: true
Attachment #8740901 -
Flags: review?(gfritzsche)
Assignee | ||
Comment 14•9 years ago
|
||
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•9 years ago
|
||
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`?
Attachment #8740901 -
Flags: review?(gfritzsche)
Comment 16•9 years ago
|
||
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•9 years ago
|
||
Nit, it would have to be `ubr !== undefined`.
Assignee | ||
Comment 18•9 years ago
|
||
This also checks for the Windows major version to be >= 10.
Attachment #8740901 -
Attachment is obsolete: true
Attachment #8740978 -
Flags: review?(gfritzsche)
Assignee | ||
Updated•9 years ago
|
Attachment #8740902 -
Attachment description: bug1255472_winregistry.patch → Part 2 - Add the WOW64_64 flag support to WindowsRegistry
Assignee | ||
Updated•9 years ago
|
Attachment #8740902 -
Flags: review?(gfritzsche)
Assignee | ||
Comment 19•9 years ago
|
||
(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•9 years ago
|
||
(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•9 years ago
|
||
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.
Attachment #8740978 -
Flags: review?(gfritzsche)
Comment 22•9 years ago
|
||
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.
Attachment #8740902 -
Flags: review?(gfritzsche) → review+
Assignee | ||
Comment 23•9 years ago
|
||
Ah, I didn't know about the version comparator. Good to know.
Attachment #8740978 -
Attachment is obsolete: true
Attachment #8741432 -
Flags: review?(gfritzsche)
Assignee | ||
Comment 24•9 years ago
|
||
Attachment #8740902 -
Attachment is obsolete: true
Attachment #8741433 -
Flags: review+
Comment 25•9 years ago
|
||
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.
Attachment #8741432 -
Flags: review?(gfritzsche) → review+
Assignee | ||
Comment 26•9 years ago
|
||
Attachment #8741432 -
Attachment is obsolete: true
Attachment #8741666 -
Flags: review+
Assignee | ||
Comment 27•9 years ago
|
||
Updated•9 years ago
|
Points: --- → 2
Priority: P2 → P1
Updated•9 years ago
|
Mentor: gfritzsche
Whiteboard: [measurement:client] [lang=js] → [measurement:client]
Assignee | ||
Comment 28•9 years ago
|
||
(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/alessio.placitelli@gmail.com-a4fe5d5945396c7987f5ff5a84f86b8b7895368b/try-win64/
Both the UBR and the build number are correct.
Status: NEW → ASSIGNED
Assignee | ||
Comment 29•9 years ago
|
||
Attachment #8741767 -
Flags: review?(gfritzsche)
Assignee | ||
Comment 30•9 years ago
|
||
Attachment #8741767 -
Attachment is obsolete: true
Attachment #8741767 -
Flags: review?(gfritzsche)
Attachment #8741777 -
Flags: review?(gfritzsche)
Comment 31•9 years ago
|
||
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"
Attachment #8741777 -
Flags: review?(gfritzsche) → review+
Assignee | ||
Comment 32•9 years ago
|
||
Attachment #8741777 -
Attachment is obsolete: true
Attachment #8741875 -
Flags: review+
Assignee | ||
Comment 33•9 years ago
|
||
|windowsUBR| is now null on failure.
Attachment #8741666 -
Attachment is obsolete: true
Attachment #8741877 -
Flags: review+
Assignee | ||
Comment 34•9 years ago
|
||
Slightly changed the test to account for |windowsUBR| being null.
Attachment #8741433 -
Attachment is obsolete: true
Attachment #8741878 -
Flags: review+
Assignee | ||
Comment 35•9 years ago
|
||
Assignee | ||
Comment 36•9 years ago
|
||
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•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/508d2f80ae5e
https://hg.mozilla.org/mozilla-central/rev/e49ca5f6fef2
https://hg.mozilla.org/mozilla-central/rev/abe15212891c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Reporter | ||
Comment 38•9 years ago
|
||
(Very limited) initial data is coming in and showing sane-looking results \m/. Is this something we could safely uplift to Aurora as well?
Assignee | ||
Comment 39•9 years ago
|
||
(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.
Assignee | ||
Comment 40•9 years ago
|
||
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
Flags: needinfo?(gfritzsche)
Comment 41•9 years ago
|
||
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.
Flags: needinfo?(gfritzsche)
Reporter | ||
Comment 42•9 years ago
|
||
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
Assignee | ||
Comment 43•9 years ago
|
||
(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.
Assignee | ||
Comment 44•9 years ago
|
||
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
Attachment #8741877 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 45•9 years ago
|
||
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]:
Attachment #8741878 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•9 years ago
|
Attachment #8741875 -
Flags: approval-mozilla-aurora?
status-firefox47:
--- → affected
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+
Attachment #8741877 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8741875 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8741878 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 47•9 years ago
|
||
bugherder uplift |
You need to log in
before you can comment on or make changes to this bug.
Description
•