Closed Bug 829829 Opened 9 years ago Closed 9 years ago

The stub installer fails to download the file when the server redirects to an HTTPS server

Categories

(Firefox :: Installer, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 21
Tracking Status
firefox20 --- verified
firefox21 --- verified
firefox22 --- verified
firefox-esr17 --- wontfix
b2g18 --- wontfix
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

Details

Attachments

(3 files, 2 obsolete files)

Attached patch Patch (v1) (obsolete) — Splinter Review
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!
Attachment #701354 - Flags: review?(robert.bugzilla)
(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
Attachment #701354 - Flags: review?(robert.bugzilla) → review?(netzen)
(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?
Attached patch Patch (v2) (obsolete) — Splinter Review
Assignee: nobody → ehsan
Attachment #701354 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #701354 - Flags: review?(netzen)
Attachment #701374 - Flags: review?(netzen)
(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.
OS: Mac OS X → Windows 7
(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
Attachment #701374 - Flags: review?(netzen) → review-
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.
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.
Attached patch Patch (v3)Splinter Review
Attachment #701374 - Attachment is obsolete: true
Attachment #701610 - Flags: review?(netzen)
(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.
Attachment #701610 - Flags: review?(netzen) → review+
Attached file new dll
Sorry for taking so long... this fell off of my radar
Brian promised to test this when we have a new DLL.
Flags: needinfo?(netzen)
Works great. We no longer redirect to https but I was able to get fiddler to fake force such a redirect.
Flags: needinfo?(netzen)
Attachment #709332 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/eba3447f6c37
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
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
Attachment #701610 - Flags: approval-mozilla-beta?
Comment on attachment 701610 [details] [diff] [review]
Patch (v3)

Approving for beta alongside bug 811573, in support of an upcoming stub installer experiment.
Attachment #701610 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.