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

VERIFIED FIXED in Firefox 20

Status

()

defect
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
Firefox 21
x86
Windows 7
Points:
---

Firefox Tracking Flags

(firefox20 verified, firefox21 verified, firefox22 verified, firefox-esr17 wontfix, b2g18 wontfix, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Assignee

Description

7 years ago
Posted 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)
Assignee

Comment 2

7 years ago
(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!
Assignee

Comment 4

7 years ago
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)
Assignee

Comment 7

7 years ago
(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?
Assignee

Comment 8

7 years ago
Posted 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
Assignee

Comment 10

7 years ago
(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.
Assignee

Comment 12

7 years ago
(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-
Assignee

Comment 14

7 years ago
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.
Assignee

Comment 20

7 years ago
(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.
Assignee

Comment 22

7 years ago
Posted patch Patch (v3)Splinter Review
Attachment #701374 - Attachment is obsolete: true
Attachment #701610 - Flags: review?(netzen)
Assignee

Comment 23

7 years ago
(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?
Assignee

Comment 25

7 years ago
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+
Posted file new dll
Sorry for taking so long... this fell off of my radar
Assignee

Comment 29

7 years ago
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: 6 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.