322206 - (StubInstaller) Firefox net / stub installer for x86

Status: VERIFIED FIXED

(Firefox :: Installer, defect)

Description

[Benjamin Smedberg]

We want/need a new installer for Firefox 2.0 which allows for stub downloads as well as ditching the xpinstall system in favor of flatter archives.

Thunderbird should probably have something similar; the amount of shared code depends on the architecture of the new installer.

Beltzner's initial-cut UI spec is wiki.mozilla.org/User:Beltzner/Specification_of_Stub_Installer_User_Interface

Rob Strong will take the lead here and decide whether to use existing technologies such as NSIS, or even switch entirely to a MSI-based solution (using WIX?).

This installer will also lead us to the Firefox 3 installer which is XULRunner+app... we're going to try and develop a shared solution to reduce development overhaul.

Comment 1

[Grey Nicholson (they/them)]

*** Bug 238212 has been marked as a duplicate of this bug. ***

Comment 2

[Kohei Yoshino [:kohei]]

We're hearing more and more software vendors would like to bundle (an unmodified version of) Firefox or Thunderbird with their application. Google Chrome also has a net installer. This should be seriously considered.

Comment 3

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

Jim Mathies made one for Firefox distributions a couple of years ago... cc'ing him

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This is pretty straightforward to do with Windows Installer (you can pass a URL to Windows Installer and it will do "The Right Thing") but would be a lot more difficult if we wanted it to be cross platform.  The bug is marked All but the description and Beltzner's spec both refer to Windows ...

Comment 6

[u217243]

jmathies: do you have a sample of net installer you made before?

Comment 7

[Jim Mathies [:jimm]]

(In reply to comment #6)
> jmathies: do you have a sample of net installer you made before?

No, but the app wouldn't be hard to recreate. I'm not sure a stand alone exe downloader is the best approach these days though. Do we have a basic list of requirements from vendors? How would they like to integrate our net installer with their software?

Comment 8

[Kohei Yoshino [:kohei]]

Although we don't have specific requirements, we'd like to have a generic net installer that other executables are also able to invoke.

Consider some cases:

* PC makers which would like to bundle Firefox/Thunderbird
* external HDD/USB makers would like to bundle Firefox/Thunderbird
* ISPs which would like to add "Install Thunderbird" button on their own setup tool

Personally I like ChromeSetup.exe -- simple and fast.

Comment 10

[Chris Lyon [:clyon]]

what's holding this up from happening.

Comment 11

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

Other work that has been given a higher priority.

It would be helpful to those that have to do the work if the security issue (e.g. sg:want) this helps with or resolves was provided. Thanks

Comment 12

[Jesse Ruderman]

The primary security benefit is letting us fix bug 358384 without spending gobs of money.  See bug 358384 comment 19.

A secondary security benefit is letting us get 64-bit Windows builds to computers that support them.  64-bit builds have much stronger ASLR, and ASLR has been a key line of defense against exploitation for the last few years.  But this also depends on us actually supporting 64-bit builds ;)

Comment 13

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

Attachment: mockups given to faaborg on 2/11/2011

Comment 14

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

We aren't going to have the same stub installer for all platforms so limiting this bug to Windows only.

Comment 15

[alex_mayorga]

Let me get it out of my system, I despise net installers (why to wait for download again after I've "downloaded" the installer, there's people out there that still doesn't have always on connectivity)

(In reply to comment #12)
> A secondary security benefit is letting us get 64-bit Windows builds to
> computers that support them.

Wouldn't sniffing for "WOW64" in the user agent determine 64-bit Windows "reliably"?
Non scientific test, I went to http://whatsmyuseragent.com/ on 32-bit IE8 on Windows 7 64-bit and it reports " Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.1; WOW64 [...]"

Comment 17

[Jacob Appelbaum]

Have you guys considered the use of TUF ( https://www.updateframework.com/ ) as a way to make this happen?

Comment 18

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

I've looked at it and it doesn't seem like a good fit for what we want to accomplish with this bug.

Comment 19

[Jacob Appelbaum]

TUF seems to cover all of the likely security issues that are known and it covers them in a safe way.

What are the objections?

Another that is basically TUF:
https://gitweb.torproject.org/thandy.git

Comment 20

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

For starters, this is an installer and not an updater and we will be using the same ui and the majority of the same code when installing with a full installer as with the stub.

Comment 21

[Jacob Appelbaum]

Thandy is an installer and an updater. TUF is the same.

They both place a heavy focus on secure updating but they both also care very much about the initial install. This seems pretty relevant to MoCo.

What are the other issues?

Comment 22

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

Can you provide details as to the specific benefits that TUF will provide? As a matter if fact, please do so in Bug 696472.

Comment 23

[Jacob Appelbaum]

Yeah, I'll put it in the other bug.

For anyone reading along at home, please read these:
https://www.updateframework.com/wiki/About
https://www.updateframework.com/browser/specs/tuf-spec.txt
http://freehaven.net/~arma/tuf-ccs2010.pdf

Just to crib from the spec and why TUF is so important as a bare minimum:

1.5.2. Goals for specific attacks to protect against

   Note: When saying the framework protects against an attack, this means that
   the attack will not be successful.  It does not mean that a client will
   always be able to successfully update during an attack.  Fundamentally, an
   attacker positioned to intercept and modify a client's communication will
   always be able to perform a denial of service.  The part we have control
   over is not allowing an inability to update to go unnoticed.

   Rollback attacks.  Attackers should not be able to trick clients into
   installing software that is older than that which the client previously knew
   to be available.

   Indefinite freeze attacks.  Attackers should not be able respond to client
   requests with the same, outdated metadata without the client being aware of
   the problem.

   Endless data attacks.  Attackers should not be able to respond to client
   requests with huge amounts of data (extremely large files) that interfere
   with the client's system.

   Slow retrieval attacks.  Attackers should not be able to prevent clients
   from being aware of interference with receiving updates by responding to
   client requests so slowly that automated updates never complete.

   Extraneous dependencies attacks.  Attackers should not be able to cause
   clients to download or install software dependencies that are not the
   intended dependencies.

   Mix-and-match attacks.  Attackers should not be able to trick clients into
   using a combination of metadata that never existed together on the
   repository at the same time.

   Malicious repository mirrors should not be able to prevent updates from good
   mirrors.

Comment 24

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

From the TUF docs, TUF is a security framework for installing installers. For example:
https://gitweb.torproject.org/thandy.git/blob/HEAD:/specs/thandy-spec.txt#l119

states that the source could be an MSI which *IS* an installer. So, with what we are trying to accomplish with the stub installer we would need to implement it to install TUF if we were to go with TUF. We can evaluate whether there is value in using TUF in bug 696472 but TUF does not negate our need for a stub installer.

Comment 25

[Jacob Appelbaum]

(In reply to Robert Strong [:rstrong] (do not email) from comment #24)
> From the TUF docs, TUF is a security framework for installing installers.
> For example:
> https://gitweb.torproject.org/thandy.git/blob/HEAD:/specs/thandy-spec.
> txt#l119
> 
> states that the source could be an MSI which *IS* an installer. So, with
> what we are trying to accomplish with the stub installer we would need to
> implement it to install TUF if we were to go with TUF.

Yes, you have to install the TUF installer to install all the other components securely.

> We can evaluate
> whether there is value in using TUF in bug 696472 but TUF does not negate
> our need for a stub installer.

The stub installer needs to solve all of the issues that TUF addresses - otherwise a network attacker can serve an old firefox to a new installer, no?

It sounds like an MSI wrapper around a TUF binary is what is needed.

Comment 26

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

You can wrap it in an MSI, download a TUF installer with a stub, etc. and the point I am making is that TUF doesn't remove the need for a stub installer, etc. since TUF itself needs to be installed. Also, the stub installer needs to securely and safely handle a subset of what TUF does.

btw: the reason I commented here was to let the people cc'd on this bug know that TUF itself doesn't solve what this bug is going to solve and further advocacy comments regarding TUF should be made in bug 696472.

Comment 27

[Jacob Appelbaum]

(In reply to Robert Strong [:rstrong] (do not email) from comment #26)
> You can wrap it in an MSI, download a TUF installer with a stub, etc. and
> the point I am making is that TUF doesn't remove the need for a stub
> installer, etc. since TUF itself needs to be installed. Also, the stub
> installer needs to securely and safely handle a subset of what TUF does.
> 

That's the part where we disagree. I suggest that you take a TUF installer and wrap that in a signed MSI. TUF seems like it will be a fine stub.

> btw: the reason I commented here was to let the people cc'd on this bug know
> that TUF itself doesn't solve what this bug is going to solve and further
> advocacy comments regarding TUF should be made in bug 696472.

As far as I can tell, TUF would meet the needs of any reasonable stub.

In any case, I see in wiki.mozilla.org/User:Beltzner/Specification_of_Stub_Installer_User_Interface that it's all UI and no security concerns. Seems unwise. Is there a real list of requirements that talks about anything other than the UI?

Comment 28

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Mozilla China already implemented a stub installer and uses it by default for the China edition of Firefox. I do not know if/how much we could reuse what they have done, but it seems like a good goal to get the China edition to use the same stub installer as regular Firefox, so that we know how the China edition installer works and so Mozilla China, secteam, and the installer team are all on the same page.

Comment 29

[Bryan Quigley]

Using the "net installer" isn't going to be default from the Mozilla site, correct?  (I understand the full installer will use the same underlying code, just without the downloader part)

Would another option be to have the application binary in the core download, and then have it pull just language packs? (This could give many other advantages, less data to replicate between mirrors, etc)

Comment 30

[alanjstr]

Because of the way Mozilla currently builds Firefox, the languages are built into the big DLL and not separated.

Comment 31

[John Hopkins (:jhopkins)]

On the topic of a stub installer:
* will Mozilla continue to provide a standaline .exe installer for Windows (for offline installs, etc.) once the stub installer is released or will it be 'hidden away' from the main download page?
* will a stub installer trigger any _runtime_ behaviour alerts of popular anti-virus engines (eg. an .exe downloads and runs another .exe -> raise alert)?  I'd suggest some testing in that area.

Comment 32

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

(In reply to John Hopkins (:jhopkins) from comment #31)
> On the topic of a stub installer:
> * will Mozilla continue to provide a standaline .exe installer for Windows
> (for offline installs, etc.) once the stub installer is released or will it
> be 'hidden away' from the main download page?
The plan is to also provide a full installer and I don't expect that to change since deployments will need this.

> * will a stub installer trigger any _runtime_ behaviour alerts of popular
> anti-virus engines (eg. an .exe downloads and runs another .exe -> raise
> alert)?  I'd suggest some testing in that area.
We have done various stub installers over the years and it I am fairly certain it won't anymore than any other stub installer (or for that matter our current installer which is also based on NSIS). One example, some Firewalls can be configured to require the user to allow network access for the stub. And of course we will test.

Comment 33

[Jason Smith [:jsmith]]

FYI - Proposal for testing this feature is here - https://etherpad.mozilla.org/stub-installer-testing.

Comment 34

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

Attachment: CertCheck NSIS plugin code

I considered going with just including certificatecheck.h / certificatecheck.cpp but decided against it since I'd like to make this available on the NSIS site as well. With that in mind, this wouldn't need to land if I make it available on the NSIS site.

Comment 35

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

Attachment: CertCheck NSIS dll

Comment 36

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

Attachment: 1. CertCheck dll for checkin

This is the diff of the dll for checkin if the code passes review

Comment 37

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

Attachment: InetBgDL NSIS plug-in dll

From http://nsis.sourceforge.net/InetBgDL_plug-in

Comment 38

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

Attachment: 2. InetBgDL NSIS plug-in for checkin

Brian, could you take a look at the code in the following plugin?
http://nsis.sourceforge.net/InetBgDL_plug-in

Comment 39

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

Attachment: 3. New artwork (still need artwork for Aurora, Nightly, and unofficial... these are placeholders)

Comment 40

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

Comment on attachment 661702 [details] [diff] [review]
3. New artwork (still need artwork for Aurora, Nightly, and unofficial... these are placeholders)

For now the official artwork will be used for all branding until the new artwork is available.

Comment 41

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

Attachment: 4. Makefile changes for new plugins and artwork

Comment 42

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

also, generic locale config files

Comment 43

[Brian R. Bondy [:bbondy]]

Awesome! I'll be reviewing in the morning.

Comment 44

[Frank Wein [:mcsmurf]]

Robert: Maybe you already mentioned it somewhere, but I can't find it atm: Do I need to include new artwork in the SeaMonkey code as well so that the NSIS build process won't break or is this optional for now (not sure if we want/need a stub installer, we'll see)?

Comment 45

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

You don't. Artwork is entirely app specific and copied by app Makefiles, etc.
http://mxr.mozilla.org/mozilla-central/source/browser/branding/nightly/

Comment 46

[Brian R. Bondy [:bbondy]]

Comment on attachment 661702 [details] [diff] [review]
3. New artwork (still need artwork for Aurora, Nightly, and unofficial... these are placeholders)

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

I *think* these could fit into a color table with 16 colors (4bpp), which would half the size of the bmps.  But that can be done/investigate in a follow up bug after this all lands.

Comment 47

[Brian R. Bondy [:bbondy]]

Comment on attachment 661705 [details] [diff] [review]
4. Makefile changes for new plugins and artwork

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

Impressive that you even flipped the pencil bmp for RTL :)

I assume these files are coming in a different patch?
locale.nlf \
locale-rtl.nlf \

Comment 48

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

(In reply to Brian R. Bondy [:bbondy] from comment #47)
> Comment on attachment 661705 [details] [diff] [review]
> Makefile changes for new plugins and artwork
> 
> Review of attachment 661705 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Impressive that you even flipped the pencil bmp for RTL :)
> 
> I assume these files are coming in a different patch?
> locale.nlf \
> locale-rtl.nlf \
Forgot to hg add them. Since I accidentally deleted the ui resource file and had to recreate it so I'll add them there.

Comment 49

[Brian R. Bondy [:bbondy]]

Comment on attachment 661701 [details] [diff] [review]
2. InetBgDL NSIS plug-in for checkin

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

I think it's scary to add binaries without source, could we also include the source for the matching binary too? 
other-licenses\nsis\Contrib
http://nsis.sourceforge.net/InetBgDL_plug-in

Comment 50

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

Actually, they are in attachment #661705 [details] [diff] [review] but they are utf-16le. I'll resubmit attachment #661705 [details] [diff] [review] without it with the r+ and submit it separately as utf-8.

Comment 51

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

(In reply to Brian R. Bondy [:bbondy] from comment #49)
> Comment on attachment 661701 [details] [diff] [review]
> InetBgDL NSIS plug-in for checkin
> 
> Review of attachment 661701 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think it's scary to add binaries without source, could we also include the
> source for the matching binary too? 
> other-licenses\nsis\Contrib
> http://nsis.sourceforge.net/InetBgDL_plug-in
We could but we typically only do that when we modify source and just use the existing source. I also see this as little different than the binaries we use in Mozilla Build for this case.

Comment 52

[Brian R. Bondy [:bbondy]]

I like to include source for any binary in case the code ever disappears 10 years from now and someone needs to make a change.  But if you're against it, that's OK :D

Comment 53

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

Attachment: 5. binary resource containing the new ui for checkin

Comment 54

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

Attachment: binary resource containing the new ui

Comment 55

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

Attachment: generic locale files and Makefile changes

Comment 56

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

Comment on attachment 661721 [details] [diff] [review]
generic locale files and Makefile changes

Missed a couple of things

Comment 57

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

Attachment: 4. Makefile changes for new plugins and artwork rev2

Removed references to locale.nlf and locale-rtl.nlf. Carrying forward r+

Comment 58

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

Attachment: 6. locale files (generic) and Makefile changes for new ui

Comment 59

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

(In reply to Brian R. Bondy [:bbondy] from comment #52)
> I like to include source for any binary in case the code ever disappears 10
> years from now and someone needs to make a change.  But if you're against
> it, that's OK :D
I'm ok with it... I'll put together a patch tomorrow for that.

Comment 60

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

Attachment: artwork zip file for reference

Comment 61

[Brian R. Bondy [:bbondy]]

Comment on attachment 661692 [details] [diff] [review]
CertCheck NSIS plugin code

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

Since this code is used in 2 places, it would be good to have security pass through this patch as well to make sure we're using WinVerifyTrust as they expect we should.  They did do a review of the service, but that was a ton of code.  I think a targeted review of CertCheck.cpp only would be worth the hassle (but should not hold up landing).

::: other-licenses/nsis/Contrib/CertCheck/CertCheck.cpp
@@ +203,5 @@
> +
> +/**
> + * Verifies the trust of the specified file path.
> + *
> + * @param  stacktop  A pointer to the top of the stack

nit: Pls add what you're expecting to be pushed on the stack, i.e. filepath, certname, and certIssuer, where file path is the top of the stack.

@@ +205,5 @@
> + * Verifies the trust of the specified file path.
> + *
> + * @param  stacktop  A pointer to the top of the stack
> + * @param  variables A pointer to the NSIS variables 
> + * @return 1 if the trust was verified, 0 otherwise

nit: Also indicate that the stack will be modified to contain "1" if successful

@@ +278,5 @@
> +  trustData.pFile = &fileToCheck;
> +
> +  GUID policyGUID = WINTRUST_ACTION_GENERIC_VERIFY_V2;
> +  // Check if the file is signed by something that is trusted.
> +  LONG ret = WinVerifyTrust(NULL, &policyGUID, &trustData);

nit: just return ret here, the code below doesn't matter since there is no more logging in this code.

@@ +290,5 @@
> +
> +/**
> + * Verifies the trust of the specified file path.
> + *
> + * @param  stacktop  A pointer to the top of the stack

nit: document how the stack will be used and modified as above.

Comment 62

[Brian R. Bondy [:bbondy]]

Comment on attachment 661695 [details]
1. CertCheck dll for checkin

r+ assuming this was built with VC6

Comment 63

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

Comment on attachment 661695 [details]
1. CertCheck dll for checkin

Didn't have time to setup my VC6 environment. Would you by chance still have yours setup?

Comment 64

[Brian R. Bondy [:bbondy]]

I think I have a VM with it setup, I'll check into it tomorrow when I'm at that laptop.

Comment 65

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

Comment on attachment 661692 [details] [diff] [review]
CertCheck NSIS plugin code

Ian, this is essentially the same code as used by the maintenance service's certificate check. It doesn't include those files so this can be a stand alone NSIS plugin that will be submitted to the NSIS site. Could you review it just in case?

Comment 66

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #63)
> Comment on attachment 661695 [details]
> 1. CertCheck dll for checkin
> 
> Didn't have time to setup my VC6 environment. Would you by chance still have
> yours setup?

Crap, I don't have it anymore. I think it was on my win2k VM.  I think we should just file one or two follow ups to reduce the installer file size via 4bpp bmps and also compiling this dll with vc6.

Comment 67

[Jason Smith [:jsmith]]

Btw - Feel free to drop a reference to any builds for testing here. I'll start testing it when I get a reference.

Comment 68

[Ian Melven :imelven]

(In reply to Robert Strong [:rstrong] (do not email) from comment #65)
> Comment on attachment 661692 [details] [diff] [review]
> CertCheck NSIS plugin code
> 
> Ian, this is essentially the same code as used by the maintenance service's
> certificate check. It doesn't include those files so this can be a stand
> alone NSIS plugin that will be submitted to the NSIS site. Could you review
> it just in case?

I will take a look ASAP. bsmith, I know you are very busy but you might want to have a look as well.

Comment 69

[Ian Melven :imelven]

Comment on attachment 661692 [details] [diff] [review]
CertCheck NSIS plugin code

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

It might be good to make sure the stub installer binaries have ASLR and /GS and all that goodness turned on, but that's a separate/followup
bug IMO. 

A few nits, but the string NULL termination stuff are the only real potential issues, I think.

::: other-licenses/nsis/Contrib/CertCheck/CertCheck.cpp
@@ +33,5 @@
> + * @param  certContext  The certificate context of the file
> + * @param  infoToMatch  The acceptable information to match
> + * @return FALSE if the info does not match or if any error occurs in the check
> + */
> +BOOL 

nit: trailing whitespace

@@ +34,5 @@
> + * @param  infoToMatch  The acceptable information to match
> + * @return FALSE if the info does not match or if any error occurs in the check
> + */
> +BOOL 
> +DoCertificateAttributesMatch(PCCERT_CONTEXT certContext, 

nit : trailing whitespace

@@ +42,5 @@
> +  LPTSTR szName = NULL;
> +
> +  if (infoToMatch.issuer) {
> +    // Pass in NULL to get the needed size of the issuer buffer.
> +    dwData = CertGetNameString(certContext, 

nit : trailing whitespace

@@ +97,5 @@
> +      return FALSE;
> +    }
> +
> +    // If the issuer does not match, return a failure.
> +    if (!infoToMatch.name || 

nit: trailing whitespace

@@ +108,5 @@
> +    LocalFree(szName);
> +  }
> +
> +  // If there were any errors we would have aborted by now.
> +  return TRUE;

still not sure about returning TRUE in the case when neither a name or issue is passed in

@@ +120,5 @@
> + * @return ERROR_SUCCESS if successful, ERROR_NOT_FOUND if the info 
> + *         does not match, or the last error otherwise.
> + */
> +DWORD
> +CheckCertificateForPEFile(LPCWSTR filePath, 

nit: trailing whitespace

@@ +124,5 @@
> +CheckCertificateForPEFile(LPCWSTR filePath, 
> +                          CertificateCheckInfo &infoToMatch)
> +{
> +  HCERTSTORE certStore = NULL;
> +  HCRYPTMSG cryptMsg = NULL; 

nit: trailing whitespace

@@ +132,5 @@
> +
> +  // Get the HCERTSTORE and HCRYPTMSG from the signed file.
> +  DWORD encoding, contentType, formatType;
> +  BOOL result = CryptQueryObject(CERT_QUERY_OBJECT_FILE,
> +                                 filePath, 

nit: trailing whitespace

@@ +134,5 @@
> +  DWORD encoding, contentType, formatType;
> +  BOOL result = CryptQueryObject(CERT_QUERY_OBJECT_FILE,
> +                                 filePath, 
> +                                 CERT_QUERY_CONTENT_FLAG_PKCS7_SIGNED_EMBED,
> +                                 CERT_QUERY_CONTENT_FLAG_ALL, 

nit: trailing whitespace

@@ +144,5 @@
> +  }
> +
> +  // Pass in NULL to get the needed signer information size.
> +  DWORD signerInfoSize;
> +  result = CryptMsgGetParam(cryptMsg, CMSG_SIGNER_INFO_PARAM, 0, 

nit: trailing whitespace

this assumes there's only one signer, which i assume there always will be for now 
for our signed packages/updates

@@ +160,5 @@
> +  }
> +
> +  // Get the signer information (PCMSG_SIGNER_INFO).
> +  // In particular we want the issuer and serial number.
> +  result = CryptMsgGetParam(cryptMsg, CMSG_SIGNER_INFO_PARAM, 0, 

nit: trailing whitespace

@@ +168,5 @@
> +    goto cleanup;
> +  }
> +
> +  // Search for the signer certificate in the certificate store.
> +  CERT_INFO certInfo;     

nit: trailing whitespace

@@ +171,5 @@
> +  // Search for the signer certificate in the certificate store.
> +  CERT_INFO certInfo;     
> +  certInfo.Issuer = signerInfo->Issuer;
> +  certInfo.SerialNumber = signerInfo->SerialNumber;
> +  certContext = CertFindCertificateInStore(certStore, ENCODING, 0, 

nit: trailing whitespace

@@ +204,5 @@
> +/**
> + * Verifies the trust of the specified file path.
> + *
> + * @param  stacktop  A pointer to the top of the stack
> + * @param  variables A pointer to the NSIS variables 

nit : trailing whitespace

@@ +208,5 @@
> + * @param  variables A pointer to the NSIS variables 
> + * @return 1 if the trust was verified, 0 otherwise
> + */
> +extern "C" void __declspec(dllexport)
> +VerifyCertNameIssuer(HWND hwndParent, int string_size, 

nit: trailing whitespace

@@ +229,5 @@
> +    MultiByteToWideChar(CP_ACP, 0, tmp3, -1, certIssuer, MAX_PATH);
> +#else
> +    wcscpy(filePath, tmp1);
> +    wcscpy(certName, tmp2);
> +    wcscpy(certIssuer, tmp3);

would prefer wcsncpy and explicit null termination of all these

@@ +234,5 @@
> +#endif
> +
> +  CertificateCheckInfo allowedCertificate = {
> +    certName, 
> +    certIssuer, 

nit: trailing whitespace after both lines above

@@ +267,5 @@
> +  trustData.cbStruct = sizeof(trustData);
> +  trustData.pPolicyCallbackData = NULL;
> +  trustData.pSIPClientData = NULL;
> +  trustData.dwUIChoice = WTD_UI_NONE;
> +  trustData.fdwRevocationChecks = WTD_REVOKE_NONE; 

nit: trailing whitespace

this seems to leave whether revocation checks are done up to the authenticode policy provider's default,
which seems ok.

@@ +278,5 @@
> +  trustData.pFile = &fileToCheck;
> +
> +  GUID policyGUID = WINTRUST_ACTION_GENERIC_VERIFY_V2;
> +  // Check if the file is signed by something that is trusted.
> +  LONG ret = WinVerifyTrust(NULL, &policyGUID, &trustData);

the use of WinVerifyTrust looks ok to me - we should test to make sure we're correct about what we can do with respect to revocation etc
as part of shipping this feature, IMO

@@ +291,5 @@
> +/**
> + * Verifies the trust of the specified file path.
> + *
> + * @param  stacktop  A pointer to the top of the stack
> + * @param  variables A pointer to the NSIS variables 

nit: trailing whitespace

@@ +295,5 @@
> + * @param  variables A pointer to the NSIS variables 
> + * @return 1 if the trust was verified, 0 otherwise
> + */
> +extern "C" void __declspec(dllexport)
> +VerifyCertTrust(HWND hwndParent, int string_size, 

nit: trailing whitespace

@@ +304,5 @@
> +
> +  popstring(stacktop, tmp, MAX_PATH);
> +
> +#if !defined(UNICODE)
> +    MultiByteToWideChar(CP_ACP, 0, tmp, -1, filePath, MAX_PATH);

MultiByteToWideChar does not guarantee terminating strings properly

@@ +306,5 @@
> +
> +#if !defined(UNICODE)
> +    MultiByteToWideChar(CP_ACP, 0, tmp, -1, filePath, MAX_PATH);
> +#else
> +    wcscpy(filePath, tmp);

would prefer wcsncpy

@@ +317,5 @@
> +    pushstring(stacktop, TEXT("0"), 2);
> +  }
> +}
> +
> +BOOL WINAPI 

nit: trailing whitespace

@@ +340,5 @@
> +    return 1;
> +  }
> +
> +  th = (*stacktop);
> +  lstrcpyn(str,th->text, len);

might be safer to explicitly null terminate the string always

@@ +357,5 @@
> +*/
> +void pushstring(stack_t **stacktop, const TCHAR *str, int len)
> +{
> +  stack_t *th;
> +  if (!stacktop) { 

nit: trailing whitespace

@@ +362,5 @@
> +    return;
> +  }
> +
> +  th = (stack_t*)GlobalAlloc(GPTR, sizeof(stack_t) + len);
> +  lstrcpyn(th->text, str, len);

might be safer to explicitly null terminate the string always

Comment 70

[Ian Melven :imelven]

Comment on attachment 661692 [details] [diff] [review]
CertCheck NSIS plugin code

r+ but i would like the string termination stuff to be more bulletproof.

Comment 71

[Brian R. Bondy [:bbondy]]

Comment on attachment 661692 [details] [diff] [review]
CertCheck NSIS plugin code

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

::: other-licenses/nsis/Contrib/CertCheck/CertCheck.cpp
@@ +216,5 @@
> +  TCHAR tmp2[MAX_PATH] = { L'\0' };
> +  TCHAR tmp3[MAX_PATH] = { L'\0' };
> +  WCHAR filePath[MAX_PATH] = { '\0' };
> +  WCHAR certName[MAX_PATH] = { '\0' };
> +  WCHAR certIssuer[MAX_PATH] = { '\0' };

nit: The WCHAR variations should be L'\0', the TCHAR ones _T('\0')
Also changing all of these declarations to MAX_PATH + 1 I think would solve the potential null termination issues

Comment 72

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

Attachment: 1.extra CertCheck NSIS plugin code rev2

Brian, could you check if I made the requested changes. I went with your suggestion in comment #71. I didn't change pushstring or popstring either since it is used pretty consistently in the NSIS plugin code and if it is changed we can just create a helper that all of our NSIS plugin code uses.

Comment 73

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

I'm pretty certain I have VC 6 around somewhere and I'll recompile the plugin after I find it.

Comment 74

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

Attachment: 7. Common code

Comment 75

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

Attachment: 8. Locale support

I'm still working on some of the one off UI and if I have to add more strings I'll do so in a future patch.

Comment 76

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

Attachment: 9. stub patch in progress

Brian, there are still a couple of more things to do such as providing a link to download the full installer if the cert checks fail... I'm submitting it for your reference.

Comment 77

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

Brian, though bug 791613 won't help with the stub initially I'd like to land it with this at the same time and possibly try to jump a couple of trains as well.

Comment 78

[Brian R. Bondy [:bbondy]]

Comment on attachment 662018 [details] [diff] [review]
1.extra CertCheck NSIS plugin code rev2

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

r=me

::: other-licenses/nsis/Contrib/CertCheck/CertCheck.cpp
@@ +249,5 @@
> +    MultiByteToWideChar(CP_ACP, 0, tmp3, -1, certIssuer, MAX_PATH);
> +#else
> +    wcscpy(filePath, tmp1);
> +    wcscpy(certName, tmp2);
> +    wcscpy(certIssuer, tmp3);

nit: might as well change these to wcsncpy(..., MAX_PATH); x3

@@ +315,5 @@
> +VerifyCertTrust(HWND hwndParent, int string_size,
> +                TCHAR *variables, stack_t **stacktop, void *extra)
> +{
> +  TCHAR tmp[MAX_PATH] = { _T('\0') };
> +  WCHAR filePath[MAX_PATH] = { L'\0' };

nit: Make these MAX_PATH + 1 to guarantee null termination below

@@ +322,5 @@
> +
> +#if !defined(UNICODE)
> +    MultiByteToWideChar(CP_ACP, 0, tmp, -1, filePath, MAX_PATH);
> +#else
> +    wcscpy(filePath, tmp);

nit: Might as well change this to wcsncpy(..., MAX_PATH);

Comment 79

[Brian R. Bondy [:bbondy]]

Comment on attachment 662088 [details] [diff] [review]
8. Locale support

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

::: browser/locales/en-US/installer/nsisstrings.properties
@@ +22,5 @@
> +
> +INTRO_BLURB=Thanks for choosing $BrandFullName, the browser that chooses you above everything else. Click to install.
> +INSTALL_BLURB1=You're about to enjoy the very latest in speed, flexibility and security so you're always in control.
> +INSTALL_BLURB2=That’s because $BrandShortName is made by a non-profit to make browsing and the Web better for you.
> +INSTALL_BLURB3=You’re also joining a global community of users, contributors and developers working to make the best browser in the world.

nit: INSTALL_BLURB3=You’re also joining a global community of users, contributors, and developers working to make the best browser in the world. (extra comma before and is preferred IMHO)

@@ +50,5 @@
> +DOWNLOADING_DONE=Downloaded
> +INSTALLING_TO_BE_DONE=Installing
> +INSTALLING_IN_PROGRESS=Installing…
> +
> +SELECT_FOLDER=Select the folder to install Firefox in.

nit: SELECT_FOLDER=Select the folder to install Firefox in: (may go over them all to see which ones should have colons and which ones shouldn't. It looks inconsistent but it's hard to tell just from this)

Comment 80

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

They can be seen in these screenshots
http://exchangecode.com/robert/work/screenshots.png

cc'ing shorlander and Madhava for input on the string changes in comment #79

Comment 81

[Brian R. Bondy [:bbondy]]

Comment on attachment 662081 [details] [diff] [review]
7. Common code

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

r=me with changes

::: toolkit/mozapps/installer/windows/nsis/common.nsh
@@ +6985,5 @@
> +!define /math PBM_SETRANGE32 ${WM_USER} + 6
> +
> +!define SHACF_FILESYSTEM 1
> +
> +!define MOZ_LOADTRANSPARENT ${LR_LOADFROMFILE}|${LR_LOADTRANSPARENT}|${LR_LOADMAP3DCOLORS}

nit: You may want to document somewhere that since LR_LOADMAP3DCOLORS is used, the loaded bitmaps must be at most 8bpp.

@@ +7016,5 @@
> +
> +  ${If} $0 <> 0
> +    System::Call 'user32::GetClientRect(i R0, i r0)'
> +    System::Call '*$0(i, i, i .s, i .s)'
> +    System::Free $0

I think here and on many other calls you can just have System::Call '*(i, i, i .s, i .s)' and remove the System::Free. Please verify with one call though first if you do this change.

@@ +7048,5 @@
> +  Push $0
> +
> +  System::Call 'user32::GetWindowLong(i ${_HANDLE}, i ${GWL_STYLE}) i .r0'
> +  IntOp $0 $0 | ${_STYLE}
> +  IntOp $0 $0 - ${_STYLE}

(neat trick ORing to ensure it's there then subtract.)

@@ +7049,5 @@
> +
> +  System::Call 'user32::GetWindowLong(i ${_HANDLE}, i ${GWL_STYLE}) i .r0'
> +  IntOp $0 $0 | ${_STYLE}
> +  IntOp $0 $0 - ${_STYLE}
> +  System::Call 'user32::SetWindowLongW(i ${_HANDLE}, i ${GWL_STYLE}, i r0)'

nit: user32::SetWindowLong

@@ +7067,5 @@
> +
> +  System::Call 'user32::GetWindowLong(i ${_HANDLE}, i ${GWL_EXSTYLE}) i .r0'
> +  IntOp $0 $0 | ${_EXSTYLE}
> +  IntOp $0 $0 - ${_EXSTYLE}
> +  System::Call 'user32::SetWindowLongW(i ${_HANDLE}, i ${GWL_EXSTYLE}, i r0)'

user32::SetWindowLong

@@ +7093,5 @@
> +!define GetTextExtent "!insertmacro GetTextExtentCall"
> +!define un.GetTextExtent "!insertmacro GetTextExtentCall"
> +
> +!macro GetTextExtent
> +!macroend

I didn't think these empty macros were needed for artificial function calls, but you mentioned they are.  Would be nice to verify they are indeed needed, but if you don't have time I'll take your word for it.

@@ +7098,5 @@
> +
> +!macro un.GetTextExtent
> +!macroend
> +
> +!macro GetTextExtent_

nit: Would be good to document here what you expect on the stack and what you'll be returning.  I know we don't call it externally, but it's nice to have :)

@@ +7110,5 @@
> +  Push $6
> +  Push $7
> +
> +  ; reuse the existing control instead of creating a new one
> +  GetDlgItem $2 $HWNDPARENT 1028

What control ID does 1028 associate to? Please either comment or do a define that is used here and below

@@ +7113,5 @@
> +  ; reuse the existing control instead of creating a new one
> +  GetDlgItem $2 $HWNDPARENT 1028
> +
> +  System::Call 'user32::GetDC(i r2) i .r3'
> +  System::Call 'gdi32::SelectObject(i r3, i r0)'

This already exists in SysFunc.nsh, I'm ok with you leaving it as is but just pointing it out.  Could be called with System::Call "${sysSelectObject} (R3, R0)

@@ +7161,5 @@
> +
> +!macro un.GetDlgItemWidthHeight
> +!macroend
> +
> +!macro GetDlgItemWidthHeight_

nit: Document what you expect on the stack and what you're returning.

@@ +7167,5 @@
> +  Push $1
> +  Push $2
> +
> +  System::Call '*(i, i, i, i) i .r2'
> +  System::Call 'user32::GetClientRect(i r0, i r2)'

nit: Add a comment that GetClientRect always has top and left values of 0 so this works.

@@ +7201,5 @@
> +
> +!macro un.GetDlgItemEndPX
> +!macroend
> +
> +!macro GetDlgItemEndPX_

nit: doc stack usage

@@ +7206,5 @@
> +  Exch $0 ; handle of control
> +  Push $1
> +  Push $2
> +
> +  FindWindow $1 "#32770" "" $HWNDPARENT

Comment that #32770 is the dialog class

@@ +7283,5 @@
> +      ${If} ${Errors}
> +        ; When there aren't newlines in the text calculate the size of the
> +        ; rectangle needed for the text with a minimum of three lines of text.
> +        ClearErrors
> +        System::Call 'user32::DrawTextW(i r4, t $\"$2$\", i r5, i r6, \

user32::DrawText right? (and the 2 below cases)

@@ +7366,5 @@
> +  !verbose pop
> +!macroend
> +
> +!macro un.GetTextWidthHeight
> +  !ifndef un.GetTextWidthHeight

Shouldn't this ifndef go above the !macro? or I think it can just be removed.

Comment 82

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 662096 [details] [diff] [review]
9. stub patch in progress

FYI, to get the stub uploaded by 'make upload' you'll need to add it to
 http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/packager.mk#980

Comment 83

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

> ::: toolkit/mozapps/installer/windows/nsis/common.nsh
> @@ +6985,5 @@
> > +!define /math PBM_SETRANGE32 ${WM_USER} + 6
> > +
> > +!define SHACF_FILESYSTEM 1
> > +
> > +!define MOZ_LOADTRANSPARENT ${LR_LOADFROMFILE}|${LR_LOADTRANSPARENT}|${LR_LOADMAP3DCOLORS}
> 
> nit: You may want to document somewhere that since LR_LOADMAP3DCOLORS is
> used, the loaded bitmaps must be at most 8bpp.
Will do

> @@ +7016,5 @@
> > +
> > +  ${If} $0 <> 0
> > +    System::Call 'user32::GetClientRect(i R0, i r0)'
> > +    System::Call '*$0(i, i, i .s, i .s)'
> > +    System::Free $0
> 
> I think here and on many other calls you can just have System::Call '*(i, i,
> i .s, i .s)' and remove the System::Free. Please verify with one call though
> first if you do this change.
I'll check though I don't think so. Also, it is allocated earlier in the code.

> @@ +7049,5 @@
> > +
> > +  System::Call 'user32::GetWindowLong(i ${_HANDLE}, i ${GWL_STYLE}) i .r0'
> > +  IntOp $0 $0 | ${_STYLE}
> > +  IntOp $0 $0 - ${_STYLE}
> > +  System::Call 'user32::SetWindowLongW(i ${_HANDLE}, i ${GWL_STYLE}, i r0)'
> 
> nit: user32::SetWindowLong
There are other cases where this file will only work on Unicode and I intended to just use wide calls so it is obvious. Would you have a problem with just fixing those cases where it isn't explicitly using wide calls?

> @@ +7093,5 @@
> > +!define GetTextExtent "!insertmacro GetTextExtentCall"
> > +!define un.GetTextExtent "!insertmacro GetTextExtentCall"
> > +
> > +!macro GetTextExtent
> > +!macroend
> 
> I didn't think these empty macros were needed for artificial function calls,
> but you mentioned they are.  Would be nice to verify they are indeed needed,
> but if you don't have time I'll take your word for it.
I'm 99% sure that I verified this several weeks ago and I'll check again.

> @@ +7098,5 @@
> > +
> > +!macro un.GetTextExtent
> > +!macroend
> > +
> > +!macro GetTextExtent_
> 
> nit: Would be good to document here what you expect on the stack and what
> you'll be returning.  I know we don't call it externally, but it's nice to
> have :)
Will do for internal macro documentation inside the macro since it is internal.

> @@ +7110,5 @@
> > +  Push $6
> > +  Push $7
> > +
> > +  ; reuse the existing control instead of creating a new one
> > +  GetDlgItem $2 $HWNDPARENT 1028
> 
> What control ID does 1028 associate to? Please either comment or do a define
> that is used here and below
An NSIS control we don't use... Will doc it.

> @@ +7161,5 @@
> > +
> > +!macro un.GetDlgItemWidthHeight
> > +!macroend
> > +
> > +!macro GetDlgItemWidthHeight_
> 
> nit: Document what you expect on the stack and what you're returning.
> 
> @@ +7167,5 @@
> > +  Push $1
> > +  Push $2
> > +
> > +  System::Call '*(i, i, i, i) i .r2'
> > +  System::Call 'user32::GetClientRect(i r0, i r2)'
> 
> nit: Add a comment that GetClientRect always has top and left values of 0 so
> this works.
Will do

> @@ +7206,5 @@
> > +  Exch $0 ; handle of control
> > +  Push $1
> > +  Push $2
> > +
> > +  FindWindow $1 "#32770" "" $HWNDPARENT
> 
> Comment that #32770 is the dialog class
Will do

> @@ +7366,5 @@
> > +  !verbose pop
> > +!macroend
> > +
> > +!macro un.GetTextWidthHeight
> > +  !ifndef un.GetTextWidthHeight
> 
> Shouldn't this ifndef go above the !macro? or I think it can just be removed.
No, that is for checking if it is defined using !define (see other usage in this file). When checking if a macro is defined !ifmacrodef and ifmacrondef are used.

Comment 84

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

Attachment: 7. Common code

I documented what is on Exch on the stack as appropriate. I didn't document what the individual pushed / popped registers since they are used often used for multiple things and it can be determined by reading the code. I removed the empty macros... not sure why the usage I looked at in the NSIS include directory did this. I went with wide calls since some parts of common.nsh require unicode, already uses wide calls, and I'd prefer it to be explicit.

Comment 85

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

(In reply to Robert Strong [:rstrong] (do not email) from comment #83)
>...
> > Shouldn't this ifndef go above the !macro? or I think it can just be removed.
> No, that is for checking if it is defined using !define (see other usage in
> this file). When checking if a macro is defined !ifmacrodef and ifmacrondef
> are used.
btw: aiui this allows the calling of a NSIS Function instead of inserting the same macro multiple times which increases code size.

Comment 86

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

Attachment: 10. stub patch in progress rev2

Almost there... still need to do:
# Write Access check
# Disk space check
# Download Error messagebox
# test Elevation cancel continue for non-admin (should already work)
# alternate non-admin install directory

Comment 87

[Brian R. Bondy [:bbondy]]

Comment on attachment 662468 [details] [diff] [review]
7. Common code

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

> There are other cases where this file will only work on Unicode and I intended 
> to just use wide calls so it is obvious. Would you have a problem with just 
> fixing those cases where it isn't explicitly using wide calls?

Yup I'm fine with that as long as they are all consistent, which they are in this new patch. Thanks!

> re: other clarifications 

Cool, thanks for the info. I'm still learning some parts of NSIS :)

Comment 88

[Brian R. Bondy [:bbondy]]

> re the documentation on the internal macros: 

I knew they were only internal but I just seen them analogous to private member functions in C++ which I still document even if I'm only calling them once on the actual function definition and not on the function call.  NSIS is a bit worse though because you can't have parameters for functions.  

So to validate that the caller is calling correctly, you need to know the order of how to push, and hence have to read more code.  The commenting you did is good, and this doesn't really matter, thanks!

Comment 89

[Brian R. Bondy [:bbondy]]

Comment on attachment 662490 [details] [diff] [review]
10. stub patch in progress rev2

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

I know this is still in progress, just wanted to add a couple quick notes from a quick skim:
- For the configure stuff, someone else needs to review that, maybe khuey.  Please split that out and ask for review sooner than later in case it takes longer to review.
- win32 calls as widechar variants
- Maybe add a "# Required Plugins:" block to the top of the file to be consistent with our other nsis files
- For the min OS version stuff, instead of duplicating that code, could we move the common.nsh same code into a macro and then insert it at both places?

Comment 90

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

Attachment: 8. Locale support

Replaced ’ from the strings provided with '... carrying forward r+

Comment 91

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

Attachment: 9. configure changes

Kyle, I need a new configure option for the stub installer... then it can be ifdef'd so app's that don't want it don't build it when running the Windows make installer.

Comment 92

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

Attachment: 5. binary resource containing the new ui for checkin rev2

Fixed the tabbing order for the buttons and checkbox. Carrying forward r+

Comment 93

[(not currently active) Ted Mielczarek]

Comment on attachment 662831 [details] [diff] [review]
9. configure changes

FWIW, for options that normal developers don't need to fiddle with we generally prefer to skip the configure option and just require an env var to be set in confvars.sh and then AC_DEFINE/AC_SUBST it. This reduces the number of potential things for people to find in `configure --help` and fiddle.

Comment 94

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

That works for me

Comment 95

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

Attachment: 9. configure changes

Minimal configure changes per comment #93

Ted, could you review this?

Comment 96

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

(In reply to Brian R. Bondy [:bbondy] from comment #62)
> Comment on attachment 661695 [details]
> 1. CertCheck dll for checkin
> 
> r+ assuming this was built with VC6
Found an old system of mine with VC6 on it. Yay!

Comment 97

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

Attachment: 2.extra InetBgDL NSIS plug-in code

I had to fix a couple of bugs in the InetBgDL NSIS plugin code. There is a bunch of functionality I plan on adding to this in the future but that will have to wait until the future. :(

Comment 98

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

btw: I fixed style here and there but not consistently throughout the files.

Comment 99

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

Attachment: 2. InetBgDL NSIS plug-in dll for checkin

Resultant dll only to be checked in after attachment #663904 [details] [diff] [review] is r+

Comment 100

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

Attachment: 1.extra CertCheck NSIS plugin code rev3

With changes

Comment 101

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

Attachment: 1. CertCheck NSIS plugin dll for checkin

Compiled with VC6

Comment 102

[Brian R. Bondy [:bbondy]]

Comment on attachment 663905 [details] [diff] [review]
2. InetBgDL NSIS plug-in dll for checkin

/me 's binary review skills are a little rusty but looks good :)

Comment 103

[(not currently active) Ted Mielczarek]

As a followup it'd be nice to figure out an easy way to build these plugins without resorting to VC6.

Comment 104

[Brian R. Bondy [:bbondy]]

Comment on attachment 663904 [details] [diff] [review]
2.extra InetBgDL NSIS plug-in code

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

I know most of this is not your code, but several things still seem wrong or needs tweaking.  Everything is pretty small though.

::: other-licenses/nsis/Contrib/InetBgDL/InetBgDL.cpp
@@ +34,5 @@
> +  g_N_CCH = N_CCH; \
> +  g_N_Vars = N_Vars; \
> +  } while(0)
> +
> +#define Stats_SetWin32Err(ec) g_cbCurrXF=(ec)

This doesn't make sense to me, we don't call it often but why would we be setting the current transferred to the last error?
I think this should be either removed or set to:
#define Stats_SetWin32Err(ec) SetLastError(ec)
It looks to me like this was supposed to both SetLastError AND store a different global value that would then get reported inside the stats call to registry, but it seems like it was never implemented.

@@ +79,5 @@
> +#ifndef ONELOCKTORULETHEMALL
> +  StatsLock_AcquireExclusive();
> +#endif
> +  g_FilesTotal = 0; // This causes the Task thread to exit the transfer loop
> +  if (WAIT_OBJECT_0 != WaitForSingleObject(g_hThread, 10 * 1000))

Is 10 seconds long enough here?  It's not clear to me if this is only called on error (NSPIM_UNLOAD and /RESET)

@@ +83,5 @@
> +  if (WAIT_OBJECT_0 != WaitForSingleObject(g_hThread, 10 * 1000))
> +  {
> +    TerminateThread(g_hThread, ERROR_OPERATION_ABORTED);
> +  }
> +  CloseHandle(g_hThread);

if (g_hTHread) {
  CloseHandle(g_hThread);
}

@@ +110,5 @@
> +    Reset();
> +    /*
> +    **  Should do cleanup here but the process is about to die so we don't care
> +    **
> +    CloseHandle(g_hThread);

This can be removed since it's already closed in Reset();

@@ +125,5 @@
> +  HINTERNET hInetSes = NULL;
> +  HANDLE hLocalFile;
> +  bool completedFile = false;
> +startnexttask:
> +  hLocalFile = NULL;

hLocalFile = INVALID_HANDLE_VALUE;

@@ +172,5 @@
> +      g_Status = STATUS_ERR_GETLASTERROR;
> +      // if (pFile) DeleteFile(pFile->text);
> +      StatsLock_ReleaseExclusive();
> +    }
> +    InternetCloseHandle(hInetSes);

For the case directly after InternetOpen...
if (hInetes) {
 InternetCloseHandle(hInetSes);
}

@@ +173,5 @@
> +      // if (pFile) DeleteFile(pFile->text);
> +      StatsLock_ReleaseExclusive();
> +    }
> +    InternetCloseHandle(hInetSes);
> +    CloseHandle(hLocalFile);

For the case after the CreateFile call directly...
if (INVALID_HANDLE_VALUE != hLocalFile) {
  CloseHandle(hLocalFile);
}

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

I like that we're setting the user agent like this so we can track what's coming from the stub installer.
But shouldn't we also be setting the operating system and operating system version in the user agent string?
I know we can track the downloads of the actual stub, but it may be interesting to see what the difference is between stub downloading from a browser, and actual installer downloading from the stub. Maybe you'd see for example that on some version of some operating system that it leads to incomplete downloads.

@@ +232,5 @@
> +    const UINT cbBufXF = 1024 * sizeof(TCHAR);
> +    BYTE*bufXF = (BYTE*) pURL->text;
> +#endif
> +    DWORD cbio,cbXF;
> +    BOOL retXF = InternetReadFile(hInetFile, bufXF, cbBufXF, &cbio);

Just calling this out, InternetReadFile can return insufficient buffer if the buffer is not large enough to contain 1 line of the returned stream.  If we expect more than a 1024 character line, maybe the buffer above should be increased.

@@ +251,5 @@
> +      {
> +        // Reading without StatsLock is ok in this thread...
> +        if (g_cbCurrXF != cbThisFile)
> +        {
> +          ec = ERROR_BROKEN_PIPE;

This doesn't really make sense to me.  If InternetReadFile returns TRUE, which it does in this case, and 0 is returned, then the download is complete.  Regardless of wethr or not the file size is known that we are downloading.

@@ +277,5 @@
> +      }
> +
> +      StatsLock_AcquireExclusive();
> +      if (FILESIZE_UNKNOWN != cbThisFile) {
> +        g_cbCurrTot = cbThisFile;

This condition seems like it doesn't matter, since if cbThisFile is set to FILESIZE_UNKNOWN so is g_cbCurrTot, but I'm fine with keeping it.

@@ +287,5 @@
> +
> +  TRACE2(_T("TaskThreadProc completed %s, ec=%u\n"),pURL->text,ec);
> +  InternetCloseHandle(hInetFile);
> +  if (ERROR_SUCCESS == ec) {
> +    CloseHandle(hLocalFile);

nit: I don't think this is needed but would be nice just in case
if (INVALID_HANDLE_VALUE != hLocalFile) {
  CloseHandle(hLocalFile);
  hLocalFile = INVALID_HANDLE_VALUE;
}

@@ +347,5 @@
> +    if (!g_hThread)
> +    {
> +      DWORD tid;tid;
> +      g_hThread = CreateThread(NULL, 0, TaskThreadProc, NULL, 0,
> +                               sizeof(TCHAR) > 1 ? NULL : &tid);

nit: I heard of a problem on win98 with passing NULL here, but let's just either have the tread id returned or not.  Probably just always have the thread id returned.

@@ +348,5 @@
> +    {
> +      DWORD tid;tid;
> +      g_hThread = CreateThread(NULL, 0, TaskThreadProc, NULL, 0,
> +                               sizeof(TCHAR) > 1 ? NULL : &tid);
> +      SetThreadPriority(g_hThread, THREAD_PRIORITY_LOWEST);

if (!g_hThread) {
   goto freeurlandexit;
}

SetThreadPriority(g_hThread, THREAD_PRIORITY_LOWEST);
What is the motivation for setting the thread priority as lowest here?  I think I'd prefer no such call.

Comment 105

[Brian R. Bondy [:bbondy]]

(In reply to Ted Mielczarek [:ted] from comment #103)
> As a followup it'd be nice to figure out an easy way to build these plugins
> without resorting to VC6.

Do you mean as part of the build process?
You can build with vs2010 but the only down side is that the file size will be larger.

Comment 106

[(not currently active) Ted Mielczarek]

Ah, ok, is that the sole reason you did that? It'd be nice to document it either way.

Comment 107

[Brian R. Bondy [:bbondy]]

(In reply to Ted Mielczarek [:ted] from comment #106)
> Ah, ok, is that the sole reason you did that? It'd be nice to document it
> either way.

Yup, yup.

Comment 108

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

(In reply to Brian R. Bondy [:bbondy] from comment #104)
>...
> ::: other-licenses/nsis/Contrib/InetBgDL/InetBgDL.cpp
> @@ +34,5 @@
> > +  g_N_CCH = N_CCH; \
> > +  g_N_Vars = N_Vars; \
> > +  } while(0)
> > +
> > +#define Stats_SetWin32Err(ec) g_cbCurrXF=(ec)
> 
> This doesn't make sense to me, we don't call it often but why would we be
> setting the current transferred to the last error?
> I think this should be either removed or set to:
> #define Stats_SetWin32Err(ec) SetLastError(ec)
> It looks to me like this was supposed to both SetLastError AND store a
> different global value that would then get reported inside the stats call to
> registry, but it seems like it was never implemented.
Done

> @@ +79,5 @@
> > +#ifndef ONELOCKTORULETHEMALL
> > +  StatsLock_AcquireExclusive();
> > +#endif
> > +  g_FilesTotal = 0; // This causes the Task thread to exit the transfer loop
> > +  if (WAIT_OBJECT_0 != WaitForSingleObject(g_hThread, 10 * 1000))
> 
> Is 10 seconds long enough here?  It's not clear to me if this is only called
> on error (NSPIM_UNLOAD and /RESET)
It returns almost immediately for me now though I found a case where the lock was preventing it which is now fixed.

> @@ +83,5 @@
> > +  if (WAIT_OBJECT_0 != WaitForSingleObject(g_hThread, 10 * 1000))
> > +  {
> > +    TerminateThread(g_hThread, ERROR_OPERATION_ABORTED);
> > +  }
> > +  CloseHandle(g_hThread);
> 
> if (g_hTHread) {
>   CloseHandle(g_hThread);
> }
Done

> @@ +110,5 @@
> > +    Reset();
> > +    /*
> > +    **  Should do cleanup here but the process is about to die so we don't care
> > +    **
> > +    CloseHandle(g_hThread);
> 
> This can be removed since it's already closed in Reset();
Removed the commented out code

> @@ +125,5 @@
> > +  HINTERNET hInetSes = NULL;
> > +  HANDLE hLocalFile;
> > +  bool completedFile = false;
> > +startnexttask:
> > +  hLocalFile = NULL;
> 
> hLocalFile = INVALID_HANDLE_VALUE;
Done

> @@ +172,5 @@
> > +      g_Status = STATUS_ERR_GETLASTERROR;
> > +      // if (pFile) DeleteFile(pFile->text);
> > +      StatsLock_ReleaseExclusive();
> > +    }
> > +    InternetCloseHandle(hInetSes);
> 
> For the case directly after InternetOpen...
> if (hInetes) {
>  InternetCloseHandle(hInetSes);
> }
Done

> @@ +173,5 @@
> > +      // if (pFile) DeleteFile(pFile->text);
> > +      StatsLock_ReleaseExclusive();
> > +    }
> > +    InternetCloseHandle(hInetSes);
> > +    CloseHandle(hLocalFile);
> 
> For the case after the CreateFile call directly...
> if (INVALID_HANDLE_VALUE != hLocalFile) {
>   CloseHandle(hLocalFile);
> }
Done

> @@ +181,5 @@
> > +  }
> > +
> > +  if (!hInetSes)
> > +  {
> > +    hInetSes = InternetOpen(USERAGENT,INTERNET_OPEN_TYPE_PRECONFIG,NULL,NULL,0);
> 
> I like that we're setting the user agent like this so we can track what's
> coming from the stub installer.
> But shouldn't we also be setting the operating system and operating system
> version in the user agent string?
> I know we can track the downloads of the actual stub, but it may be
> interesting to see what the difference is between stub downloading from a
> browser, and actual installer downloading from the stub. Maybe you'd see for
> example that on some version of some operating system that it leads to
> incomplete downloads.
We will be sending a ping with this info from stub.nsi so I don't really see the need for that here.

> @@ +232,5 @@
> > +    const UINT cbBufXF = 1024 * sizeof(TCHAR);
> > +    BYTE*bufXF = (BYTE*) pURL->text;
> > +#endif
> > +    DWORD cbio,cbXF;
> > +    BOOL retXF = InternetReadFile(hInetFile, bufXF, cbBufXF, &cbio);
> 
> Just calling this out, InternetReadFile can return insufficient buffer if
> the buffer is not large enough to contain 1 line of the returned stream.  If
> we expect more than a 1024 character line, maybe the buffer above should be
> increased.
Increased to 4096

> @@ +251,5 @@
> > +      {
> > +        // Reading without StatsLock is ok in this thread...
> > +        if (g_cbCurrXF != cbThisFile)
> > +        {
> > +          ec = ERROR_BROKEN_PIPE;
> 
> This doesn't really make sense to me.  If InternetReadFile returns TRUE,
> which it does in this case, and 0 is returned, then the download is
> complete.  Regardless of wethr or not the file size is known that we are
> downloading.
I'm leaving this here for now since we our servers provide file size and I'd like to think about this before removing it.

> @@ +277,5 @@
> > +      }
> > +
> > +      StatsLock_AcquireExclusive();
> > +      if (FILESIZE_UNKNOWN != cbThisFile) {
> > +        g_cbCurrTot = cbThisFile;
> 
> This condition seems like it doesn't matter, since if cbThisFile is set to
> FILESIZE_UNKNOWN so is g_cbCurrTot, but I'm fine with keeping it.
Kept

> @@ +287,5 @@
> > +
> > +  TRACE2(_T("TaskThreadProc completed %s, ec=%u\n"),pURL->text,ec);
> > +  InternetCloseHandle(hInetFile);
> > +  if (ERROR_SUCCESS == ec) {
> > +    CloseHandle(hLocalFile);
> 
> nit: I don't think this is needed but would be nice just in case
> if (INVALID_HANDLE_VALUE != hLocalFile) {
>   CloseHandle(hLocalFile);
>   hLocalFile = INVALID_HANDLE_VALUE;
> }
Done

> @@ +347,5 @@
> > +    if (!g_hThread)
> > +    {
> > +      DWORD tid;tid;
> > +      g_hThread = CreateThread(NULL, 0, TaskThreadProc, NULL, 0,
> > +                               sizeof(TCHAR) > 1 ? NULL : &tid);
> 
> nit: I heard of a problem on win98 with passing NULL here, but let's just
> either have the tread id returned or not.  Probably just always have the
> thread id returned.
Done

> 
> @@ +348,5 @@
> > +    {
> > +      DWORD tid;tid;
> > +      g_hThread = CreateThread(NULL, 0, TaskThreadProc, NULL, 0,
> > +                               sizeof(TCHAR) > 1 ? NULL : &tid);
> > +      SetThreadPriority(g_hThread, THREAD_PRIORITY_LOWEST);
> 
> if (!g_hThread) {
>    goto freeurlandexit;
> }
> 
> SetThreadPriority(g_hThread, THREAD_PRIORITY_LOWEST);
> What is the motivation for setting the thread priority as lowest here?  I
> think I'd prefer no such call.
Done

Comment 109

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

Attachment: 2.extra InetBgDL NSIS plug-in code rev2

Comment 110

[Brian R. Bondy [:bbondy]]

Comment on attachment 664411 [details] [diff] [review]
2.extra InetBgDL NSIS plug-in code rev2

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

r=me with the nits. Thanks!

::: other-licenses/nsis/Contrib/InetBgDL/InetBgDL.cpp
@@ +243,5 @@
> +
> +      TRACE2(_T("InternetReadFile true with 0 cbio, cbThisFile=%d gle=%u\n"), cbThisFile, GetLastError());
> +      if (FILESIZE_UNKNOWN != cbThisFile)
> +      {
> +        // Reading without StatsLock is ok in this thread...

nit: This comment is confusing here, maybe remove it?

@@ +244,5 @@
> +      TRACE2(_T("InternetReadFile true with 0 cbio, cbThisFile=%d gle=%u\n"), cbThisFile, GetLastError());
> +      if (FILESIZE_UNKNOWN != cbThisFile)
> +      {
> +        // Reading without StatsLock is ok in this thread...
> +        if (g_cbCurrXF != cbThisFile)

nit 2: Please combine these 2 conditions  
if (FILESIZE_UNKNOWN != cbThisFile && g_cbCurrXF != cbThisFile) {
}

@@ +246,5 @@
> +      {
> +        // Reading without StatsLock is ok in this thread...
> +        if (g_cbCurrXF != cbThisFile)
> +        {
> +          ec = ERROR_BROKEN_PIPE;

OK I understand this condition now.  Please add a comment above the new combined condition above something like:
// If we haven't transferred all of the file, and we know how big the file is, and we
// have no more data to read from the HTTP request, then set a broken pipe error.

Comment 111

[Brian R. Bondy [:bbondy]]

Actually you can keep that comment but combine it with the other suggested ones pls.

Comment 112

[Brian R. Bondy [:bbondy]]

> We will be sending a ping with this info from stub.nsi so I 
> don't really see the need for that here.

OK cool I wasn't sure how it was being tracked, that sounds good.

Comment 113

[Madhava Enros [:madhava]]

(In reply to Brian R. Bondy [:bbondy] from comment #79)

> > +INTRO_BLURB=Thanks for choosing $BrandFullName, the browser that chooses you above everything else. Click to install.
> > +INSTALL_BLURB1=You're about to enjoy the very latest in speed, flexibility and security so you're always in control.
> > +INSTALL_BLURB2=That’s because $BrandShortName is made by a non-profit to make browsing and the Web better for you.
> > +INSTALL_BLURB3=You’re also joining a global community of users, contributors and developers working to make the best browser in the world.
> 
> nit: INSTALL_BLURB3=You’re also joining a global community of users,
> contributors, and developers working to make the best browser in the world.
> (extra comma before and is preferred IMHO)

I, personally, prefer the oxford comma, too. But, from our writing styleguide http://www.mozilla.org/en-US/styleguide/communications/copy-rules/

"we do not use serial commas (also called Oxford commas) in Mozilla communications"

Comment 114

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

Attachment: 2.extra InetBgDL NSIS plug-in code rev3

Addressed review comments.

I'll recompile the dll with VC6 and attach the patch for it soon.

Comment 115

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

Attachment: 2. InetBgDL NSIS plug-in dll for checkin

Comment 116

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

Attachment: 11. main patch rev1

Brian, I still need to finish the
1. messagebox to download the full installer on failure
2. set as default (will probably just launch the helper to do this)
3. taskbar / quicklaunch shortcut creation (started but commented out)

I'll finish these up in a new patch.

The scariest and biggest PITA part of this code is the install directory checks so please pay close attention to it when reviewing. I don't want to use the common functions for this since they are buggy and we can't use the builtin capabilities since that only applies to the instfiles page.

I didn't bother adding the urls for the plugins since we might as well bring them all into the tree.

Comment 117

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

btw: I'll submit a followup patch for the additional items and possibly to cover review comments as well in the hope that it makes it easier to review.

Comment 118

[Brian R. Bondy [:bbondy]]

Comment on attachment 665375 [details] [diff] [review]
11. main patch rev1

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

- There's some win8 CheckboxShortcutInStartMenu handling already on m-c that needs to be brought into this.  This will go live while win8 is released so I think it should be added now.
- Ditto CheckboxSetAsDefault
- You mentioned you were going to launch the helper.exe for defaults handling.   This will probably need a secreview since I think it'll be done after being elevated.  Whereas when we call it from firefox.exe we're a medium integrity process, so that would be more OK for dll injection.  Alternatively call the common code directly to reduce a security risk.
- You mentioned these are pending: messagebox, taskbar/quicklaunch
- If possible I'd appreciate a zip of your patch queue and series file so I can try this out on my machine, all of my reviews have been code only and without me actually seeing it thus far.  I find that actually trying the code makes me think of more places to probe the code on.

I would like another pass at this after the changes before r+ing.

::: browser/installer/windows/nsis/stub.nsi
@@ +109,5 @@
> +ChangeUI all "nsisui.exe"
> +!ifdef HAVE_64BIT_OS
> +  InstallDir "$PROGRAMFILES64\${BrandFullName}\"
> +!else
> +  InstallDir "$PROGRAMFILES32\${BrandFullName}\"

Have you considered using InstallDirRegKey instead of all the custom handling with InitialInstallDir? I haven't tested to see if it works as expected with the back button but it would be simpler.  You already have the code in place so I'm not suggesting changing it now, just wondering what the motivation was for the change.

@@ +140,5 @@
> +
> +Page custom createDummy ; Needed to enable the Intro page's back button
> +Page custom createIntro leaveIntro
> +Page custom createOptions leaveOptions
> +Page custom createInstall leaveInstall

nit: Would be nice to have a quick comment description of what each page is.

@@ +153,5 @@
> +  ${Unless} ${RunningX64}
> +  ${OrUnless} ${AtLeastWinVista}
> +    MessageBox MB_OK|MB_ICONSTOP "$(WARN_MIN_SUPPORTED_OS_MSG)" IDOK
> +    ; Nothing initialized so no need to call OnEndCommon
> +    Quit

nit: Would be nice to have a comment just after !ifdef HAVE_64BIT_OS that says:
; Restrict x64 builds from being installed on x86 and pre Vista

@@ +252,5 @@
> +Function createDummy
> +FunctionEnd
> +
> +Function createIntro
> +  ; If Back is clicked on the options page reset variables

Is it better to reset variables here or preserve them from what the user selected.
I think if I pressed back I'd be doing it because I wanted to make a slight change of something I did in the last screen.
I'm sure you already thought about this in detail though, so I'm fine with it as is.

@@ +518,5 @@
> +  ${GetLongPath} "$0" $0
> +  ${If} "$INSTDIR" == "$0"
> +    LockWindow off
> +    MessageBox MB_OK|MB_ICONEXCLAMATION "$(WARN_ROOT_INSTALL)"
> +    Abort

nit: Comment before the Abort call
; Stay on the page

@@ +525,5 @@
> +  Call CanWrite
> +  ${If} "$CanWriteToInstallDir" == "false"
> +    LockWindow off
> +    MessageBox MB_OK|MB_ICONEXCLAMATION "$(WARN_WRITE_ACCESS)"
> +    Abort

nit: ;Stay on the page

@@ +532,5 @@
> +  Call CheckSpace
> +  ${If} "$HasRequiredSpaceAvailable" == "false"
> +    LockWindow off
> +    MessageBox MB_OK|MB_ICONEXCLAMATION "$(WARN_DISK_SPACE)"
> +    Abort

nit: ;Stay on the page

@@ +701,5 @@
> +FunctionEnd
> +
> +Function StartDownload
> +  ${NSD_KillTimer} StartDownload
> +  InetBgDL::Get "${URLStubDownload}" "$PLUGINSDIR\download.exe" /END

Please check with security if you think it's needed on the WinINet flag INTERNET_FLAG_IGNORE_REDIRECT_TO_HTTP that this uses.
It allows https to http redirects silently.  I don't think it matters though since we're just downloading an installer, but maybe there's a privacy issue there (although I personally think it's fine).

Also I think this call will silently handle HTTP redirects because INTERNET_FLAG_NO_AUTO_REDIRECT i not set. And I think that it will automatically handle things like chunked encoding transparently.  Please let me know if you think that's not the case.

@@ +716,5 @@
> +  # $1 = Completed files
> +  # $2 = Remaining files
> +  # $3 = Number of downloaded bytes for the current file
> +  # $4 = Size of current file (Empty string if the size is unknown)
> +  ${If} $0 > 299

Please add a comment here explaining that 300s and 100s error codes should be handled automatically by the WinINet calls inside InetBgDL.

@@ +775,5 @@
> +
> +      CertCheck::VerifyCertTrust "$PLUGINSDIR\download.exe"
> +      Pop $0
> +      CertCheck::VerifyCertNameIssuer "$PLUGINSDIR\download.exe" \
> +                                      "${CertNameDownload}" "${CertIssuerDownload}"

Please have security sign off on this, I know there was concern for the maintenanceservice where I had to refuse write access to the file I'd be executing from a high integrity process.  In this case you'd open download.exe without write sharing before doing these 2 calls.  And keep it open until after it is executed.  This is to protect against someone overwritting the binary just after the check but before executing it.  A low integrity process could do this in theory and escalate to a high integrity process.

I think this is more unlikely for your case though since the installer location is not at a known location beforehand by an attacker. So personally I'd be fine with it.

It even went a step further with the maintenanceservice where we had to worry about network drives, in which case we currently just skip using the maintenance service, but I don't think it has to go that far in my opinion for this.

I'd be fine if this was posted to a diff bug and landed independently in some given time frame that security signs off on.

@@ +830,5 @@
> +          Rename "$INSTDIR\freebl3.dll" "$INSTDIR\${TO_BE_DELETED}\freebl3.dll"
> +          Rename "$INSTDIR\AccessibleMarshal.dll" "$INSTDIR\${TO_BE_DELETED}\AccessibleMarshal.dll"
> +        ${EndIf}
> +
> +        Exec "$PLUGINSDIR\download.exe /INI=$PLUGINSDIR\${CONFIG_INI}"

It's my understanding that this can have a DLL injection attack iff the main installer without stub has one.  (Because they're identical)
If that's not the case please request a security review for this.

@@ +877,5 @@
> +Function OnBack
> +  StrCpy $WasOptionsButtonClicked "true"
> +  StrCpy $R9 1 ; Goto the next page
> +  Call RelativeGotoPage
> +  Abort

Can you comment a bit more on what this does? I don't fully understand what happens with this Abort call.

@@ +1033,5 @@
> +  ${If} ${FileExists} "$INSTDIR"
> +    ; Use the existing directory when it exists
> +    StrCpy $0 "$INSTDIR"
> +  ${Else}
> +    ; Get the topmost directory that exists for new installs

What is $0 set to in this case, is it always set?

@@ +1035,5 @@
> +    StrCpy $0 "$INSTDIR"
> +  ${Else}
> +    ; Get the topmost directory that exists for new installs
> +    ${DoUntil} ${FileExists} "$0"
> +      ${GetParent} "$0" $0

What if this is an invalid path that has no slasshes, maybe a check for errors and a break?

@@ +1041,5 @@
> +  ${EndIf}
> +
> +  GetTempFileName $2 "$0"
> +  Delete $2
> +  CreateDirectory "$2"

I think the inherited security attributes for a directory is the same as for a file, but I'm fine with doing a temp directory + a temp file.

@@ +1052,5 @@
> +    ${If} ${FileExists} "$3"
> +      Delete "$3"
> +      StrCpy $CanWriteToInstallDir "true"
> +    ${Else}
> +      StrCpy $CanWriteToInstallDir "false"

This should already be set, I think you can remove this Else condition.

Comment 119

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

Couple of quick replies to comment #118

> @@ +252,5 @@
> > +Function createDummy
> > +FunctionEnd
> > +
> > +Function createIntro
> > +  ; If Back is clicked on the options page reset variables
> 
> Is it better to reset variables here or preserve them from what the user
> selected.
> I think if I pressed back I'd be doing it because I wanted to make a slight
> change of something I did in the last screen.
> I'm sure you already thought about this in detail though, so I'm fine with
> it as is.
The difference in this case is that we don't have anything on the previous page that can be set that isn't already on that page.

> @@ +1041,5 @@
> > +  ${EndIf}
> > +
> > +  GetTempFileName $2 "$0"
> > +  Delete $2
> > +  CreateDirectory "$2"
> 
> I think the inherited security attributes for a directory is the same as for
> a file, but I'm fine with doing a temp directory + a temp file.
Agreed. We had some bug reports with squirrelly permissions for the non-inherited case which is why both are done.

Comment 120

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

(In reply to Brian R. Bondy [:bbondy] from comment #118)
> Comment on attachment 665375 [details] [diff] [review]
>...
> - There's some win8 CheckboxShortcutInStartMenu handling already on m-c that
> needs to be brought into this.  This will go live while win8 is released so
> I think it should be added now.
> - Ditto CheckboxSetAsDefault
Agreed

> - You mentioned you were going to launch the helper.exe for defaults
> handling.   This will probably need a secreview since I think it'll be done
> after being elevated.  Whereas when we call it from firefox.exe we're a
> medium integrity process, so that would be more OK for dll injection. 
> Alternatively call the common code directly to reduce a security risk.
It will be done from the non-elevated process.

Comment 121

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

> ::: browser/installer/windows/nsis/stub.nsi
> @@ +109,5 @@
> > +ChangeUI all "nsisui.exe"
> > +!ifdef HAVE_64BIT_OS
> > +  InstallDir "$PROGRAMFILES64\${BrandFullName}\"
> > +!else
> > +  InstallDir "$PROGRAMFILES32\${BrandFullName}\"
> 
> Have you considered using InstallDirRegKey instead of all the custom
> handling with InitialInstallDir? I haven't tested to see if it works as
> expected with the back button but it would be simpler.  You already have the
> code in place so I'm not suggesting changing it now, just wondering what the
> motivation was for the change.
It doesn't work with our install method. See code in NO_INSTDIR_FROM_REG blocks in common.nsh for how this was handled though it turns out that it was disabled by defining NO_INSTDIR_FROM_REG at some point on all branches in defines.nsi.in so I am not implementing that here until sometime later after I understand why.

Comment 122

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

Attachment: 11. main patch rev2

Should cover everything but the Win7 taskbar shortcut when not setting as default and the following

> - You mentioned you were going to launch the helper.exe for defaults
> handling.   This will probably need a secreview since I think it'll be done
> after being elevated.  Whereas when we call it from firefox.exe we're a
> medium integrity process, so that would be more OK for dll injection. 
> Alternatively call the common code directly to reduce a security risk.
> - You mentioned these are pending: messagebox, taskbar/quicklaunch
> - If possible I'd appreciate a zip of your patch queue and series file so I
> can try this out on my machine, all of my reviews have been code only and
> without me actually seeing it thus far.  I find that actually trying the
> code makes me think of more places to probe the code on.
> 

> @@ +701,5 @@
> > +FunctionEnd
> > +
> > +Function StartDownload
> > +  ${NSD_KillTimer} StartDownload
> > +  InetBgDL::Get "${URLStubDownload}" "$PLUGINSDIR\download.exe" /END
> 
> Please check with security if you think it's needed on the WinINet flag
> INTERNET_FLAG_IGNORE_REDIRECT_TO_HTTP that this uses.
> It allows https to http redirects silently.  I don't think it matters though
> since we're just downloading an installer, but maybe there's a privacy issue
> there (although I personally think it's fine).
> 
> Also I think this call will silently handle HTTP redirects because
> INTERNET_FLAG_NO_AUTO_REDIRECT i not set. And I think that it will
> automatically handle things like chunked encoding transparently.  Please let
> me know if you think that's not the case.
> 
> @@ +775,5 @@
> > +
> > +      CertCheck::VerifyCertTrust "$PLUGINSDIR\download.exe"
> > +      Pop $0
> > +      CertCheck::VerifyCertNameIssuer "$PLUGINSDIR\download.exe" \
> > +                                      "${CertNameDownload}" "${CertIssuerDownload}"
Needs to lock
Comment that it uses the temp dir so network drives shouldn't be a concern.

> @@ +830,5 @@
> > +          Rename "$INSTDIR\freebl3.dll" "$INSTDIR\${TO_BE_DELETED}\freebl3.dll"
> > +          Rename "$INSTDIR\AccessibleMarshal.dll" "$INSTDIR\${TO_BE_DELETED}\AccessibleMarshal.dll"
> > +        ${EndIf}
> > +
> > +        Exec "$PLUGINSDIR\download.exe /INI=$PLUGINSDIR\${CONFIG_INI}"
> 
> It's my understanding that this can have a DLL injection attack iff the main
> installer without stub has one.  (Because they're identical)
> If that's not the case please request a security review for this.
>

Comment 123

[Brian R. Bondy [:bbondy]]

Comment on attachment 665648 [details] [diff] [review]
11. main patch rev2

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

Looks good although I'll do another pass tomorrow after trying the code.

> Should cover everything but the Win7 taskbar shortcut when not setting as default and the following
I think we still need a message box to download the full on failures too.

::: browser/branding/official/branding.nsi
@@ +14,5 @@
>  !define URLUpdateInfo         "http://www.mozilla.com/${AB_CD}/firefox/"
> +
> +# It isn't possible to get the size of the installation prior to downloading
> +# so the stub installer uses an estimate.
> +!define APPROXIMATE_REQUIRED_SPACE_MB "42.2"

Do you expect this to be significantly different for each brand? Maybe this should be in defines.nsi.in instead?

::: browser/installer/windows/nsis/stub.nsi
@@ +789,5 @@
> +
> +      CertCheck::VerifyCertTrust "$PLUGINSDIR\download.exe"
> +      Pop $0
> +      CertCheck::VerifyCertNameIssuer "$PLUGINSDIR\download.exe" \
> +                                      "${CertNameDownload}" "${CertIssuerDownload}"

I think you mentioned that you agree that download.exe needs to lock before these 2 calls right?
I agree that network drives are not a concern.

@@ +1111,5 @@
> +  ${EndIf}
> +FunctionEnd
> +
> +Function ExecSetAsDefaultAppUser
> +  Exec "$\"$INSTDIR\uninstall\helper.exe$\" /SetAsDefaultAppUser"

I haven't tested this yet, but if we're executing this unelevated, won't helper.exe prompt with UAC? If not how else could it set the HKLM StartMenuInternetKeys.  This would work fine on win8 only as far as I can tell.  Maybe I'm overlooking something?

Comment 124

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

(In reply to Brian R. Bondy [:bbondy] from comment #118)
>...
> > Should cover everything but the Win7 taskbar shortcut when not setting as default and the following
> I think we still need a message box to download the full on failures too.
Yes... accidentally removed that from the what needs to be done list.

> ::: browser/branding/official/branding.nsi
> @@ +14,5 @@
> >  !define URLUpdateInfo         "http://www.mozilla.com/${AB_CD}/firefox/"
> > +
> > +# It isn't possible to get the size of the installation prior to downloading
> > +# so the stub installer uses an estimate.
> > +!define APPROXIMATE_REQUIRED_SPACE_MB "42.2"
> 
> Do you expect this to be significantly different for each brand? Maybe this
> should be in defines.nsi.in instead?
I had it there originally... I'll move it back and if it is needed it can be split out then.

> ::: browser/installer/windows/nsis/stub.nsi
> @@ +789,5 @@
> > +
> > +      CertCheck::VerifyCertTrust "$PLUGINSDIR\download.exe"
> > +      Pop $0
> > +      CertCheck::VerifyCertNameIssuer "$PLUGINSDIR\download.exe" \
> > +                                      "${CertNameDownload}" "${CertIssuerDownload}"
> 
> I think you mentioned that you agree that download.exe needs to lock before
> these 2 calls right?
> I agree that network drives are not a concern.
Right though I haven't delved into that yet. I am also considering using the system's temp directory since I believe it is more secure than the user's.

> @@ +1111,5 @@
> > +  ${EndIf}
> > +FunctionEnd
> > +
> > +Function ExecSetAsDefaultAppUser
> > +  Exec "$\"$INSTDIR\uninstall\helper.exe$\" /SetAsDefaultAppUser"
> 
> I haven't tested this yet, but if we're executing this unelevated, won't
> helper.exe prompt with UAC? If not how else could it set the HKLM
> StartMenuInternetKeys.  This would work fine on win8 only as far as I can
> tell.  Maybe I'm overlooking something?
The required HKLM keys I believe are handled on install but I'll double check.

Comment 125

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

Attachment: 11. main patch rev3

I think this covers everything but the Win7 taskbar shortcut when not setting as default and the UAC prompt when setting as default when installing as a non-admin. I'll see if I can fix these though I don't think it should stop this from landing.

Comment 126

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

I've changed the following locally
Removed
!insertmacro InitHashAppModelId

Modified
System::Call 'kernel32::CreateFileW(t "$DownloadDir\download.exe",
to
System::Call 'kernel32::CreateFileW(t "$DownloadDir\download.exe", \

Comment 127

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

Attachment: 11. main patch rev4

Includes fixes from comment #126

Comment 128

[Brian R. Bondy [:bbondy]]

> Right though I haven't delved into that yet. I am also considering using the 
> system's temp directory since I believe it is more secure than the user's.

I tried to write to it via an unelevated cmd.exe but I still could.
If you do go the route of not locking but instead writing into a new dir at a high integrity location, then that's cool but the dir will be different for the unelevated case.

Comment 129

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

Latest patch locks it as well.

Comment 130

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

Attachment: 12. Fix download dir

Found an issue with setting the download dir

Comment 131

[Brian R. Bondy [:bbondy]]

Comment on attachment 665954 [details] [diff] [review]
12. Fix download dir

Is the win temp dir always called temp on all locales of Windows?  I think NSIS exposes this value with $TEMP.

Comment 132

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

(In reply to Brian R. Bondy [:bbondy] from comment #131)
> Comment on attachment 665954 [details] [diff] [review]
> 12. Fix download dir
> 
> Is the win temp dir always called temp on all locales of Windows?  I think
> NSIS exposes this value with $TEMP.
I believe it is called temp by default (might be possible to change). $TEMP is the user's temp and I could get its name and use it. Then again, I am tempted to go with the user's temp since there is file locking now.

Comment 133

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

Attachment: 12. Remove DownloadDir and use $PLUGINSDIR rev2

Just use file locking... talked with bbondy about this over irc

Comment 134

[Chris AtLee [:catlee]]

Rob, I've created bouncer links for nightly, aurora and beta for testing. These *will* get out of date, but should suffice for now.

http://download.mozilla.org/?product=firefox-nightly-latest&os=win&lang=en-US
http://download.mozilla.org/?product=firefox-aurora-latest&os=win&lang=en-US
http://download.mozilla.org/?product=firefox-beta-latest&os=win&lang=en-US
http://download.mozilla.org/?product=firefox-latest&os=win&lang=en-US

Comment 135

[Brian R. Bondy [:bbondy]]

For the checkbox in preferences, it says:
  Install the Nightly background update service.

Since this is a global service, and to stay consistent with the base non stub installer, and since it wouldn't match existing support documentation, please make this say instead:
  Install Maintenance Service

Comment 136

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

I worked with UX on the text so they need to approve that change.

Comment 137

[Brian R. Bondy [:bbondy]]

OK so if UX agrees for a change, then we have to update the text here.
If there is not a change, we should also:
- update the base installer text to match.
- notify support so they can update their documentation http://support.mozilla.org/en-US/kb/what-mozilla-maintenance-service

Comment 138

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

Perhaps go with "Install the background update service."
and change the documentation to include both for now until the Full installer is changed to the new UI.

Comment 139

[Brian R. Bondy [:bbondy]]

That same checkbox should also only be shown in these conditions:

    Call IsUserAdmin
    Pop $R0
    ${If} $R0 == "true"
    ; Only proceed if we have HKLM write access
    ${AndIf} $TmpVal == "HKLM"
    ; On Windows 2000 we do not install the maintenance service.
    ${AndIf} ${AtLeastWinXP}

Comment 140

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

(In reply to Brian R. Bondy [:bbondy] from comment #139)
> That same checkbox should also only be shown in these conditions:
> 
>     Call IsUserAdmin
>     Pop $R0
>     ${If} $R0 == "true"
>     ; Only proceed if we have HKLM write access
>     ${AndIf} $TmpVal == "HKLM"
>     ; On Windows 2000 we do not install the maintenance service.
>     ${AndIf} ${AtLeastWinXP}
Will add the HKLM and IsUserAdmin requirements.

Our minimum Windows version supported is now Windows XP SP2.

Comment 141

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

(In reply to Chris AtLee [:catlee] from comment #134)
> Rob, I've created bouncer links for nightly, aurora and beta for testing.
> These *will* get out of date, but should suffice for now.
> 
> http://download.mozilla.org/?product=firefox-nightly-latest&os=win&lang=en-US
> http://download.mozilla.org/?product=firefox-aurora-latest&os=win&lang=en-US
> http://download.mozilla.org/?product=firefox-beta-latest&os=win&lang=en-US
> http://download.mozilla.org/?product=firefox-latest&os=win&lang=en-US
Thanks!

Comment 142

[Brian R. Bondy [:bbondy]]

Comment on attachment 665839 [details] [diff] [review]
11. main patch rev4

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

r+ for this patch on condition that in follow up patches we will do:
- As discussed on IRC, currently if you install on top it silently installs and launches firefox.exe.  Instead we should give an error to close firefox like the base installer does.
- As discussed on IRC, the install to dir should be dependent on the brand you're building.  You also mentioned you'd update the brnading.nsi links when you do this, although I think they are independent tasks.
- With a limited user account, I press no to elevate, it defaults to users app data dir which is good, but then I start to install and it prompts me for elevation again.  It should instead not prompt for elevation and only set the HKCU keys. (see below suggestion)

I think it would be good if not already done, to loop a QA person into this bug, and provide them with a build to go over any litmus tests that exist, as well as test other things they can think of.

::: browser/installer/windows/nsis/stub.nsi
@@ +1173,5 @@
> +
> +Function ExecSetAsDefaultAppUser
> +  ; Using the helper.exe lessens the stub installer size.
> +  ; This could ask for elevatation when the user doesn't install as admin.
> +  Exec "$\"$INSTDIR\uninstall\helper.exe$\" /SetAsDefaultAppUser"

What about adding a switch to common.nsh here to call directly into the HKCU variant of this function that doesn't prompt for elevation?

Comment 143

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

(In reply to Brian R. Bondy [:bbondy] from comment #142)
> Comment on attachment 665839 [details] [diff] [review]
> 11. main patch rev4
> 
> Review of attachment 665839 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ for this patch on condition that in follow up patches we will do:
> - As discussed on IRC, currently if you install on top it silently installs
> and launches firefox.exe.  Instead we should give an error to close firefox
> like the base installer does.
Will do

> - As discussed on IRC, the install to dir should be dependent on the brand
> you're building.  You also mentioned you'd update the brnading.nsi links
> when you do this, although I think they are independent tasks.
Done

> - With a limited user account, I press no to elevate, it defaults to users
> app data dir which is good, but then I start to install and it prompts me
> for elevation again.  It should instead not prompt for elevation and only
> set the HKCU keys. (see below suggestion)
This is difficult for a number of reasons and I'd prefer prompting instead so if someone isn't elevated and can we get the registry keys we want and if they don't elevate we just continue. I'll consider other options at a later date.

> I think it would be good if not already done, to loop a QA person into this
> bug, and provide them with a build to go over any litmus tests that exist,
> as well as test other things they can think of.
Jason Smith is looped in.

Comment 144

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #143)
> > - With a limited user account, I press no to elevate, it defaults to users
> > app data dir which is good, but then I start to install and it prompts me
> > for elevation again.  It should instead not prompt for elevation and only
> > set the HKCU keys. (see below suggestion)
> This is difficult for a number of reasons and I'd prefer prompting instead
> so if someone isn't elevated and can we get the registry keys we want and if
> they don't elevate we just continue. I'll consider other options at a later
> date.

I'm fine with either of these 2 things:
- Posting a new bug to make sure unelevated limited user account installs from the stub installer do not have the option to set as default.
- Posting a new bug to make sure unelevated limited user account installs from the base installer do have the option for default users, and do prompt.

I know parity with the old installer is not a requirement, and that's why I'm saying new bug. But I think small differences like this should be avoided when possible.

Comment 145

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

Attachment: Fixes 1 rev1

Removes the checkbox to set as default when we don't have HKLM write access.
Removes the checkbox for the maintenance service for various cases.

Another patch on the way for the firefox already running case.

Comment 146

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

Along with the other requested changes besides the firefox already running case.

Comment 147

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

Attachment: Fixes 2 rev1

Adds the close app notification. Doesn't use the common.nsh macro because that aborts which has been problematic from the custom nsDialogs pages when I tested it.

Comment 148

[Brian R. Bondy [:bbondy]]

Comment on attachment 666045 [details] [diff] [review]
Fixes 1 rev1

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

r=me with that change

::: browser/installer/windows/nsis/stub.nsi
@@ +511,5 @@
> +  ClearErrors
> +  WriteRegStr HKLM "Software\Mozilla" "${BrandShortName}InstallerTest" \
> +                   "Write Test"
> +  ${If} ${Errors}
> +  ${AndIf} $0 != "true"

${OrIf} $0 != "true"

Comment 149

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

Attachment: Fixes 1 rev2

Comment 150

[Brian R. Bondy [:bbondy]]

Comment on attachment 666052 [details] [diff] [review]
Fixes 2 rev1

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

::: browser/installer/windows/nsis/stub.nsi
@@ +1217,5 @@
> +
> +  ; Find the installation directory when launching using GetFunctionAddress
> +  ; from an elevated installer since $INSTDIR will not be set in this installer
> +  ${StrFilter} "${FileMainEXE}" "+" "" "" $R9
> +  ReadRegStr $0 HKLM "Software\Clients\StartMenuInternet\$R9\DefaultIcon" ""

If the user unchecks set as default this will launch the wrong firefox right?

Comment 151

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

(In reply to Brian R. Bondy [:bbondy] from comment #150)
> Comment on attachment 666052 [details] [diff] [review]
> Fixes 2 rev1
> 
> Review of attachment 666052 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/installer/windows/nsis/stub.nsi
> @@ +1217,5 @@
> > +
> > +  ; Find the installation directory when launching using GetFunctionAddress
> > +  ; from an elevated installer since $INSTDIR will not be set in this installer
> > +  ${StrFilter} "${FileMainEXE}" "+" "" "" $R9
> > +  ReadRegStr $0 HKLM "Software\Clients\StartMenuInternet\$R9\DefaultIcon" ""
> 
> If the user unchecks set as default this will launch the wrong firefox right?
No, it always sets these keys if the installer has write permissions and in this specific case we are elevated and hence have write permissions.

Comment 152

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

Pushed to inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2bef7fba06b
https://hg.mozilla.org/integration/mozilla-inbound/rev/17d699699df3
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc19791bc037
https://hg.mozilla.org/integration/mozilla-inbound/rev/be58ccf6c876
https://hg.mozilla.org/integration/mozilla-inbound/rev/359e9233c228
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb61ab26f9ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/33c84d894f23
https://hg.mozilla.org/integration/mozilla-inbound/rev/56c4bfbc32a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2ec58a44647
https://hg.mozilla.org/integration/mozilla-inbound/rev/5da759429ea1
https://hg.mozilla.org/integration/mozilla-inbound/rev/f76ec4fa1f97
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b15c193e5ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/506dc963450f
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e4ad6f5f8cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f7638e3036f
https://hg.mozilla.org/integration/mozilla-inbound/rev/63bbe1bad527

Comment 153

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

Actually the following changeset is for bug 791613
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e4ad6f5f8cf

Comment 154

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

Attachment: Missed !insertmacro GetPathFromString

Landed on inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e52c022991d

Comment 155

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/63bbe1bad527
https://hg.mozilla.org/mozilla-central/rev/3f7638e3036f
https://hg.mozilla.org/mozilla-central/rev/506dc963450f
https://hg.mozilla.org/mozilla-central/rev/6b15c193e5ed
https://hg.mozilla.org/mozilla-central/rev/f76ec4fa1f97
https://hg.mozilla.org/mozilla-central/rev/5da759429ea1
https://hg.mozilla.org/mozilla-central/rev/d2ec58a44647
https://hg.mozilla.org/mozilla-central/rev/56c4bfbc32a6
https://hg.mozilla.org/mozilla-central/rev/33c84d894f23
https://hg.mozilla.org/mozilla-central/rev/cb61ab26f9ff
https://hg.mozilla.org/mozilla-central/rev/359e9233c228
https://hg.mozilla.org/mozilla-central/rev/be58ccf6c876
https://hg.mozilla.org/mozilla-central/rev/fc19791bc037
https://hg.mozilla.org/mozilla-central/rev/17d699699df3
https://hg.mozilla.org/mozilla-central/rev/f2bef7fba06b
https://hg.mozilla.org/mozilla-central/rev/2e52c022991d

Comment 156

[Frank Wein [:mcsmurf]]

Robert: Can/should those two lines in makensis.mk also be ifdef_ed with MOZ_STUB_INSTALLER?
"+	cp $(CONFIG_DIR)/stub.exe "$(DIST)/$(PKG_INST_PATH)$(PKG_INST_BASENAME)-stub.exe"
+	chmod 0755 "$(DIST)/$(PKG_INST_PATH)$(PKG_INST_BASENAME)-stub.exe""

At least the SeaMonkey build does not like that as it does not build the stub installer. I guess Thunderbird has the same problem.

Comment 157

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

Definitely and sorry I missed that. I'd prefer to do it in bug 795580 and if you submit a patch I'll review it right now. I'll take care of it tomorrow otherwise.

Comment 158

[Jason Smith [:jsmith]]

Questions - Is the build for the stub installer still going to show up under http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/ for nightly builds? Or as of right now, they won't until some other bug is fixed to start hosting stub installer builds? Just generally looking to find the build for the code that just landed so that I can start testing immediately.

Comment 159

[Chris AtLee [:catlee]]

Here's the log from last night's nightly:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-09-29-03-06-24-mozilla-central/mozilla-central-win32-nightly-bm12-build1-build61.txt.gz

So I see the stub installer getting built, but not uploaded. Rob, does it need to get added to UPLOAD_EXTRA_FILES or similar?

Comment 160

[Chris AtLee [:catlee]]

Rob, I think you need something like this so that the stub installer is uploaded after the build:

https://hg.mozilla.org/try/rev/ae6f597c4a09
https://tbpl.mozilla.org/?tree=Try&rev=ae6f597c4a09

Comment 161

[Jason Smith [:jsmith]]

Build link: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ae6f597c4a09/try-win32/

Comment 162

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

Filed bug 795982 for signing and upload

Comment 163

[Jason Smith [:jsmith]]

I've finished off an initial test pass on this on the en-US build that looked at areas such as, but not limited to:

- General installation and uninstall use cases, along with registry as a result of install across Win 7 64-bit, Win Vista 32-bit, Win 8, Win XP
- Basic quick checks that nothing stops working with Firefox that's expected (startup, crash reporting, updating, shutdown, tab browsing, add-ons) across the Windows operating systems stated above
- Nightly and Aurora stub installers - extensively more testing the nightly stub installer, with some sanity checks (e.g. install, uninstall, launch, quit) on the aurora stub installer
- More extensive testing in areas such as installing on guest accounts, cutting network connections, custom install, antiviruses (e.g. Norton and Avast), pave-over install use cases, UAC settings
- Exploratory testing in areas such installing during a locked screen & hibernation, multiple stubs running at the same time, pulling down and installing the stub across different browsers, and other areas not noted that were developed on the fly
- Crowd-source testing with support by Alex by advertising heavily to Mozillians, blogs, twitter, facebook, etc to get outside contributors to try out and test the stub installer to find more bugs in the wild

Bugs have been linked to this bug for anything that has been found. Many of the bugs are being triaged with [stub+] for blocking the english deployment of the stub, [stub=] as a want to fix this before we ship, but not blocking, and [stub-] for a non-blocker. A summary report of the current state can be found here - http://wiki.mozilla.org/Stub_Installer/Triage. 

Any followup work or testing will be tracked on separate bugs.

Closing this as verified for the initial test pass being done for the english build.

Comment 164

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

Separating the 32 and 64 bit stub installer implementations since the 32 bit is for alll practical purposes complete while the 64 bit has issues with 64 bit Firefox in general that will hold it up.