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

Status: VERIFIED FIXED

(Firefox :: Installer, defect)

Description

[(no longer active)]

Attachment: Patch (v1)

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!

Comment 1

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=ee0323630e67

Comment 2

[(no longer active)]

(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 :(

Comment 3

[Brian R. Bondy [:bbondy]]

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

Comment 4

[(no longer active)]

Why do you need vc6?

Comment 5

[Brian R. Bondy [:bbondy]]

Produces a smaller footprint

Comment 6

[Brian R. Bondy [:bbondy]]

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

Comment 7

[(no longer active)]

(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?

Comment 8

[(no longer active)]

Attachment: Patch (v2)

Comment 9

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

(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.

Comment 10

[(no longer active)]

(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...

Comment 11

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

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.

Comment 12

[(no longer active)]

(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 13

[Brian R. Bondy [:bbondy]]

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

Comment 14

[(no longer active)]

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?

Comment 15

[Brian R. Bondy [:bbondy]]

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.

Comment 16

[Brian R. Bondy [:bbondy]]

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

Comment 17

[Brian R. Bondy [:bbondy]]

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.

Comment 18

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

See bug 829207

Comment 19

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

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.

Comment 20

[(no longer active)]

(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.

Comment 21

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

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

Comment 22

[(no longer active)]

Attachment: Patch (v3)

Comment 23

[(no longer active)]

(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?

Comment 24

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

Yes, you should be.

Comment 25

[(no longer active)]

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 26

[Brian R. Bondy [:bbondy]]

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.

Comment 27

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

Attachment: patch - new dll diff

Comment 28

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

Attachment: new dll

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

Comment 29

[(no longer active)]

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

Comment 30

[Brian R. Bondy [:bbondy]]

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

Comment 31

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

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

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eba3447f6c37

Comment 33

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

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 34

[Alex Keybl [:akeybl]]

Comment on attachment 701610 [details] [diff] [review]
Patch (v3)

Approving for beta alongside bug 811573, in support of an upcoming stub installer experiment.

Comment 35

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

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

Comment 36

[Bogdan Maris, Desktop QA]

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.

Comment 37

[Mihai Morar, (:MihaiMorar)]

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

Comment 38

[Mihai Morar, (:MihaiMorar)]

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