I was trying to demo the stub installer today and it failed miserably in that the installer did not manage to download anything, so I tried to understand why. After a lengthy debugging session, this is what I found. So we fall into this loop in the inetbgdl NSIS plugin: <http://mxr.mozilla.org/mozilla-central/source/other-licenses/nsis/Contrib/InetBgDL/InetBgDL.cpp#386>. On the first iteration we're on an HTTP connection and everything is fine. Then the server sends a redirect to https://ftp.mozilla.org/... and we close the old connection and open a new connection here <http://mxr.mozilla.org/mozilla-central/source/other-licenses/nsis/Contrib/InetBgDL/InetBgDL.cpp#441> using port 443, which is fine. then, we loop back and call HttpOpenRequest here <http://mxr.mozilla.org/mozilla-central/source/other-licenses/nsis/Contrib/InetBgDL/InetBgDL.cpp#393> *without* passing the INTERNET_FLAG_SECURE flag, which means that we will end up sending plaintext data to a connection on port 443 which expects SSL, so the server just closes the connection on us and we continue to try again indefinitely. This matches what we observed in Wireshark as well. The fix is really simple, the patch that I attached does it. Robert, I don't actually know how to build the stub installer so I did not test this, but could you please apply my patch, create a new inetbgdl.dll, rebuild the stub installer and see if this fixes the bug? Thanks!

(In reply to :Ehsan Akhgari from comment #1) > https://tbpl.mozilla.org/?tree=Try&rev=ee0323630e67 Actually we don't build the DLL binary throughout the build, so this try server will not do what we want here :(

We don't have any automated tests anyway by the way. We'll have to build this with vc6 too. Nice find!

Why do you need vc6?

Produces a smaller footprint

Comment on attachment 701354 [details] [diff] [review] Patch (v1) Review of attachment 701354 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, I'll take the review and test this ::: other-licenses/nsis/Contrib/InetBgDL/InetBgDL.cpp @@ +394,5 @@ > path, NULL, NULL, NULL, > INTERNET_FLAG_NO_AUTO_REDIRECT | > INTERNET_FLAG_PRAGMA_NOCACHE | > + INTERNET_FLAG_RELOAD | > + (port == 443 ? INTERNET_FLAG_SECURE : 0), 0); nit: port == INTERNET_DEFAULT_HTTPS_PORT here and below

(In reply to Brian R. Bondy [:bbondy] from comment #5) > Produces a smaller footprint OK, but I don't have access to vc6 myself. Do you?

(In reply to :Ehsan Akhgari from comment #7) > (In reply to Brian R. Bondy [:bbondy] from comment #5) > > Produces a smaller footprint > > OK, but I don't have access to vc6 myself. Do you? I do though I *think* I might have had difficulty compiling this plugin. I will try later tonight.

(In reply to comment #9) > (In reply to :Ehsan Akhgari from comment #7) > > (In reply to Brian R. Bondy [:bbondy] from comment #5) > > > Produces a smaller footprint > > > > OK, but I don't have access to vc6 myself. Do you? > I do though I *think* I might have had difficulty compiling this plugin. I will > try later tonight. Thanks. Even if we can't get this to compile on vc6, we should compile it with a newer VC and see how much if affects the generated binary size. Hopefully not too much...

Already did. The stub is meant to be as small as possible and these small increases nickel and dime the size over time so we do all we can to keep it small.

(In reply to comment #11) > Already did. The stub is meant to be as small as possible and these small > increases nickel and dime the size over time so we do all we can to keep it > small. Makes sense. Thanks for the due diligence here. :-)

Comment on attachment 701374 [details] [diff] [review] Patch (v2) Review of attachment 701374 [details] [diff] [review]: ----------------------------------------------------------------- We need to also pass this to the 2nd last parameter of InternetOpenUrl via IOUFlags. That code is hit when a user of the NSIS plugin wants to use HTTP range requests for a download but the server does not support them. The info from the URL is parsed above I believe you can just !_stricmp(protocol, "https") ? INTERNET_FLAG_SECURE : 0

I see. I'm too tired tonight, but I'll try to look into it this weekend. BTW, how can I build the stub installer locally for testing?

pymake -sC browser/installer/windows/ The installer will show up in: objdir\dist\install\sea\firefox-20.0a1.en-US.win32.installer.exe To debug NSIS plugins just attach to the installer process. Your breakpoint won't be enabled until the DLL plugin is used.

Can https be disabled on the server for now until this is fixed? I'm not sure when it was enabled.

It might implicitly set the flag for InternetOpenUrl when the url text is https:// but I don't see any documentation that says that it does by the way.

See bug 829207

I also checked that the stub appears to be working on nightly, aurora, and beta. There is and has been an issue on release but we don't provide stubs for release so it isn't as big of a deal.

(In reply to Brian R. Bondy [:bbondy] from comment #15) > pymake -sC browser/installer/windows/ > > The installer will show up in: > objdir\dist\install\sea\firefox-20.0a1.en-US.win32.installer.exe That builds the full installer for me, but not the stub installer.

Likely due to http://mxr.mozilla.org/mozilla-central/source/browser/confvars.sh#15

(In reply to comment #21) > Likely due to > http://mxr.mozilla.org/mozilla-central/source/browser/confvars.sh#15 Ah, yes. If I put |export MOZ_STUB_INSTALLER=1| in my .mozconfig, I should be fine, right?

Yes, you should be.

Hmm, doing an optimized build, I still don't see the stub installer in objdir/dist/install/sea. There's only the full installer there.

Comment on attachment 701610 [details] [diff] [review] Patch (v3) Review of attachment 701610 [details] [diff] [review]: ----------------------------------------------------------------- This is hanging for me at the end of the download progress but the code looks right. I'm going to mark this as an r+ but will test the vc6 binary when Rob builds it before r+ing that.

Sorry for taking so long... this fell off of my radar

Brian promised to test this when we have a new DLL.

Works great. We no longer redirect to https but I was able to get fiddler to fake force such a redirect.

Pushed to mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/eba3447f6c37

Comment on attachment 701610 [details] [diff] [review] Patch (v3) I want this to land bug 811573 on mozilla-beta [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 829207 User impact if declined: reintroduce bug 829207 if we go with http > https redirects again. Testing completed (on m-c, etc.): it has baked on m-c and m-a for quite some time. Risk to taking this patch (and alternatives if risky): the changes made by this patch have been in use for quite some time with no negative side affects. String or UUID changes made by this patch: none

Comment on attachment 701610 [details] [diff] [review] Patch (v3) Approving for beta alongside bug 811573, in support of an upcoming stub installer experiment.

Pushed attachment 701610 [details] [diff] [review] to mozilla-beta. https://hg.mozilla.org/releases/mozilla-beta/rev/663b0a5e3c66 The changes to the dll were pushed as part of bug 811573 https://hg.mozilla.org/releases/mozilla-beta/rev/a0449ebebe8f

Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20100101 Firefox/20.0 Stub installer downloads all the necessary files and is properly installed. Though I have seen that when downloaded "Firefox Setup Stub 20.0b4.exe" actually installs FF 20 beta 2 (buildID: 20130227063501)and it updates to FF 20 beta 3(buildID: 20130305164032) on beta channel.

Verified as fixed on Windows 7 x64 using FF 21 RC1: 20130507015204 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20100101 Firefox/21.0

Verified as fixed on Windows 7 x64 using FF 22b1 : 20130514181517 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0