811573 - Add more data points to the metrics ping for the stub installer

Status: VERIFIED FIXED

(Firefox :: Installer, defect)

Description

[Robert Strong (they/them - no direct email)]

Additional data points I think I would like:

Server url that the stub downloaded the install from
Install into default location - 1 or 0
Windows version
32 or 64 bit OS
Number of retries
Seconds to download for the last download
Seconds from start of download to install finish

Comment 1

[Robert Strong (they/them - no direct email)]

Let's go with wanted stub=

Comment 2

[Robert Strong (they/them - no direct email)]

Data points that I have implemented (still needs a little cleanup though)

Stub URL Version = v5
Build Channel = The build channel. Expected values for our builds:
nightly
aurora
beta
release
Update Channel = The application update channel. Expected values for our builds:
nightly
aurora
beta
release
Locale = the Firefox locale that was chosen to install (e.g. en-US)
Firefox x64 = 1 if installing Firefox 64 bit and 0 otherwise (e.g. 32 bit Firefox)
Running x64 Windows = 1 if running Windows 64 bit and 0 otherwise (e.g. 32 bit Windows)
Windows Major Version = self-explanatory (e.g. 6 = Windows 7)
Minor = self-explanatory (e.g. 1 = current Windows 7)
Build = self-explanatory (e.g. 7601 = current Windows 7)
Exit Code = the stub installer's exit code. Possible values:
0 = successful install

The following apply to the download phase:
10 = download cancelled by the user
11 = too many attempts to download (current max retries = 10)
12 = completed downloaded size is smaller than the minimum expected size (current minimum = 15728640 KB)
13 = completed downloaded size is larger than the maximum expected size (current maximum = 31457280 KB)

The following apply to the pre-install check phase:
20 = unable to acquire a file handle to the downloaded file
30 = downloaded file's certificate is not trusted by the certificate store
31 = downloaded file's certificate attribute values were incorrect
32 = downloaded file's certificate is not trusted by the certificate store and certificate attribute values were incorrect

The following apply to the install phase:
40 = 1 if the installation timed out (current install timeout = 120 seconds)

Firefox Launch Code = Possible values:
0 = if no attempt was made to launch Firefox
1 = if Firefox was running
2 = if Firefox was launched
Download Retry Count = The number of download retries. If there are no retries this value will be 0.
Downloaded Amount = The amount downloaded in KB for the last attempted download.
Download Phase Seconds = The number of seconds to complete the download phase. It is theoretically possible for this to be 0 if the user cancelled the download really quickly though I have never been able to do so.
Last Download Seconds = The number of seconds to complete the last download which can be different if the previous download had to be restarted. It is theoretically possible for this to be 0 if the user cancelled the download really quickly though I have never been able to do so.
Preinstall Seconds = The number of seconds to complete the pre-installation check phase. If this phase was never entered it will be 0 (e.g. cancelled download).
Install Seconds = The number of seconds to complete the installation phase. If this phase was never entered it will be 0 (e.g. cancelled download).
Finish Seconds = The number of seconds after the installation has finished for the installer to close. If this phase was never entered it will be 0 (e.g. cancelled download).
Opened Download Page = 1 if the page to download the complete installer was opened on failure and 0 otherwise.
Existing Profile = 1 if the %LOCALAPPDATA%\Mozilla\Firefox directory exists and 0 otherwise.
Existing Firefox Version = Existing Firefox version when installing on top of an existing install and 0 otherwise.
Existing Firefox Build ID = Existing Firefox build ID when installing on top of an existing install and 0 otherwise.
New Firefox Version = The new install's Firefox version when installation was successful and 0 otherwise.
New Firefox Build ID = The new install's Firefox build ID when installation was successful and 0 otherwise.
Default Install Dir = 1 when installing into the default installation directory and 0 otherwise.
Has Admin = 1 when installing with administrator privileges and 0 otherwise.
Was Options Button Clicked = 1 when the stub installer's option button was clicked and 0 otherwise.

Comment 3

[Robert Strong (they/them - no direct email)]

Attachment: part 1 - new data points

Comment 4

[Robert Strong (they/them - no direct email)]

Comment on attachment 714248 [details] [diff] [review]
part 1 - new data points

I left STUB_DEBUG defined to prevent metrics from getting data from testing. I'll comment it out before landing.

Comment 5

[Robert Strong (they/them - no direct email)]

Cleaned up details for all of the values

Stub URL Version = a unique ID used to associate the parameters used in the metrics ping as they are modified.

Build Channel = The build channel. Expected values for our builds:
nightly
aurora
beta
release

Update Channel = The application update channel. Expected values for our builds:
nightly
aurora
beta
release

Locale = the Firefox locale that was chosen to install (e.g. en-US)

Firefox x64 = 1 if installing Firefox 64 bit and 0 otherwise (e.g. 32 bit Firefox)

Running x64 Windows = 1 if running Windows 64 bit and 0 otherwise (e.g. 32 bit Windows)

Windows Major Version = self-explanatory (e.g. 6 = Windows 7)

Minor = self-explanatory (e.g. 1 = current Windows 7)

Build = self-explanatory (e.g. 7601 = current Windows 7)

Exit Code = the stub installer's exit code. Possible values:
0 = successful install

The following apply to the download phase:
10 = download cancelled by the user
11 = too many attempts to download (current max retries = 10)
12 = completed downloaded size is smaller than the minimum expected size (current minimum = 15728640 KB)
13 = completed downloaded size is larger than the maximum expected size (current maximum = 31457280 KB)

The following apply to the pre-install check phase:
20 = unable to acquire a file handle to the downloaded file
30 = downloaded file's certificate is not trusted by the certificate store
31 = downloaded file's certificate attribute values were incorrect
32 = downloaded file's certificate is not trusted by the certificate store and certificate attribute values were incorrect

The following apply to the install phase:
40 = 1 if the installation timed out (current install timeout = 120 seconds)

Firefox Launch Code = Possible values:
0 = if no attempt was made to launch Firefox
1 = if Firefox was running
2 = if Firefox was launched

Download Retry Count = The number of download retries. If there are no retries this value will be 0 and the current maximum value is 9 (e.g. maximum attempts = 10 and maximum retries = 9).

Downloaded Amount = The amount downloaded in KB for the last attempted download.

Download Phase Seconds = The number of seconds to complete the download phase which includes all download attempts. It is possible for this to be 0 if the user cancelled the download really quickly.

Last Download Seconds = The number of seconds to complete the last download which can be different from the Download Phase Seconds if the previous download had to be restarted or if it took a second or more to initiate the download. It is possible for this to be 0 if the user cancelled the download really quickly.

Preinstall Seconds = The number of seconds to complete the pre-installation check phase. It is possible for this phase to complete in 0 seconds and if this phase was never entered it will be also 0 (e.g. cancelled download).

Install Seconds = The number of seconds to complete the installation phase. If this phase was never entered it will be 0 (e.g. cancelled download) and I highly doubt it will ever be 0 if the phase was entered.

Finish Seconds = The number of seconds after the installation has finished for the installer to close. It is possible for this phase to complete in 0 seconds and if this phase was never entered it will be 0 (e.g. cancelled download).

Opened Download Page = 1 if the page to download the complete installer was opened on failure and 0 otherwise.

Existing Profile = 1 if the %LOCALAPPDATA%\Mozilla\Firefox directory exists and 0 otherwise.

Existing Firefox Version = Existing Firefox version when installing on top of an existing install and 0 otherwise.

Existing Firefox Build ID = Existing Firefox build ID when installing on top of an existing install and 0 otherwise.

New Firefox Version = The new install's Firefox version when installation was successful and 0 otherwise.

New Firefox Build ID = The new install's Firefox build ID when installation was successful and 0 otherwise.

Default Install Dir = 1 when installing into the default installation directory and 0 otherwise.

Has Admin = 1 when installing with administrator privileges and 0 otherwise.

Was Options Button Clicked = 1 when the stub installer's option button was clicked and 0 otherwise.

Comment 6

[Robert Strong (they/them - no direct email)]

The url for the above is as follows
download-stats.mozilla.org/stub/Stub URL Version/Build Channel/Update Channel/Firefox Locale/Firefox x64/Running x64 Windows/Windows Major Version/Windows Minor Version/Windows Build Version/Exit Code/Firefox Launch Code/Download Retry Count/Downloaded Amount/Download Phase Seconds/Last Download Seconds/Preinstall Seconds/Install Seconds/Finish Seconds/Opened Download Page/Existing Profile/Existing Firefox Version/Existing Firefox Build ID/New Firefox Version/New Firefox Build ID/Default Install Dir/Has Admin/Was Options Button Clicked/

Comment 7

[Brian R. Bondy [:bbondy]]

Documentation nit:

> 12 = completed downloaded size is smaller than the minimum expected 
> size (current minimum = 15728640 KB)
> 13 = completed downloaded size is larger than the maximum expected 
> size (current maximum = 31457280 KB)

These values are in bytes 15MB and 30MB respectively.
By the way we may want to increase the max a bit since the size might be growing by a little bit from a different bug.

Comment 8

[Brian R. Bondy [:bbondy]]

Comment on attachment 714248 [details] [diff] [review]
part 1 - new data points

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

::: browser/installer/windows/nsis/stub.nsi
@@ +116,5 @@
> + * The following errors prefixed with ERR_PREINSTALL apply to the pre-install
> + * check phase.
> + */
> +; Unable to acquire a file handle to the downloaded file
> +!define ERR_PREINSTALL_INVALID_HANDLE 30

The documentation in comment 5 says this should be 20, please update here or in the doc.

@@ +119,5 @@
> +; Unable to acquire a file handle to the downloaded file
> +!define ERR_PREINSTALL_INVALID_HANDLE 30
> +
> +; The downloaded file's certificate is not trusted by the certificate store.
> +!define ERR_PREINSTALL_CERT_UNTRUSTED 31

ditt inconsistent doc #

@@ +122,5 @@
> +; The downloaded file's certificate is not trusted by the certificate store.
> +!define ERR_PREINSTALL_CERT_UNTRUSTED 31
> +
> +; The downloaded file's certificate attribute values were incorrect.
> +!define ERR_PREINSTALL_CERT_ATTRIBUTES 32

ditt inconsistent doc #

@@ +126,5 @@
> +!define ERR_PREINSTALL_CERT_ATTRIBUTES 32
> +
> +; The downloaded file's certificate is not trusted by the certificate store and
> +; certificate attribute values were incorrect.
> +!define ERR_PREINSTALL_CERT_UNTRUSTED_AND_ATTRIBUTES 33

ditt inconsistent doc #

@@ +134,5 @@
> + */
> +; The installation timed out. The installation timeout is defined by the number
> +; of progress steps defined in InstallProgresSteps and the install timer
> +; interval defined in InstallIntervalMS
> +!define ERR_INSTALL_TIMEOUT 40

doc nit: the documentation says 40=1, not sure what that means, but the rest is valid.

@@ +143,5 @@
> +; Minimum size expected to download
> +!define DownloadMinSize 15728640 ; 15 MB in bytes
> +
> +; Maximum size expected to download
> +!define DownloadMaxSize 31457280 ; 30 MB in bytes

Maybe less tight in case people keeps a stub around and uses it a couple years from now for some sort of automated installation.  I'll leave it up to you if you want to change or not.

@@ -319,5 @@
>    ${EndIf}
>  
>    StrCpy $IsDownloadFinished ""
> -  StrCpy $FirefoxLaunch "0"
> -  StrCpy $ExitCode "${ERR_UNKNOWN}"

I'm not 100% sure exit code will be set in all paths, but I suspect you are, so just calling it out in case it's safer to just initialize this.

@@ +1138,5 @@
> +        ${If} $4 < ${DownloadMinSize}
> +          StrCpy $ExitCode "${ERR_DOWNLOAD_SIZE_TOO_SMALL}"
> +        ${ElseIf} $4 > ${DownloadMaxSize}
> +          StrCpy $ExitCode "${ERR_DOWNLOAD_SIZE_TOO_LARGE}"
> +        ${EndIf}

Possibly have 2 new error codes here for "too many retries and too small" and "too many retries and too big"? And then the old error values for when this happens without max retries?

Comment 9

[Robert Strong (they/them - no direct email)]

(In reply to Brian R. Bondy [:bbondy] from comment #7)
> Documentation nit:
> 
> > 12 = completed downloaded size is smaller than the minimum expected 
> > size (current minimum = 15728640 KB)
> > 13 = completed downloaded size is larger than the maximum expected 
> > size (current maximum = 31457280 KB)
> 
> These values are in bytes 15MB and 30MB respectively.
> By the way we may want to increase the max a bit since the size might be
> growing by a little bit from a different bug.
I'll go with 35 MB

(In reply to Brian R. Bondy [:bbondy] from comment #8)
> Comment on attachment 714248 [details] [diff] [review]
> patch rev1
> 
> Review of attachment 714248 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/installer/windows/nsis/stub.nsi
> @@ +116,5 @@
> > + * The following errors prefixed with ERR_PREINSTALL apply to the pre-install
> > + * check phase.
> > + */
> > +; Unable to acquire a file handle to the downloaded file
> > +!define ERR_PREINSTALL_INVALID_HANDLE 30
> 
> The documentation in comment 5 says this should be 20, please update here or
> in the doc.
Fixed

> 
> @@ +119,5 @@
> > +; Unable to acquire a file handle to the downloaded file
> > +!define ERR_PREINSTALL_INVALID_HANDLE 30
> > +
> > +; The downloaded file's certificate is not trusted by the certificate store.
> > +!define ERR_PREINSTALL_CERT_UNTRUSTED 31
> 
> ditt inconsistent doc #
Fixed

> 
> @@ +122,5 @@
> > +; The downloaded file's certificate is not trusted by the certificate store.
> > +!define ERR_PREINSTALL_CERT_UNTRUSTED 31
> > +
> > +; The downloaded file's certificate attribute values were incorrect.
> > +!define ERR_PREINSTALL_CERT_ATTRIBUTES 32
> 
> ditt inconsistent doc #
Fixed

> 
> @@ +126,5 @@
> > +!define ERR_PREINSTALL_CERT_ATTRIBUTES 32
> > +
> > +; The downloaded file's certificate is not trusted by the certificate store and
> > +; certificate attribute values were incorrect.
> > +!define ERR_PREINSTALL_CERT_UNTRUSTED_AND_ATTRIBUTES 33
> 
> ditt inconsistent doc #
Fixed

> 
> @@ +134,5 @@
> > + */
> > +; The installation timed out. The installation timeout is defined by the number
> > +; of progress steps defined in InstallProgresSteps and the install timer
> > +; interval defined in InstallIntervalMS
> > +!define ERR_INSTALL_TIMEOUT 40
> 
> doc nit: the documentation says 40=1, not sure what that means, but the rest
> is valid.
Typo and fixed

> 
> @@ +143,5 @@
> > +; Minimum size expected to download
> > +!define DownloadMinSize 15728640 ; 15 MB in bytes
> > +
> > +; Maximum size expected to download
> > +!define DownloadMaxSize 31457280 ; 30 MB in bytes
> 
> Maybe less tight in case people keeps a stub around and uses it a couple
> years from now for some sort of automated installation.  I'll leave it up to
> you if you want to change or not.
I'll go with 35 MB

> 
> @@ -319,5 @@
> >    ${EndIf}
> >  
> >    StrCpy $IsDownloadFinished ""
> > -  StrCpy $FirefoxLaunch "0"
> > -  StrCpy $ExitCode "${ERR_UNKNOWN}"
> 
> I'm not 100% sure exit code will be set in all paths, but I suspect you are,
> so just calling it out in case it's safer to just initialize this.
I am from the metrics reports and looking through the code though it is no problem to add it back just in case.

> 
> @@ +1138,5 @@
> > +        ${If} $4 < ${DownloadMinSize}
> > +          StrCpy $ExitCode "${ERR_DOWNLOAD_SIZE_TOO_SMALL}"
> > +        ${ElseIf} $4 > ${DownloadMaxSize}
> > +          StrCpy $ExitCode "${ERR_DOWNLOAD_SIZE_TOO_LARGE}"
> > +        ${EndIf}
> 
> Possibly have 2 new error codes here for "too many retries and too small"
> and "too many retries and too big"? And then the old error values for when
> this happens without max retries?
Good call and will add

Comment 10

[Robert Strong (they/them - no direct email)]

Attachment: part 2 - new plugin code

This gets us the IP address for metrics. I think I am going to refrain from removing the code for range requests in case we want to re-add it again one day. I am also going to clean up the new lines in this file prior to checking it in.

Comment 11

[Robert Strong (they/them - no direct email)]

Attachment: new dll

I'll carry forward r+ on the dll after the review.

What do you think about always having the trace functions available? It might prove handy when debugging.

Comment 12

[Robert Strong (they/them - no direct email)]

Then again, it is an additional 5KB in size and every bit / byte counts.

Comment 13

[Robert Strong (they/them - no direct email)]

I'll attach a part 3 with the additional stub NSIS code

Comment 14

[Robert Strong (they/them - no direct email)]

(In reply to Robert Strong [:rstrong] (do not email) from comment #10)
>... I think I am going to refrain from
> removing the code for range requests in case we want to re-add it again one
> day.
I thought about it and think it will be better to just remove it and I'll do that in another patch.

Comment 15

[Robert Strong (they/them - no direct email)]

Attachment: part 3 - plugin range request removal

Though I could remove the redirect handling I'd like to keep it since I think it will be handy for resume support (we can do this for beta and release since they have unique paths).

Comment 16

[Robert Strong (they/them - no direct email)]

Attachment: dll that uses OutputDebugString for testing

Latest dll that uses OutputDebugString for testing

Comment 17

[Robert Strong (they/them - no direct email)]

Attachment: part 4 - plugin dll compiled with previous patches

Implicit r+ if other plugin patches are r+'d

Comment 18

[Robert Strong (they/them - no direct email)]

Attachment: In progress part 5

For testing purposes... still have a little bit more to do.

Comment 19

[Brian R. Bondy [:bbondy]]

Comment on attachment 718111 [details] [diff] [review]
part 2 - new plugin code

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

::: other-licenses/nsis/Contrib/InetBgDL/InetBgDL.cpp
@@ +221,5 @@
> +    // PCSTR.
> +#ifdef UNICODE
> +    swprintf(g_ServerIP, L"%S", lpvStatusInformation);
> +#else
> +    swprintf(g_ServerIP, L"%s", lpvStatusInformation);

If the above comment is correct then there should be no #ifdef UNICODE block here and you should always use swprintf(g_ServerIP, L"%S", lpvStatusInformation);.

@@ +227,5 @@
> +  }
> +
> +#if defined(PLUGIN_DEBUG)
> +  switch (dwInternetStatus)
> +  {

nit: You could probably use FormatMessage here to save some space but I'm fine with this since it's debug code only.

@@ +416,5 @@
>    }
>  
>    if (!hInetSes)
>    {
> +    hInetSes = InternetOpen(USERAGENT, INTERNET_OPEN_TYPE_PRECONFIG, NULL, NULL, 1);

INTERNET_FLAG_IDN_DIRECT? Please use a flag constant here and elsewhere.
Before it was 0 but 0 is used for the absence of flags.

@@ +713,4 @@
>          break;
>        }
>  
> +      FlushFileBuffers(hLocalFile);

This will slow it down a bit and I'm not sure what benefit it will give. If the file handle is closed on a reconnect or something it would be flushed at that point.

Comment 20

[Brian R. Bondy [:bbondy]]

Comment on attachment 718287 [details] [diff] [review]
part 3 - plugin range request removal

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

Please check if BasicParseURL can also be removed.
I think /rangerequest needs to be removed from the NSIS files which used to specify whether or not to use range requests.

Comment 21

[Brian R. Bondy [:bbondy]]

nevermind about the /rangerequest removal comment, I see you do that in the in progress patch.

Comment 22

[Robert Strong (they/them - no direct email)]

(In reply to Brian R. Bondy [:bbondy] from comment #19)
> Comment on attachment 718111 [details] [diff] [review]
> part 2 - new plugin code
> 
> Review of attachment 718111 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: other-licenses/nsis/Contrib/InetBgDL/InetBgDL.cpp
> @@ +221,5 @@
> > +    // PCSTR.
> > +#ifdef UNICODE
> > +    swprintf(g_ServerIP, L"%S", lpvStatusInformation);
> > +#else
> > +    swprintf(g_ServerIP, L"%s", lpvStatusInformation);
> 
> If the above comment is correct then there should be no #ifdef UNICODE block
> here and you should always use swprintf(g_ServerIP, L"%S",
> lpvStatusInformation);.
Thanks!

>...
> @@ +416,5 @@
> >    }
> >  
> >    if (!hInetSes)
> >    {
> > +    hInetSes = InternetOpen(USERAGENT, INTERNET_OPEN_TYPE_PRECONFIG, NULL, NULL, 1);
> 
> INTERNET_FLAG_IDN_DIRECT? Please use a flag constant here and elsewhere.
> Before it was 0 but 0 is used for the absence of flags.
Thanks... thought this was a context flag.

> @@ +713,4 @@
> >          break;
> >        }
> >  
> > +      FlushFileBuffers(hLocalFile);
> 
> This will slow it down a bit and I'm not sure what benefit it will give. If
> the file handle is closed on a reconnect or something it would be flushed at
> that point.
That was leftover from trying to figure out the corrupt download issue. Removed

Comment 23

[Robert Strong (they/them - no direct email)]

Attachment: part 2 - new plugin code rev2

Comment 24

[Robert Strong (they/them - no direct email)]

Comment on attachment 718557 [details] [diff] [review]
part 2 - new plugin code rev2

meh... need to fix up the swprintf

Comment 25

[Brian R. Bondy [:bbondy]]

Comment on attachment 718557 [details] [diff] [review]
part 2 - new plugin code rev2

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

Oops re: the context arg.

Comment 26

[Robert Strong (they/them - no direct email)]

This all works fine for Unicode and is borked in a bunch of ways for ANSI. Let's go with removing ANSI support since we don't care about it... at least for now.

Comment 27

[Robert Strong (they/them - no direct email)]

(In reply to Robert Strong [:rstrong] (do not email) from comment #26)
> This all works fine for Unicode and is borked in a bunch of ways for ANSI.
> Let's go with removing ANSI support since we don't care about it... at least
> for now.
Should have said ignoring ANSI support

Comment 28

[Brian R. Bondy [:bbondy]]

That sounds good to me.

Comment 29

[Robert Strong (they/them - no direct email)]

Attachment: part 3 - plugin range request removal rev2

Carrying forward r+

If BasicParseURL can be removed without problems I'll submit a new patch to do so

Comment 30

[Robert Strong (they/them - no direct email)]

Attachment: part 4 - additional plugin cleanup

Replaced BasicParseURL with InternetCrackURL, made tracing so it doesn't care how many args are passed, and uses the last url found after the redirects.

Comment 31

[Robert Strong (they/them - no direct email)]

Attachment: part 5 - convert newlines

This has been annoying the hell out of me

Comment 32

[Robert Strong (they/them - no direct email)]

Attachment: part 6 - new plugin dll

Implicit r+ if other patches are r+'d

Comment 33

[Robert Strong (they/them - no direct email)]

Attachment: dll that uses OutputDebugString for testing

Comment 34

[Robert Strong (they/them - no direct email)]

Attachment: In progress part 7 for testing

Comment 35

[Robert Strong (they/them - no direct email)]

Attachment: part 4 - additional plugin cleanup rev2

Fixed setting the read buffer

Comment 36

[Robert Strong (they/them - no direct email)]

Attachment: part 5 - convert newlines rev2

Comment 37

[Robert Strong (they/them - no direct email)]

Attachment: part 4 - additional plugin cleanup rev3

Screwed up one call to TRACE in the previous patch... meh

Comment 38

[Robert Strong (they/them - no direct email)]

Attachment: part 5 - convert newlines rev3

Comment 39

[Robert Strong (they/them - no direct email)]

Attachment: part 6 - new plugin dll rev2

Implicit r+ if other patches are r+'d

Comment 40

[Robert Strong (they/them - no direct email)]

Attachment: dll that uses OutputDebugString for testing

Comment 41

[Brian R. Bondy [:bbondy]]

Comment on attachment 719360 [details] [diff] [review]
part 4 - additional plugin cleanup rev3

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

::: other-licenses/nsis/Contrib/InetBgDL/InetBgDL.cpp
@@ +138,5 @@
>      StatsLock_AcquireExclusive();
> +#if defined(_UNICODE)
> +    wsprintf(g_ServerIP, _T("%S"), lpvStatusInformation);
> +#else
> +    wsprintf(g_ServerIP, _T("%s"), lpvStatusInformation);

Should always be %S and no need for #if defined (_UNICODE) here

@@ +418,5 @@
> +  }
> +
> +  INTERNET_SCHEME scheme = uc.nScheme;
> +  INTERNET_PORT port = uc.nPort;
> +  TCHAR server[MAX_STRLEN] = { '\0' };

nit: _T

@@ +421,5 @@
> +  INTERNET_PORT port = uc.nPort;
> +  TCHAR server[MAX_STRLEN] = { '\0' };
> +  // urlpath and extrainfo both have a size of MAX_STRLEN hence why path is
> +  // MAX_STRLEN * 2.
> +  TCHAR path[MAX_STRLEN * 2] = { '\0' };

nit: _T

@@ +439,5 @@
>    HINTERNET hInetFile;
>    DWORD cbio = sizeof(DWORD);
>  
>    // Keep looping until we don't have a redirect anymore
> +  TCHAR URLBuffer[2048] = { '\0' };

nit: _T

::: other-licenses/nsis/Contrib/InetBgDL/InetBgDL.h
@@ +24,5 @@
> +void MYTRACE(LPCTSTR fmt, ...)
> +{
> +  va_list argptr;      
> +  va_start(argptr, fmt); 
> +  TCHAR buffer[2048] = { '\0' };

nit: T('\0') and while here also remove trailing spaces (or do that in the newline removal patch)

@@ +26,5 @@
> +  va_list argptr;      
> +  va_start(argptr, fmt); 
> +  TCHAR buffer[2048] = { '\0' };
> +  wvsprintf(buffer, fmt, argptr);
> +  buffer[(sizeof(buffer)/sizeof(*buffer)) - 1] = '\0';

nit: _T('\0')

@@ +27,5 @@
> +  va_start(argptr, fmt); 
> +  TCHAR buffer[2048] = { '\0' };
> +  wvsprintf(buffer, fmt, argptr);
> +  buffer[(sizeof(buffer)/sizeof(*buffer)) - 1] = '\0';
> +  OutputDebugString(buffer);

Please add va_end(argptr); here

@@ +34,3 @@
>  #else
> +void MYTRACE(...) { }
> +#  define TRACE MYTRACE

I think you can just move the #  define TRACE MYTRACE below the #endif below.

Comment 42

[Robert Strong (they/them - no direct email)]

Attachment: part 4 - additional plugin cleanup rev4

Comments addressed... carrying forward r+

Comment 43

[Robert Strong (they/them - no direct email)]

Attachment: part 5 - convert newlines rev4

Comment 44

[Robert Strong (they/them - no direct email)]

Attachment: part 6 - new plugin dll rev3

New plugin dll built from r+'d patches

Comment 45

[Robert Strong (they/them - no direct email)]

Attachment: part 6 - fix bug with getting the file size

I did a bunch of testing and realized that HTTP_QUERY_CONTENT_LENGTH was being used on a connection that is closed then reopened. This changes the plugin to get this info on the connection that is actually used. I think I have found another bug and I'll submit another patch for that if it really is after I investigate.

Comment 46

[Robert Strong (they/them - no direct email)]

Attachment: part 7 - new plugin dll rev4

Comment 47

[Robert Strong (they/them - no direct email)]

Attachment: part 7 - add server IP to metrics and various other stub.nsi fixes

Comment 48

[Robert Strong (they/them - no direct email)]

(In reply to Robert Strong [:rstrong] (do not email) from comment #47)
> Created attachment 720219 [details] [diff] [review]
> part 8 - add server IP to metrics and various other stub.nsi fixes
Note that I left STUB_DEBUG defined and will remove it on checkin

Comment 49

[Robert Strong (they/them - no direct email)]

(In reply to Robert Strong [:rstrong] (do not email) from comment #9)
> (In reply to Brian R. Bondy [:bbondy] from comment #7)
>...
> > @@ +1138,5 @@
> > > +        ${If} $4 < ${DownloadMinSize}
> > > +          StrCpy $ExitCode "${ERR_DOWNLOAD_SIZE_TOO_SMALL}"
> > > +        ${ElseIf} $4 > ${DownloadMaxSize}
> > > +          StrCpy $ExitCode "${ERR_DOWNLOAD_SIZE_TOO_LARGE}"
> > > +        ${EndIf}
> > 
> > Possibly have 2 new error codes here for "too many retries and too small"
> > and "too many retries and too big"? And then the old error values for when
> > this happens without max retries?
> Good call and will add
Note: I plan on adding this in the next patch.

Comment 50

[Robert Strong (they/them - no direct email)]

The latest set of parameters sent. I'll submit the patch after some more testing

http://download-stats.mozilla.org/stub/Stub_URL_Version/Build_Channel/Update_Channel/Locale/Firefox_x64/Running_x64_Windows/Windows_Major_Version/Window__Minor_Version/Windows_Build_Version/Exit_Code/Firefox_Launch_Code/Download_Retry_Count/Downloaded_Bytes/Introduction_Phase_Seconds/Options_Phase_Seconds/Download_Phase_Seconds/Download_First_Transfer_Seconds/Last_Download_Seconds/PreInstall_Phase_Seconds/Install_Phase_Seconds/Finish_Phase_Seconds/Intro_Page_Shown_Count/Options_Page_Shown_Count/Initial_Install_Requirements_Code/Opened_Download_Page/Existing_Profile/Existing_Firefox_Version/Existing_Firefox_Build_ID/New_Firefox_Version/New_Firefox_Build_ID/Default_Install_Dir/Has_Admin/Download_Server_IP

Stub_URL_Version = a unique ID used to associate the parameters used in the metrics ping as they are modified.

Build_Channel = The build channel. Expected values for our builds:
nightly
aurora
beta
release

Update_Channel = The application update channel. Expected values for our builds:
nightly
aurora
beta
release

Locale = the Firefox locale that was chosen to install (e.g. en-US)

Firefox_x64 = 1 if installing Firefox 64 bit and 0 otherwise (e.g. 32 bit Firefox)

Running_x64_Windows = 1 if running Windows 64 bit and 0 otherwise (e.g. 32 bit Windows)

Windows_Major_Version = self-explanatory (e.g. 6 = Windows 7)

Windows_Minor_Version = self-explanatory (e.g. 1 = current Windows 7)

Windows_Build_Version = self-explanatory (e.g. 7601 = current Windows 7)

Exit_Code = the stub installer's exit code. Possible values:
0 = successful install

The following apply to the download phase:
10 = download cancelled by the user
11 = too many attempts to download (current max retries = 10)

The following apply to the pre-install check phase:
20 = unable to acquire a file handle to the downloaded file
21 = downloaded file's certificate is not trusted by the certificate store
22 = downloaded file's certificate attribute values were incorrect
23 = downloaded file's certificate is not trusted by the certificate store and certificate attribute values were incorrect

The following apply to the install phase:
30 = the installation timed out (current install timeout = 120 seconds)

Firefox_Launch_Code = Possible values:
0 = if no attempt was made to launch Firefox
1 = if Firefox was running
2 = if Firefox was launched

Download_Retry_Count = The number of download retries. If there are no retries this value will be 0 and the current maximum value is 9 (e.g. maximum attempts = 10 and maximum retries = 9).

Downloaded_Bytes = The amount downloaded in bytes for the last attempted download.

Introduction_Phase_Seconds = The total number of seconds the introduction page was displayed (this page can be displayed multiple times).

Options_Phase_Seconds = The total number of seconds the options page was displayed (this page can be displayed multiple times).

Download_Phase_Seconds = The number of seconds to complete the download phase which includes all download attempts. It is possible for this to be 0 if the user cancelled the download really quickly.

Download_First_Transfer_Seconds = The number of seconds from the start of the download phase until the first bytes are received. This is only recorded for first request so it is possible to determine connection issues for the first request.

Last_Download_Seconds = The number of seconds to complete the last download which can be different from the Download Phase Seconds if the previous download had to be restarted or if it took a second or more to initiate the download. It is possible for this to be 0 if the user cancelled the download really quickly.

Preinstall_Phase_Seconds = The number of seconds to complete the pre-installation check phase. It is possible for this phase to complete in 0 seconds and if this phase was never entered it will be also 0 (e.g. cancelled download).

Install_Phase_Seconds = The number of seconds to complete the installation phase. If this phase was never entered it will be 0 (e.g. cancelled download) and I highly doubt it will ever be 0 if the phase was entered.

Finish_Phase_Seconds = The number of seconds after the installation has finished for the installer to close. It is possible for this phase to complete in 0 seconds and if this phase was never entered it will be 0 (e.g. cancelled download).

Intro_Page_Shown_Count = The number of times the introduction page was shown.

Options_Page_Shown_Count = The number of times the options page was shown.

Initial_Install_Requirements_Code = if the installation requirements were not initially met. Possible values:
0 = if the installation requirements were met
1 = if both the write check for the installation directory and the free space check failed
2 = if only the write check for the installation directory failed
3 = if only the free space check failed

Opened_Download_Page = 1 if the page to download the complete installer was opened when a failure occurs and 0 otherwise.

Existing_Profile = 1 if the %LOCALAPPDATA%\Mozilla\Firefox directory exists and 0 otherwise.

Existing_Firefox_Version = Existing Firefox version when installing on top of an existing install and 0 otherwise.

Existing_Firefox_Build_ID = Existing Firefox build ID when installing on top of an existing install and 0 otherwise.

New_Firefox_Version = The new install's Firefox version when installation was successful and 0 otherwise.

New_Firefox_Build_ID = The new install's Firefox build ID when installation was successful and 0 otherwise.

Default_Install_Dir = 1 when installing into the default installation directory and 0 otherwise.

Has_Admin = 1 when installing with administrator privileges and 0 otherwise.

Download_Server_IP = The IP address of the download server.

Comment 51

[Robert Strong (they/them - no direct email)]

Attachment: part 8 - more plugin cleanup

Brian, do you think the redirect handling that is removed in this patch shouldn't be removed and if not why not? If you are ok with it being removed, could you test these patches with the url redirect from http to https that you tested in Bug 829829? Thanks!

Comment 52

[Robert Strong (they/them - no direct email)]

Attachment: part 9 - new plugin dll rev5

implicit r+ if part 8 is r+'d

Comment 53

[Robert Strong (they/them - no direct email)]

Attachment: part 10 - more stub.nsi code

I added a few more data points as described in comment #50. I also changed when the ping is sent since NSIS sends NSPIM_UNLOAD which calls Reset and it was resetting during the ping with the code changes.

I swear... this should be the last patch

Comment 54

[Brian R. Bondy [:bbondy]]

Comment on attachment 721112 [details] [diff] [review]
part 10 - more stub.nsi code

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

I think this is a dupe of patch 8, maybe you uploaded the wrong one?

Comment 55

[Robert Strong (they/them - no direct email)]

Attachment: part 10 - more stub.nsi code

oops

Comment 56

[Robert Strong (they/them - no direct email)]

Attachment: Combined patch

Comment 57

[Robert Strong (they/them - no direct email)]

Attachment: dll that uses OutputDebugString for testing

Comment 58

[Robert Strong (they/them - no direct email)]

Pushed combined patch to mozilla-central
https://hg.mozilla.org/mozilla-central/rev/216ec69cc531

Comment 59

[Alex Keybl [:akeybl]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #58)
> Pushed combined patch to mozilla-central
> https://hg.mozilla.org/mozilla-central/rev/216ec69cc531

a=akeybl on the combined patch for Aurora, since this shouldn't have risk outside of the stub installer, and we're determined to start collecting new data.

Comment 60

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/189f9d9f856c

Comment 61

[Alex Keybl [:akeybl]]

a=akeybl for tip of mozilla-beta as well, no need to wait on testing from Aurora (stub installers are only pushed manually on Beta).

Comment 62

[Alex Keybl [:akeybl]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #60)
> Pushed to mozilla-aurora
> https://hg.mozilla.org/releases/mozilla-aurora/rev/189f9d9f856c

Adding qawanted/verifyme for testing on tomorrow's Aurora stub.

Comment 63

[Jason Smith [:jsmith]]

(In reply to Alex Keybl [:akeybl] from comment #62)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #60)
> > Pushed to mozilla-aurora
> > https://hg.mozilla.org/releases/mozilla-aurora/rev/189f9d9f856c
> 
> Adding qawanted/verifyme for testing on tomorrow's Aurora stub.

I think you want verifyme in this case - (it's a post resolution bug for verification).

Comment 64

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-beta
https://hg.mozilla.org/releases/mozilla-beta/rev/a0449ebebe8f

Comment 65

[Robert Strong (they/them - no direct email)]

Note: the url in comment #50 has Last_Download_Seconds and Download_First_Transfer_Seconds in reversed... so the url is actually as follows

http://download-stats.mozilla.org/stub/Stub_URL_Version/Build_Channel/Update_Channel/Locale/Firefox_x64/Running_x64_Windows/Windows_Major_Version/Window__Minor_Version/Windows_Build_Version/Exit_Code/Firefox_Launch_Code/Download_Retry_Count/Downloaded_Bytes/Introduction_Phase_Seconds/Options_Phase_Seconds/Download_Phase_Seconds/Last_Download_Seconds/Download_First_Transfer_Seconds/PreInstall_Phase_Seconds/Install_Phase_Seconds/Finish_Phase_Seconds/Intro_Page_Shown_Count/Options_Page_Shown_Count/Initial_Install_Requirements_Code/Opened_Download_Page/Existing_Profile/Existing_Firefox_Version/Existing_Firefox_Build_ID/New_Firefox_Version/New_Firefox_Build_ID/Default_Install_Dir/Has_Admin/Download_Server_IP

Comment 66

[Robert Strong (they/them - no direct email)]

I received a sample set of the data last night and the new data points look good. Hopefully that is enough for the verifyme though since I am the patch author I won't change the flag, etc.

Comment 67

[Mihai Morar, (:MihaiMorar)]

How can QA verify this fix? How can I see data points from Comment 1?

Comment 68

[Robert Strong (they/them - no direct email)]

Note: The implemented data points are in comment #50 and the url format is in comment #65.

A verification from QA would at most show that the data is being sent which we have verified from the server logs as noted in comment #66. To verify this from a system at best you could run wireshark and look at the url to download-stats for the data points. 
cc'ing juanb who has done this previously.

btw: if fiddler is used you might run into other problems as has been seen when performing any download while fiddler is intercepting packets.

Comment 69

[juan becerra [:juanb]]

I've verified this on Win XP using the latest 20.0b4 stub installer builds from: http://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/20.0b4-candidates/build1/firefox-20.0.en-US.win32.installer-stub.exe

A sample Wire Shark entry when running the stub installer displays this kind of ping:

souce: 172.16.237.164
destination:63.245.217.79
protocol: HTTP
lenght: 287
info: GET /stub/v5/beta/beta/en-US/0/0/5/1/2600/0/2/0/21648680/1/0/14/14/0/0/7/6/1/0/0/0/1/0/0/20.0/20130307075451/1/1/184.51.102.8 HTTP/1.1