Closed Bug 668436 Opened 13 years ago Closed 13 years ago

Send service pack major and minor version info in AUS ping

Categories

(Toolkit :: Application Update, defect)

Product:
Toolkit
Component:
Application Update
Platform:
All
Windows 7
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla9
Tracking Flags:
Tracking Status
firefox7 + ---
firefox9 + fixed

People

(Reporter: khuey, Assigned: TimAbraldes)

References

Details

(Keywords: qawanted, Whiteboard: [qa+])

Attachments

(1 file, 8 obsolete files)

Patch v8 - Fixed spacing, added "throw" comments, removed test for numerical service pack
10.95 KB, patch
robert.strong.bugs
: review+
akeybl
: approval-mozilla-beta+
Details | Diff | Splinter Review 
When we switch to MSVC 2010 we're going to want to exclude XP RTM and SP1 users from the update offer, which will require the ability to differentiate by service pack on AUS.
We already send the Windows major and minor version.
http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/md/windows/ntmisc.c#861

What you want are the service pack major and minor version
http://msdn.microsoft.com/en-us/library/ms724833%28v=vs.85%29.aspx

Do you know what the equivalent would be on Mac and Linux or if other data would be better for them or Windows?
Summary: Send OS minor version info in AUS ping → Send service pack major and minor version info in AUS ping
(In reply to comment #1)
> What you want are the service pack major and minor version
> http://msdn.microsoft.com/en-us/library/ms724833%28v=vs.85%29.aspx

Indeed.

> Do you know what the equivalent would be on Mac and Linux or if other data
> would be better for them or Windows?

Unfortunately I do not.
Note: this only affects Windows.

So, this is another one of those bugs where we will either need to backport to all version that we want users to be able to update to the version we compile with MSVC 2010 or have users upgrade to a specific version which has this fix and then only allow users to update to versions with this fix.

As I understand it, we are hoping to release Firefox 8 compiled with MSVC 2010 so this needs to track Firefox 7 if we go with requiring updating to Firefox 7 before they can update to Firefox 8.

Drivers, if you want the ability to update to Firefox 8 from versions prior to Firefox 7 then this will need release vehicles on those prior versions. Please let me know if that is what you want AND the release vehicles for the yet to be created patch otherwise I am going to go with just fixing this for Firefox 7.
tracking-firefox7: --- → ?
OS: All → Windows 7
Drivers, another option is to just require XP users to update to Firefox 7 since that is the only Windows version we need this additional info for.
Since there seems to be concern for XP SP#....
What are the implications for Win2000 users when moving to MSVC 2010
(In reply to comment #5)
> Since there seems to be concern for XP SP#....
> What are the implications for Win2000 users when moving to MSVC 2010

Windows 2000 will not be supported either. See bug 563318 comment 12 through 14.
Assignee: nobody → tabraldes
This code appends the service pack info if it was successfully retrieved through a call to GetVersionExW. On my machine (Windows 7, SP1), it results in this version string:
  Windows_NT 6.1.1.0

Is there other info we might want from the OSVERSIONINFOEXW struct?  The info it has available is documented here:
  http://msdn.microsoft.com/en-us/library/ms724833%28v=vs.85%29.aspx

Also, as a side note, this code doesn't seem to run on my machine unless I comment out the #ifdef XP_WIN, even though I'm building/running on Windows.
Attachment #551157 - Flags: feedback?(robert.bugzilla)
Attachment #551157 - Flags: feedback?(khuey)
Status: NEW → ASSIGNED
Comment on attachment 551157 [details] [diff] [review]
Append service pack info gathered by calling GetVersionExW through js-ctypes

Looks good.

># HG changeset patch
># Parent c7931e07dd4dcda4916d58753bfdb933372e8148
># User Tim Abraldes <tabraldes@mozilla.com>
>Bug 668436 - Send service pack major and minor version info in AUS ping; r=rs
>
>diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js
>--- a/toolkit/mozapps/update/nsUpdateService.js
>+++ b/toolkit/mozapps/update/nsUpdateService.js
>@@ -41,16 +41,17 @@
> # the terms of any one of the MPL, the GPL or the LGPL.
> #
> # ***** END LICENSE BLOCK ***** */
> */
> Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> Components.utils.import("resource://gre/modules/FileUtils.jsm");
> Components.utils.import("resource://gre/modules/AddonManager.jsm");
> Components.utils.import("resource://gre/modules/Services.jsm");
>+Components.utils.import("resource://gre/modules/ctypes.jsm")
> 
> const Cc = Components.classes;
> const Ci = Components.interfaces;
> const Cr = Components.results;
> 
> const UPDATESERVICE_CID = Components.ID("{B3C290A6-3943-4B89-8BBE-C01EB7B3B311}");
> const UPDATESERVICE_CONTRACTID = "@mozilla.org/updates/update-service;1";
> 
>@@ -193,16 +194,62 @@ XPCOMUtils.defineLazyGetter(this, "gOSVe
>   try {
>     osVersion = sysInfo.getProperty("name") + " " + sysInfo.getProperty("version");
>   }
>   catch (e) {
>     LOG("gOSVersion - OS Version unknown: updates are not possible.");
>   }
> 
>   if (osVersion) {
>+    var servicePackInfo = "";
not used

>+
>+#ifdef XP_WIN
>+    const BYTE = ctypes.uint8_t;
>+    const WORD = ctypes.uint16_t;
>+    const DWORD = ctypes.uint32_t;
>+    const WCHAR = ctypes.jschar;
>+    // TODO: `BOOL` is a typedef to `int` so
>+    // this will be wrong on machines with different `int` sizes
>+    const BOOL = ctypes.uint32_t; 
nit: extra space at end of line

>+
>+    // This structure described at:
nit: This structure *is* described at:

>+    // http://msdn.microsoft.com/en-us/library/ms724833%28v=vs.85%29.aspx
>+    const SZCSDVERSIONLENGTH = 128;
>+    const OSVERSIONINFOEXW = new ctypes.StructType('OSVERSIONINFOEXW',
>+        [
>+        {dwOSVersionInfoSize: DWORD},
>+        {dwMajorVersion: DWORD},
>+        {dwMinorVersion: DWORD},
>+        {dwBuildNumber: DWORD},
>+        {dwPlatformId: DWORD},
>+        {szCSDVersion: ctypes.ArrayType(WCHAR, SZCSDVERSIONLENGTH)},
>+        {wServicePackMajor: WORD},
>+        {wServicePackMinor: WORD},
>+        {wSuiteMask: WORD},
>+        {wProductType: BYTE},
>+        {wReserved: BYTE}
>+        ]);
>+
>+    var kernel32 = ctypes.open("Kernel32");
>+    try {
>+      var GetVersionEx = kernel32.declare("GetVersionExW",
>+          ctypes.winapi_abi,
This needs to be ctypes.default_abi so it will also work on Win64... I verified this change returns the correct value on my Win7 x64 system with both 32 and 64 bit Firefox.
http://mxr.mozilla.org/mozilla-central/source/js/src/ctypes/CTypes.cpp#4547

>+          BOOL,
>+          OSVERSIONINFOEXW.ptr);
nit: the prevailing style in this file lines up the params like so here and elsewhere... also use let though it isn't used all that much yet in this file.
let GetVersionEx = kernel32.declare("GetVersionExW", ctypes.winapi_abi,
                                    BOOL, OSVERSIONINFOEXW.ptr);


>+      var winVer = OSVERSIONINFOEXW();
>+      winVer.dwOSVersionInfoSize = OSVERSIONINFOEXW.size;
>+
>+      if(0 !== GetVersionEx(winVer.address())) {
>+        osVersion += "." + winVer.wServicePackMajor
>+          + "." + winVer.wServicePackMinor;
>+      }
needs a catch and I think we want to send .unknown when this fails as follows... I'll verify that with releng since they maintain the AUS server.
} catch (e) {
  LOG("gOSVersion - error getting service pack information. Exception: " + e);
  osVersion += ".unknown";

>+    } finally {
>+      kernel32.close();
>+    }
>+#endif
>     try {
>       osVersion += " (" + sysInfo.getProperty("secondaryLibrary") + ")";
>     }
>     catch (e) {
>       // Not all platforms have a secondary widget library, so an error is nothing to worry about.
>     }
>     osVersion = encodeURIComponent(osVersion);
>   }
Attachment #551157 - Flags: feedback?(robert.bugzilla) → feedback+
nthomas, what do you think about sending .unknown in the version part of the url when the service pack major and minor can't be retrieved?
Forgot to mention that with optimized builds you need to launch with -purgecaches to get Firefox to reread the changed nsUpdateService.js which might be why you weren't seeing the changes take affect. There are other ways to force the cache to purge such as running with the same profile and a different install (it checks the path) and deleting the compatibility.ini from the profile... there are several others.
Updated the patch with Rob's feedback comments.  I'll update again and mark for review when we hear back from releng about ".unknown" in the version string.

Rob - Thanks for the "-purgecaches" advice!
Attachment #551157 - Attachment is obsolete: true
Attachment #551157 - Flags: feedback?(khuey)
(In reply to Robert Strong [:rstrong] (do not email) from comment #9)

I think 'Windows_NT X.Y.unknown' should be OK, and at least give us a clue if the version lookup is failing in the wild. 

Kyle, will we want to block OS versions something like this ?
  Windows_NT 5.0.*        (Win2k ?)
  Windows_NT 5.1.0.*      (XP RTM?)
  Windows_NT 5.1.1.*      (XP SP1?)

What about WinXP 64bit ? That's 5.2 apparently, but so are assorted Window Server 2003 and Home products, according to table in comment #2's MSDN page. Might need to send extra info ?
(In reply to Nick Thomas [:nthomas] from comment #12)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #9)
> 
> I think 'Windows_NT X.Y.unknown' should be OK, and at least give us a clue
> if the version lookup is failing in the wild. 
> 
> Kyle, will we want to block OS versions something like this ?
>   Windows_NT 5.0.*        (Win2k ?)
>   Windows_NT 5.1.0.*      (XP RTM?)
>   Windows_NT 5.1.1.*      (XP SP1?)

Yes.

> What about WinXP 64bit ? That's 5.2 apparently, but so are assorted Window
> Server 2003 and Home products, according to table in comment #2's MSDN page.
> Might need to send extra info ?

I'm not sure, the MSDN page doesn't say that it's supported.  Not sure how we'll handle that :-/
The table in the MSDN page says that you can differentiate WinXP64 by looking for version 5.2 and this:
   (OSVERSIONINFOEX.wProductType == VER_NT_WORKSTATION)
   && (SYSTEM_INFO.wProcessorArchitecture==PROCESSOR_ARCHITECTURE_AMD64)

We can use js-ctypes to call GetSystemInfo or GetNativeSystemInfo and pass up a string based on the wProcessorArchitecture field of the SYSTEM_INFO struct: http://msdn.microsoft.com/en-us/library/ms724958%28v=vs.85%29.aspx

The attached patch (which would need a little cleanup before review) uses GetSystemInfo, so it will return "x86" for processess running under WOW64 (e.g. 32-bit firefox on 64-bit Windows), but we could use GetNativeSystemInfo instead which only cares about the OS.  Are we more interested in finding out whether the user is running 64-bit Windows or 64-bit Firefox (or both)?
Attachment #552272 - Attachment is obsolete: true
We should know from the %BUILD_TARGET% part of the query what arch of Firefox is running (WINNT_x86-msvc vs WINNT_x86_64-msvc). The %OS_VERSION% part only needs to handle the OS itself.
On my system, this patch produces:
  Windows_NT 6.1.1.0(x64)

If there's an error, it would produce something like:
  Windows_NT 6.1.1.0(unknown)
  Windows_NT 6.1.unknown(x86)
  etc.

Does this seem to fulfill our needs?
Attachment #553999 - Attachment is obsolete: true
Would be fine as is, but you could add a space for a bit more readability, eg  
  Windows_NT 6.1.1.0 (x64)
  Windows_NT 6.1.1.0 (unknown)
  Windows_NT 6.1.unknown.unknown (x86)
Great!  Sounds like we're ready for review.
Attachment #554146 - Attachment is obsolete: true
Attachment #556131 - Flags: review?(robert.bugzilla)
Comment on attachment 556131 [details] [diff] [review]
Patch v5 - Added an extra space for readability.

Kind of swamped and I'd like to get another set of eyes on this so requesting feedback-r from Brian
Attachment #556131 - Flags: feedback?(netzen)
Nice work, it looks good but I do have some suggestions.
This is just some feedback, please wait for Robert's word before actually making any changes (since my comments are non trivial changes and I'm not a peer in updates yet):

I don't think we should use ctypes at all in the jsm for 3 reasons:
1) The code would be cleaner / smaller / safer without it
2) The info may need to be reused somewhere else some other time.
3) We may need a non Win32 service pack versions in the future for some other platform so we could have consistent jsm code for all platforms if this is ever needed.

Instead you could add a #ifdef XP_WIN inside <mozilla>/xpcom/base/nsSystemInfo.cpp.  
Inside there, similar to how android does it, you would set an extra SetPropertyAsAString and call GetVersionExW from there.
That way you don't have to deal with abi/struct sizes, dynamically loading, etc.

GetNativeSystemInfo is not available on Windows 2000 so if you use it you'll have to load it dynamically.
You can also use IsWow64Process() || sizeof(int*) == 8 to check if you have an x64 computer.  (IsWow64Process also needs to be loaded dynamically since it's not avail on 2k either).
Win2k is never 64bit though so you can assume that it is not 64bit in that case. 

The jsm code would then just need to use sysInfo.getProperty("servicepack") and append if it contains something.

If we don't do my suggestion, here are some review notes about the current jsm code: 
- I have not tested the code myself yet since if we do my suggestion above it will need to be retested.
- I think we can remove the TODO comment and use ctypes.int
- This code does not check for GetNativeSystemInfo not being available (it is not on Win2k).  You can assume if it's not defined it will be x86.
Attachment #556131 - Flags: feedback?(netzen)
(In reply to Brian R. Bondy [:bbondy] from comment #21)
> Nice work, it looks good but I do have some suggestions.

Thanks! :)

> This is just some feedback, please wait for Robert's word before actually
> making any changes (since my comments are non trivial changes and I'm not a
> peer in updates yet):
> 

OK, on standby until Robert comments.
Discussed offline with Robert; he likes the SetPropertyAsAString/getProperty approach but wants to push ahead with an implementation using ctypes.  My overly-simplified notes about the rationale:
    We're trying to move away from XPCOM in general
    Extension developers are being pushed to use ctypes; we should use ctypes as well

Robert also mentioned that he'd like us to send up "unknown" architecture if GetNativeSystemInfo is non-existent (e.g. on Win2K) since it more accurately reflects the fact that we weren't able to determine the architecture.  I think that's what the current code does but I'll have to find a Win2K box to be sure.

For the next patch, I'll address the following:
    Remove the TODO comment and use ctypes.int
    Modify toolkit/mozapps/update/test/unit/test_0040_general.js to test that service pack and architecture are being sent correctly
    Test on Win2K somehow...
QA can probably find you a Win2k machine to test on.
Perhaps, last time I needed XP I wasn't able to get a system from QA so I doubt it will be any easier to get a Win2K system from them. I'll find one somewhere though so no worries.
> Test on Win2K somehow

Another thing you could try is simply changing the functions that aren't supported to different names temporarily.  That will simulate those functions not being available. 

Like:

> let GetNativeSystemInfo = kernel32.declare("GetNativeSystemInfoZZZZZ",
> ...
Another optional suggestion is to make a couple functions to encapsulate this logic.  Something along the lines of getServicePackInfo() and getArchitecture().
By the way surprisingly I found out that Windows 2000 does support an x64 version.  It was codenamed "Janus" and WOW64 even ran on it so we probably work on it.  I always thought it was only XP and up for x64 support.  I'm sure it's extremely rare but this shows that rs was right about what to do there :)

> Starting with Windows 2000, Microsoft began making 64-bit versions of Windows
> available—before this, these operating systems only existed in 32-bit versions.

Reference: 
http://en.wikipedia.org/wiki/Architecture_of_Windows_NT

I'm sure you'd be able to get the info with the environment variable PROCESSOR_ARCHITECTURE in that case, but it's not worth the work and better to return unknown.
:%s/x64/IA64/g (IA64 is diff from AMD64, but I was just surprised in general at 64-bit support)
(In reply to Brian R. Bondy [:bbondy] from comment #27)
> Another optional suggestion is to make a couple functions to encapsulate
> this logic.  Something along the lines of getServicePackInfo() and
> getArchitecture().

(In reply to Brian R. Bondy [:bbondy] from comment #26)
> > Test on Win2K somehow
> 
> Another thing you could try is simply changing the functions that aren't
> supported to different names temporarily.  That will simulate those
> functions not being available. 
> 
> Like:
> 
> > let GetNativeSystemInfo = kernel32.declare("GetNativeSystemInfoZZZZZ",
> > ...

I like both of these ideas! I'll post back soon with an updated patch and my findings from the "GetNativeSystemInfo doesn't exist" testing
(In reply to Tim Abraldes from comment #30)
> I like both of these ideas! I'll post back soon with an updated patch and my
> findings from the "GetNativeSystemInfo doesn't exist" testing

Testing was successful:
Appending "zzz" to the end of the function names resulted in a version string of "Windows_NT 6.1.unknown (unknown)"

I started separating the code into getServicePackInfo() and getArchitecture() but we lose a bunch of code reuse like the kernel32 object and the various convenience types (BYTE, WORD, BOOL, etc).  We could pass the kernel32 object in to those functions, combine them into one getServicePackAndArch() function, or open kernel32 in both functions, but all of those options seemed worse than what's currently written.  Should we leave as is?
I think it's fine as is, whatever is easiest is probably fine.
I'll run this through try server, but wanted to post what I've got.  Brian, can you take a look and let me know if you see any glaring issues?
Attachment #556131 - Attachment is obsolete: true
Attachment #565581 - Flags: review?(robert.bugzilla)
Attachment #565581 - Flags: feedback?(netzen)
Attachment #556131 - Flags: review?(robert.bugzilla)
Found this immediately after posting.  Let me know if you find any _other_ glaring issues :)
Attachment #565581 - Attachment is obsolete: true
Attachment #565590 - Flags: feedback?(netzen)
Attachment #565581 - Flags: review?(robert.bugzilla)
Attachment #565581 - Flags: feedback?(netzen)
Try run for 8df53d57ff99 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=8df53d57ff99
Results (out of 9 total builds):
    success: 9
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tabraldes@mozilla.com-8df53d57ff99
Comment on attachment 565590 [details] [diff] [review]
Patch v6 - Fixed uncaught exception in unit test

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

Looking great.  
Mainly just have an else clause for GetVersionEx in case it returns 0 and other minor formatting nits.
Added some optional test feedback too.

Also I think you'll want to run through try with all options:
"try: -b do -p all -u all -t all"

::: toolkit/mozapps/update/nsUpdateService.js
@@ +240,5 @@
> +        {wProcessorLevel: WORD},
> +        {wProcessorRevision: WORD}
> +        ]);
> +
> +    let kernel32 = ctypes.open("Kernel32");

I know it is not likely, but you never know with things like Windows 8 coming out.  If Kernel32 didn't exist then it would throw. as well.

@@ +247,5 @@
> +      try {
> +        let GetVersionEx = kernel32.declare("GetVersionExW",
> +            ctypes.default_abi,
> +            BOOL,
> +            OSVERSIONINFOEXW.ptr);

Continued lines should be aligned to one whitespace after the open parentheses.

@@ +253,5 @@
> +        winVer.dwOSVersionInfoSize = OSVERSIONINFOEXW.size;
> +
> +        if(0 !== GetVersionEx(winVer.address())) {
> +          osVersion += "." + winVer.wServicePackMajor
> +            + "." + winVer.wServicePackMinor;

+ symbol should be on the previous line.
"." should be aligned with the previous "."

@@ +254,5 @@
> +
> +        if(0 !== GetVersionEx(winVer.address())) {
> +          osVersion += "." + winVer.wServicePackMajor
> +            + "." + winVer.wServicePackMinor;
> +        }

If GetVersionEx returns 0 (FALSE) then we should also append ".unknown".  Add else clause.

@@ +266,5 @@
> +      try {
> +        let GetNativeSystemInfo = kernel32.declare("GetNativeSystemInfo",
> +            ctypes.default_abi,
> +            ctypes.void_t,
> +            SYSTEM_INFO.ptr);

Align subsequent lines.

::: toolkit/mozapps/update/test/unit/test_0040_general.js
@@ +254,5 @@
> +        {wProductType: BYTE},
> +        {wReserved: BYTE}
> +        ]);
> +
> +    let kernel32 = ctypes.open("Kernel32");

ctypes.open could throw for Kernel32 on some alternate universe.

@@ +260,5 @@
> +        let GetVersionEx = kernel32.declare("GetVersionExW",
> +            ctypes.default_abi,
> +            BOOL,
> +            OSVERSIONINFOEXW.ptr);
> +        let winVer = OSVERSIONINFOEXW();

Formatting

@@ +299,5 @@
> +    try {
> +      let GetNativeSystemInfo = kernel32.declare("GetNativeSystemInfo",
> +          ctypes.default_abi,
> +          ctypes.void_t,
> +          SYSTEM_INFO.ptr);

Formatting.

@@ +349,5 @@
> +        do_throw("Failed to obtain processor architecture: " + e);
> +      }
> +    }
> +  }
> +

Maybe we should check that the actual osVersion string matches the platform that the test is running on as well.
Attachment #565590 - Flags: feedback?(netzen) → feedback+
Thanks for the feedback, Brian!

(In reply to Brian R. Bondy [:bbondy] from comment #36)
> ::: toolkit/mozapps/update/test/unit/test_0040_general.js
> @@ +254,5 @@
> > +        {wProductType: BYTE},
> > +        {wReserved: BYTE}
> > +        ]);
> > +
> > +    let kernel32 = ctypes.open("Kernel32");
> 
> ctypes.open could throw for Kernel32 on some alternate universe.

Fixed this in the updater code, but I think it's OK here in the test because we wrap calls to getServicePack() and getProcArchitecture() in try/catch blocks and do_throw() the result if there was an exception (e.g. in ctypes.open()).  I tested by replacing the "kernel32" string with "kernel32zz" and the test failed with a descriptive error message.

> @@ +349,5 @@
> > +        do_throw("Failed to obtain processor architecture: " + e);
> > +      }
> > +    }
> > +  }
> > +
> 
> Maybe we should check that the actual osVersion string matches the platform
> that the test is running on as well.

I'm not sure what you mean here, could you clarify?  We test the osVersion string against the URL created with "%OS_VERSION%" at the end of check_test_pt9() with the line "do_check_eq(getResult(gRequestURL), osVersion);" but I suspect you're talking about something else.

I think I've addressed all the other feedback; please let me know if I missed anything.  Thanks again for checking this out!
Attachment #565590 - Attachment is obsolete: true
> We test the osVersion string against the URL created with "%OS_VERSION%" at the end of check_test_pt9

OK great that's what I meant, and the other changes look good.
You can proceed to review whenever you're ready.
Try run for 43a6d70190aa is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=43a6d70190aa
Results (out of 229 total builds):
    exception: 1
    success: 217
    warnings: 10
    failure: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tabraldes@mozilla.com-43a6d70190aa
Comment on attachment 566055 [details] [diff] [review]
Patch v7 - Incorporated feedback comments

Try run looks good, marking for review
Attachment #566055 - Flags: review?(robert.bugzilla)
Comment on attachment 566055 [details] [diff] [review]
Patch v7 - Incorporated feedback comments

I'm going to verify that all is well on x64 before r+'ing the patch but there is something weird going on with my x64 build so I need to clobber it. For now a couple of nits.

>diff --git a/toolkit/mozapps/update/test/unit/test_0040_general.js b/toolkit/mozapps/update/test/unit/test_0040_general.js
>--- a/toolkit/mozapps/update/test/unit/test_0040_general.js
>+++ b/toolkit/mozapps/update/test/unit/test_0040_general.js
>...
>@@ -223,22 +225,137 @@ function check_test_pt8() {
> function run_test_pt9() {
>   gCheckFunc = check_test_pt9;
>   var url = URL_PREFIX + "%OS_VERSION%/";
>   logTestInfo("testing url constructed with %OS_VERSION% - " + url);
>   setUpdateURLOverride(url);
>   gUpdateChecker.checkForUpdates(updateCheckListener, true);
> }
> 
>+function getServicePack() {
>+    const BYTE = ctypes.uint8_t;
>+    const WORD = ctypes.uint16_t;
>+    const DWORD = ctypes.uint32_t;
>+    const WCHAR = ctypes.jschar;
>+    const BOOL = ctypes.int;
>+
>+    // This structure is described at:
>+    // http://msdn.microsoft.com/en-us/library/ms724833%28v=vs.85%29.aspx
>+    const SZCSDVERSIONLENGTH = 128;
>+    const OSVERSIONINFOEXW = new ctypes.StructType('OSVERSIONINFOEXW',
>+        [
>+        {dwOSVersionInfoSize: DWORD},
>+        {dwMajorVersion: DWORD},
>+        {dwMinorVersion: DWORD},
>+        {dwBuildNumber: DWORD},
>+        {dwPlatformId: DWORD},
>+        {szCSDVersion: ctypes.ArrayType(WCHAR, SZCSDVERSIONLENGTH)},
>+        {wServicePackMajor: WORD},
>+        {wServicePackMinor: WORD},
>+        {wSuiteMask: WORD},
>+        {wProductType: BYTE},
>+        {wReserved: BYTE}
>+        ]);
>+
>+    let kernel32 = ctypes.open("kernel32");
>+    try {
>+      let GetVersionEx = kernel32.declare("GetVersionExW",
>+                                          ctypes.default_abi,
>+                                          BOOL,
>+                                          OSVERSIONINFOEXW.ptr);
>+      let winVer = OSVERSIONINFOEXW();
>+      winVer.dwOSVersionInfoSize = OSVERSIONINFOEXW.size;
>+
>+      if(0 === GetVersionEx(winVer.address())) {
>+        throw ("Failure in GetVersionEx (returned 0)");
I see you used do_throw in a couple of other places and in xpcshell tests we typically use do_throw. Is there a reason not to use do_throw?
Remove the extra space between throw and (

>+      }
>+
>+      return winVer.wServicePackMajor + "." + winVer.wServicePackMinor;
>+    } finally {
>+      kernel32.close();
>+    }
>+}
>+
>+function getProcArchitecture() {
>+    const WORD = ctypes.uint16_t;
>+    const DWORD = ctypes.uint32_t;
>+
>+    // This structure is described at:
>+    // http://msdn.microsoft.com/en-us/library/ms724958%28v=vs.85%29.aspx
>+    const SYSTEM_INFO = new ctypes.StructType('SYSTEM_INFO',
>+        [
>+        {wProcessorArchitecture: WORD},
>+        {wReserved: WORD},
>+        {dwPageSize: DWORD},
>+        {lpMinimumApplicationAddress: ctypes.voidptr_t},
>+        {lpMaximumApplicationAddress: ctypes.voidptr_t},
>+        {dwActiveProcessorMask: DWORD.ptr},
>+        {dwNumberOfProcessors: DWORD},
>+        {dwProcessorType: DWORD},
>+        {dwAllocationGranularity: DWORD},
>+        {wProcessorLevel: WORD},
>+        {wProcessorRevision: WORD}
>+        ]);
>+
>+    let kernel32 = ctypes.open("kernel32");
>+    try {
>+      let GetNativeSystemInfo = kernel32.declare("GetNativeSystemInfo",
>+                                                 ctypes.default_abi,
>+                                                 ctypes.void_t,
>+                                                 SYSTEM_INFO.ptr);
>+      let sysInfo = SYSTEM_INFO();
>+      // Default to unknown
>+      sysInfo.wProcessorArchitecture = 0xffff;
>+
>+      GetNativeSystemInfo(sysInfo.address());
>+      switch(sysInfo.wProcessorArchitecture) {
>+        case 9:
>+          return "x64";
>+        case 6:
>+          return "IA64";
>+        case 0:
>+          return "x86";
>+        default:
>+          throw("Unknown architecture returned from GetNativeSystemInfo: " + sysInfo.wProcessorArchitecture);
do_throw here as well

>+      }
>+    } finally {
>+      kernel32.close();
>+    }
>+}
>+
>+
> function check_test_pt9() {
>   var osVersion;
>   var sysInfo = AUS_Cc["@mozilla.org/system-info;1"].
>                 getService(AUS_Ci.nsIPropertyBag2);
>   osVersion = sysInfo.getProperty("name") + " " + sysInfo.getProperty("version");
> 
>+  if(IS_WIN) {
>+    try {
>+      let servicePack = getServicePack();
>+      if(/^\d+\.\d+$/.test(servicePack)) {
>+        osVersion += "." + servicePack;
>+      } else {
>+        throw("Got unreasonable value \"" + servicePack + "\"");
do_throw here as well

>+      }
>+    } catch (e) {
>+      do_throw("Failure obtaining service pack: " + e);
>+    }
>+
>+    if("5.0" === sysInfo.getProperty("version")) { // Win2K
>+      osVersion += " (unknown)";
>+    } else {
>+      try {
>+        osVersion += " (" + getProcArchitecture() + ")";
>+      } catch (e) {
>+        do_throw("Failed to obtain processor architecture: " + e);
>+      }
>+    }
>+  }
>+
>   if (osVersion) {
>     try {
>       osVersion += " (" + sysInfo.getProperty("secondaryLibrary") + ")";
>     }
>     catch (e) {
>       // Not all platforms have a secondary widget library, so an error is
>       // nothing to worry about.
>     }
(In reply to Robert Strong [:rstrong] (do not email) from comment #41)
> Comment on attachment 566055 [details] [diff] [review] [diff] [details] [review]
> Patch v7 - Incorporated feedback comments
> >+      if(0 === GetVersionEx(winVer.address())) {
> >+        throw ("Failure in GetVersionEx (returned 0)");
> I see you used do_throw in a couple of other places and in xpcshell tests we
> typically use do_throw. Is there a reason not to use do_throw?
> Remove the extra space between throw and (

I used "throw" in helper functions (getServicePack(), getProcArchitecture()).  In the actual test, I wrapped calls to those helper functions in try/catch blocks that "do_throw" any exceptions they encounter.  My thinking was that, by not using "do_throw" in the helper functions, I could avoid double-calling "do_throw" (though I'm not sure there's actually any issue with double-calling) and make the helper functions more general-purpose (e.g. if we ever have a test for which an exception from one of those helpers is expected, it won't fail the test by calling "do_throw" from the helper).  These benefits are debatable and the combination of "throw" and "do_throw" probably adds unnecessary maintenance complexity, so I'll switch to using "do_throw" everywhere.
After offline discussion with rs and some further investigation, I'm commenting the use of "throw" in the xpcshell test helpers instead of replacing with "do_throw"

Removing the test for whether getServicePack() returned a string in the form "number.number" since no other return is possible and the extra "throw" is unnecessarily confusing.

Also fixing a bunch of whitespace issues that somehow crept into the xpcshell test.
Attachment #566055 - Attachment is obsolete: true
Attachment #566970 - Flags: review?(robert.bugzilla)
Attachment #566055 - Flags: review?(robert.bugzilla)
Comment on attachment 566970 [details] [diff] [review]
Patch v8 - Fixed spacing, added "throw" comments, removed test for numerical service pack

I thought I r+'d this last week. :(

Please check with Nick Thomas that this is ok to land on m-c before landing.
Attachment #566970 - Flags: review?(robert.bugzilla) → review+
Could I get a simple before/after of the URL to see whether it will impact our processing of the AUS log data?
I believe the "before" looked like this:
Windows_NT 6.1

"After" can look like one of the following:
Windows_NT 6.1.1.0 (x64)
Windows_NT 6.1.unknown (x64)
Windows_NT 6.1.1.0 (unknown)
Windows_NT 6.1.unknown (unknown)

There's also a "secondary library" that can appear in parentheses after all other info in both the "before" and "after" cases
Okay, if the addition is still contained in that one field (i.e. no new slashes in the URL) then there won't be any problems on the Metrics side.  Base on comment #46, that seems to be the case.
AUSv2 (the current update server) will be fine too. It's splitting the query using /, and then removing leading/trailing whitespace.
Nick, will updates just work or will something need to be done on the server side to point clients to the correct mar with the new format?
Adding checkin-needed
Whiteboard: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fb14bafdd252
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Comment on attachment 566970 [details] [diff] [review]
Patch v8 - Fixed spacing, added "throw" comments, removed test for numerical service pack

The patch has been on the Firefox 10 train for quite some time so I am fairly comfortable with landing it for Firefox 9. The value is that it will provide the data we want in the AUS ping for determining the number of users on Windows XP and Windows XP SP1 so we can determine the number of users impacted by switching to MSVC 2010 (bug 515492).
Attachment #566970 - Flags: approval-mozilla-beta?
Attachment #566970 - Flags: approval-mozilla-aurora?
Comment on attachment 566970 [details] [diff] [review]
Patch v8 - Fixed spacing, added "throw" comments, removed test for numerical service pack

bah... don't need this on Aurora since it is already there.
Attachment #566970 - Flags: approval-mozilla-aurora?
With Rob's confidence here, I'd like to chime in, strongly advocating moving this forward to Beta so that the Product team can sooner decide about unsupporting win2K and older XP versions which are holding back other important work.
FWIW, I took a look at the AUS metrics for the current Aurora and Nightly builds. The changes from this bug look like they're working well.

The number of WinXP RTM and SP1 users (AIUI 5.1.0*, 5.1.1*) is small: < 150 pings per day for Aurora. Win2000 is in the same ballpark. Note that I looked at data for 2011-12-02, and the counts are higher than ADU because we ping up to every 8 hours on Aurora and every 2 hours on Nightly (but not by a factor of 3 or 12 because it depends how long the app gets used for).

As you'd expect there are many more XP SP2 and particularly SP3 users, some Vista (6.0.*) users, a majority on Win7 (6.1*), and some on Win8. There is one example of unknown showing up, which is '5.0.4.0 (unknown)' with about ~150 pings, but given that all of Win2000 will be deprecated it doesn't seem important we don't know the CPU architecture in that case.

-> VERIFIED
Status: RESOLVED → VERIFIED
Keywords: qawanted
Comment on attachment 566970 [details] [diff] [review]
Patch v8 - Fixed spacing, added "throw" comments, removed test for numerical service pack

[Triage Comment]
Approving for Beta as long as this lands today before we go to build (evening PST 12/6). I'll be getting in contact with Metrics to look for any inconsistencies in update ping trends over the next week.
Attachment #566970 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [qa+]
Pushed to mozilla-beta
https://hg.mozilla.org/releases/mozilla-beta/rev/b696da9cda6e
status-firefox9: --- → fixed
Target Milestone: mozilla10 → mozilla9
Flags: in-testsuite+
(In reply to Nick Thomas [:nthomas] from comment #57)
> FWIW, I took a look at the AUS metrics for the current Aurora and Nightly
> builds. The changes from this bug look like they're working well.
> 
> The number of WinXP RTM and SP1 users (AIUI 5.1.0*, 5.1.1*) is small: < 150
> pings per day for Aurora. Win2000 is in the same ballpark. Note that I
> looked at data for 2011-12-02, and the counts are higher than ADU because we
> ping up to every 8 hours on Aurora and every 2 hours on Nightly (but not by
> a factor of 3 or 12 because it depends how long the app gets used for).
> 
> As you'd expect there are many more XP SP2 and particularly SP3 users, some
> Vista (6.0.*) users, a majority on Win7 (6.1*), and some on Win8. There is
> one example of unknown showing up, which is '5.0.4.0 (unknown)' with about
> ~150 pings, but given that all of Win2000 will be deprecated it doesn't seem
> important we don't know the CPU architecture in that case.
> 
> -> VERIFIED

Nick, where were you looking at this?  Metrics dashboards or something else?
I would have started from an old update ping dashboard (or maybe started a blank Analysis View) and used the Operating Systems dimension with some version and channel restrictions.
Do we need to backport this to 3.6 in some way so that people on our last 3.6.x version and Win2K will not be offered updates to Firefox 13 when it ships?
The plan I discussed with LegNeato and got agreement to is that we would require updating to a version that contains this patch.
(In reply to Robert Strong [:rstrong] (do not email) from comment #63)
> The plan I discussed with LegNeato and got agreement to is that we would
> require updating to a version that contains this patch.

3.6 is being EOL'd on 4/24, which lines up with the final Win2K-compatible version of Firefox. Presumably that means that we'd have to always push 3.6 users to FF12. Does that line up with your previous conversation with Christian?
In general yes (we didn't discuss specific versions). Technically we can update them to Firefox 9, 10, 11, or 12 since they all contain this fix before updating them to a version above 12. In practice I think it would be simpler to require updating to 12 otherwise releng will need to setup different paths in AUS so Firefox 9, 10, and 11 on WinXP SP1 and below get updates to Firefox 12 and Firefox 9, 10, and 11 on WinXP SP2 and above would get updates to the latest which could be the previous versions as well as Firefox 12 or greater.
So we're offering 10.0 now, and in time will offer 11 and then 12. Given we can't offer 13 we have to re-offer 12 with a faked version (eg 12.0.1), if we want to reach the maximum number of users. I don't recall if the fake version shows up in update dialog or not, but we've used this technique in the past. It's a relatively simple post-processing of the snippets for us. We can keep doing this until we have driven the number of 3.6 users to a low enough level (have to in fact, given users won't get reprompted if they say 'No Thanks' until the version offered is higher).

The update server can't serve different builds based OS of the request as described in comment #65, just the build target. This will change for the better in Balrog though.
Depends on: 1358552
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: