Closed Bug 1002724 Opened 9 years ago Closed 2 years ago

Fall back to HTTPS in URL fixup (including location bar) (like for hosts that refuse a connection on port 80 via HTTP, but accepts connections via HTTPS on port 443)

Categories

(Core :: DOM: Navigation, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla78
Tracking Status
firefox78 --- fixed

People

(Reporter: rbarnes, Assigned: geekboy)

References

(Blocks 2 open bugs, )

Details

(Whiteboard: [fxprivacy])

Attachments

(8 files, 6 obsolete files)

6.59 KB, patch
Details | Diff | Splinter Review
2.89 KB, patch
Details | Diff | Splinter Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
Currently, if you enter a partial URI without a scheme into the URL bar (e.g., just a hostname or hostname/path), the browser attempts HTTP and fails if that doesn't work.

Instead, the browser should attempt HTTPS when HTTP fails, and only fail if HTTPS also fails.

Note: This is much less radical than prior proposals for improving location bar security (e.g., Bug 902338, Bug 902582, Bug 769994)
if you type the secure page even just once it should complete to the secure page.
I feel like what you suggest is basically to workaround badly configured sites, those should redirect to the https version or enforce https.
Btw, mail.mozilla.org/zimbra doesn't work for me even if I use https to visit it...
It seems bizarre to me to suggest that not supporting HTTP is somehow "bad configuration".  Sites should be able to operate HTTPS-only with no penalty.

Try "mail.mozilla.com" (with "com" instead of "org").
Requesting info from some front-end folks.
Flags: needinfo?(johnath)
Flags: needinfo?(gavin.sharp)
I'm unlikely to be your best bet for a ruling on the usability here - clearing my needinfo, but adding Madhava to have a look
Flags: needinfo?(johnath)
(In reply to Richard Barnes [:rbarnes] from comment #0)
> Instead, the browser should attempt HTTPS when HTTP fails, and only fail if
> HTTPS also fails.

This seems reasonable. We'd likely want to implement this at the docshell level, perhaps close to http://hg.mozilla.org/mozilla-central/annotate/78245b8d422d/docshell/base/nsDocShell.cpp#l7024.
Component: Location Bar → Document Navigation
Flags: needinfo?(gavin.sharp)
Product: Firefox → Core
Summary: Add HTTPS as a default scheme in location bar completion → Fall back to HTTPS
Summary: Fall back to HTTPS → Fall back to HTTPS in URL autocomplete
McManus, any thoughts on this?  Seem like a worthwhile idea?
Flags: needinfo?(mcmanus)
I think trying https on schemeless urls is perfect - grease the skids of https everywhere. http should probably remain the first choice just because of web compat issues.

maybe even bonus points for kicking off a nsISpeculativeConnect to 443 while trying 80.
Flags: needinfo?(mcmanus)
Attached patch hideous patch, first whack (obsolete) — Splinter Review
Ok, so this is a total hack and not elegant or anything and has some leftovers from initial hacking I did.  This is not ready for review.  Putting it here because I'm about to take off for the weekend and want to offer up the progress I made to anyone (rbarnes *cough, cough*) who might want to make progress.

Caveats aside, this seems to at least initially work.  I think we can *probably* even get by without using nsIURIFixup for the conversion to http, making most of the changes only in nsDocShell.

This still needs:
* Tests.  I'm only testing this manually right now with one site that does not redirect http->https, which is not an acceptable test.
* The nsISpeculativeConnect stuff mcmanus offered bonus points for using.
Assignee: nobody → sstamm
Status: NEW → ASSIGNED
Summary: Fall back to HTTPS in URL autocomplete → Fall back to HTTPS in URL fixup (like for hosts that refuse a connection on port 80 via HTTP, but accepts connections via HTTPS on port 443)
Attached patch proposed patch (obsolete) — Splinter Review
Hey mcmanus, was this what you had in mind with the speculative connect?  When URI fixup tries an HTTP uri, it triggers a speculative connect for an HTTPS version (which may or may not be used if the http version fails).

This patch still needs tests.  I'm trying to figure out the best/easiest way to test this.  If anyone has pointers to similar tests, I'd love to see them.
Attachment #8443828 - Attachment is obsolete: true
Attachment #8459018 - Flags: feedback?(mcmanus)
Comment on attachment 8459018 [details] [diff] [review]
proposed patch

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

yeah, that's pretty sweet.

we ought to be able to run a packet cap showing 80/443 handshakes happening more or less in parallel - so a refusal on 80 shouldn't be slowing down using 443 significantly.

oh - and if the url used a non-default port number then we shouldn't be doing the https thing at all, right?
Attachment #8459018 - Flags: feedback?(mcmanus) → feedback+
(In reply to Patrick McManus [:mcmanus] from comment #10)
> yeah, that's pretty sweet.

Thanks!  Sorry this is taking so long, it's priority 3 right now in my queue.  Still trying to figure out the automated tests...

> we ought to be able to run a packet cap showing 80/443 handshakes happening
> more or less in parallel - so a refusal on 80 shouldn't be slowing down
> using 443 significantly.

Yeah, that's what it looks like when I watched traffic via wireshark.  I noticed a bonus with this approach too: for those sites that serve on port 80 simply to redirect to port 443, we get a slight head start on TLS handshake with this speculative connect.  So for sites that are primarily https, this will be a partial speedup (though not as good as hsts).

> oh - and if the url used a non-default port number then we shouldn't be
> doing the https thing at all, right?

I did not yet add this to the patch, but it's pretty easy to do.  Before flagging for review I will add a gate for the speculative connect and fix-up-to-https for HTTP URLs with default port.
Summary: Fall back to HTTPS in URL fixup (like for hosts that refuse a connection on port 80 via HTTP, but accepts connections via HTTPS on port 443) → Fall back to HTTPS in URL fixup (including location bar) (like for hosts that refuse a connection on port 80 via HTTP, but accepts connections via HTTPS on port 443)
Duplicate of this bug: 1147558
What's needed to move forward here? There's a general push for more HTTPS this year, and fixing this would help a bit.
Flags: needinfo?(sstamm)
This needs tests and time to finish it.  If someone else wants to pick this up and run with it, I'd be happy to transfer ownership.
Flags: needinfo?(sstamm)
ISTM that we should apply the same testing standard here as with other URI fixup modalities.  If someone can point me to where those things get tested, I'll gladly mimic them.  Otherwise, let's do some manual testing and land this.

Dave: Do you know where existing URI fixup stuff is getting tested, if it is?
Flags: needinfo?(davemgarrett)
(In reply to Richard Barnes [:rbarnes] from comment #15)
> Dave: Do you know where existing URI fixup stuff is getting tested, if it is?

https://mxr.mozilla.org/mozilla-central/source/docshell/test/unit/
test_nsDefaultURIFixup.js, test_nsDefaultURIFixup_info.js, & test_nsDefaultURIFixup_search.js
Flags: needinfo?(davemgarrett)
Duplicate of this bug: 152785
Two days ago more than 50% of page loads were HTTPS https://twitter.com/0xjosh/status/786971412959420424

This bug (fallback to HTTPS if port 80 closed) would be the first step to bug 1158191 (try https first),

This fits perfect to https://github.com/chromium/hstspreload.appspot.com/issues/47 (Allow servers that block port 80 to preload).

It would be a perfect time to continue here. (Please! ;-)
Flags: needinfo?(mozbugs)
:keeler - can you revive this useful bug somehow? There is a patch.
Flags: needinfo?(dkeeler)
Not really my bailiwick. Maybe Tanvi knows who could work on this and/or put it on the content security roadmap?
Flags: needinfo?(dkeeler) → needinfo?(tanvi)
Panos, I heard there were plans in this direction. Is there a separate bug and/or can we reuse this patch?
Flags: needinfo?(past)
Thanks for the ping, I'll try to raise the priority of this bug.
Flags: needinfo?(past)
Whiteboard: [fxprivacy][triage]
Priority: -- → P2
Whiteboard: [fxprivacy][triage] → [fxprivacy]
As an addendum to this bug (:rbarnes suggested I comment on this bug instead of opening up a new one):

It would be great if we had a preference that made schemeless contexts (such as typing "bank.com" into a URL bar) attempt HTTPS first, and then fall back to HTTP if HTTPS isn't available.  It is, after all, hopefully the future.  As such, it would be great if we could have something for users to begin testing with.
See Also: → 1158191
See Also: → 1310447
Flags: needinfo?(mozbugs)
(In reply to April King [:April] from comment #23)
> As an addendum to this bug (:rbarnes suggested I comment on this bug instead
> of opening up a new one):
> 
> It would be great if we had a preference that made schemeless contexts (such
> as typing "bank.com" into a URL bar) attempt HTTPS first, and then fall back
> to HTTP if HTTPS isn't available.  It is, after all, hopefully the future. 
> As such, it would be great if we could have something for users to begin
> testing with.

what's the plan to deal with the compat breakage? I don't think we can afford to do it.
Can you be clear exactly about what compat things that it's going to break?  Eventually the internet will be 80, 90, 99% HTTPS and it won't make sense to expose users to MitM attacks by continually defaulting to HTTP first.
I'm very pro https - but I'm also committed to compat for our users.

Have a look at http://www.syracuse.com and https://www.syracuse.com .. silently turning the behavior that yielded the former into the latter is a bad user experience.

(In reply to April King [:April] from comment #25)
> Can you be clear exactly about what compat things that it's going to break? 
> Eventually the internet will be 80, 90, 99% HTTPS and it won't make sense to
> expose users to MitM attacks by continually defaulting to HTTP first.

I think there are benefits to what you propose, but as long as you're proposing fallback you've still got a mitm problem that simply using https:// (only) would not have.
How about we gate this on a pref, which advanced users can change (a la HTTPS Everywhere), and which we can make default once the compat risks are low enough?

Similar to the approach in Bug 1310447
Since #c23, April and Patrick are talking about a variant of bug 1158191 (to try https first [for banks]) and not about this fallback-to-https-if-port-80-is-closed bug. I don't see any compat risks to try https as fallback instead of showing an http:// connection error.
(In reply to bugzilla from comment #29)

> I don't see any compat risks to
> try https as fallback instead of showing an http:// connection error.

I agree with that.. intuition says it won't accomplish a lot in practice though
There are more new websites who don't even open port 80. Yes, older sites might need to 301 as long they are not HSTS-preloaded.
Port 443 only, older, not preloaded: http://banking.berliner-sparkasse.de (connection timeout)
Port 443 only, new, preloaded: https://perfektesgewicht.de
Port 443 only, new, not preloaded (for pending submissions): 
"There have been a lot more requests for this as of the last few weeks, so I spent the time to implement this, and preloading without port 80 now supported."
https://github.com/chromium/hstspreload.org/issues/47#issuecomment-264584336

If this functionality got implemented, new https-only sites wouldn't have to wait until they are preloaded or wouldn't have to open port 80 for a 301.
I insistently hope that this https-only barrier gets broken down.

network.http.connection-timeout;90 and network.http.response.timeout;300 (don't know which one is applicable here) are used for http:// and https://, right?
Maybe https should get its own pref (with the current value) to lower the time in the mentioned http:// ones to enable a faster fallback to https.
> How about we gate this on a pref, which advanced users can change (a la HTTPS Everywhere), and which we can make default once the compat risks are low enough?

Yes, that is what I suggested in my comment as I figured compat issues would be a problem for the indeterminate future.

> I think there are benefits to what you propose, but as long as you're proposing fallback you've still got a mitm problem that simply using https:// (only) would not have.

This is *only* when there is no scheme listed, i.e., where it currently defaults to HTTP.  If somebody types https://example.com/, or the site uses HSTS, we should definitely *never* fall back to HTTP.  But falling back to HTTP if I just type "example.com<enter>" has the exact same MITM risk with or without the preference:

With pref: HTTPS gets blocked by MITM, fallback to HTTP gets MITM'd
Without pref: HTTP gets MITM'd

Meanwhile, the preference to default to HTTPS considerably reduces people's risk to eavesdroppers, when the site in question does in fact support HTTPS.
I'm fine with april's approach under a pref, but it would seem to be something different than what this bug proposes (which can be done prefless)

april, sounds like we're in agreement downgrade exposure.. anytime you will take http at all you've got that problem.
Yeah, I asked :rbarnes if he wanted it as a separate bug, and he suggested posting it in here.  Would you prefer me to split it off?

Thanks!  :)
(In reply to April King [:April] from comment #34)
> Yeah, I asked :rbarnes if he wanted it as a separate bug, and he suggested
> posting it in here.  Would you prefer me to split it off?
> 
> Thanks!  :)

I would like to ask you to merge your try-https-first-for-banks idea into bug 1158191.

There, please do something like that (fictional):
network.https.tryfirst = false (if true, falls back to http if port 443 is closed)
network.https.enforce = false (= disable http:// = bug 1335590)
network.https.enforce.keywords = true
network.https.enforce.keywords.list = "bank,webmail" (maybe regex?)
Duplicate of this bug: 1339928
Another use case: Embedded devices (such as switches (HP Procurve)) can enable web management interface using http and/or https. You can't configure a redirect from http to https so you might want to disable http. But this is bad for user experience...
(In reply to David Keeler [:keeler] (use needinfo?) from comment #20)
> Not really my bailiwick. Maybe Tanvi knows who could work on this and/or put
> it on the content security roadmap?

This bug is hanging off of the https-everything bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1335586), which is on the seceng roadmap.  We can fall back to HTTPS when HTTP fails (as comment 0 describes).  I don't see any negative user experience issues with that.

Sid, if you aren't going to work on this bug, can you release it, in case someone else wants to take it?
Flags: needinfo?(tanvi) → needinfo?(mozbugs)
(In reply to Tanvi Vyas[:tanvi] from comment #38)
> Sid, if you aren't going to work on this bug, can you release it, in case
> someone else wants to take it?

Will do, sorry for the bug-neglect.  :-/
Assignee: mozbugs → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(mozbugs)
Attached patch Potential Patch (obsolete) — Splinter Review

I can try to pick this up again (I know, it's been a long time). Attaching an updated patch that applies on mozilla-central. This new patch supports the default-port gate requested by :mcmanus in comment 10. The speculative connect is initiated whether or not we're refused http on port 80 because there's potential we might end up getting https soon anyway. The speculative connect is easy to adjust.

Assignee: nobody → mozbugs
Attachment #8459018 - Attachment is obsolete: true
Attached patch Tests WIP (obsolete) — Splinter Review

The hard part of this appears to be testing. I can't find any live sites that exhibit this behavior (refuse connection on port 80, allow HTTPS on 443). Most either redirect http->https or allow port 80. If anyone knows of a live site, it would help me speed up testing!

I started looking into the other tests suggested by Dave G in comment 16, but the best way to automate testing this is also not obvious. I created a browser mochitest that attempts to do this, but it's not quite right. Looks like we don't end up down the right code path the way I added a server to build/pgo/server-locations.txt: it doesn't generate NS_ERROR_CONNECTION_REFUSED, but seems to go down the NS_ERROR_UNKOWN_HOST || NS_ERROR_NET_RESET code path. I haven't investigated fully, but I don't know how to quickly set up a connection reject on port 80 for a given host that also serves mochitests over HTTPS on port 443.

This patch fools around with the mochitest.youtube.com domain for testing, but I'm not confident that does what we want.

Hi folks, just a side crazy idea (beat me if I'm off here!):
Do we need to first fail the the HTTP and only then try HTTPS?
Can't we (and of course, make this changeable by the user in the config section), as we get the server's IP from the DNS query reply - do a quick "tcp ping", maybe just at only the getting-a-SYN-reply level (to save time), to both ports - 80 and 443, and if 443 reply as open - go to 443 as the first option?

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #41)
Thank you! :O

The hard part of this appears to be testing. I can't find any live sites that exhibit this behavior (refuse connection on port 80, allow HTTPS on 443). Most either redirect http->https or allow port 80. If anyone knows of a live site, it would help me speed up testing!

mozregression --launch 2020-04-21 --pref network.stricttransportsecurity.preloadlist:false -a http://mail.terrax.net -a http://rustls.org

Edit: mail.terrax.net does not support legacy IP, it requires IPv6, but rustls.org is dualstack.
Edit2: http://captured-symphonies.com is also dualstack.

The hard part of this appears to be testing. I can't find any live sites that exhibit this behavior (refuse connection on port 80, allow HTTPS on 443). Most either redirect http->https or allow port 80. If anyone knows of a live site, it would help me speed up testing!

Is this something worth investing in to get added to badssl.com, or is it a temporary need?

(In reply to April King [:April] from comment #44)

Is this something worth investing in to get added to badssl.com, or is it a temporary need?

If it's a non-trivial amount of work to get it added to badssl.com, I wouldn't bother. While that would be nice, automated tests should be sufficient and this is more about convenience than fixing a security problem.

Attached patch Potential PatchSplinter Review

Had to make a minor change to the patch, but this one seems to work.

Tested via code-following in debugger and manually with opt builds both with and without the patch (with clean profile and making sure HSTS preload is disabled):

 $ ./mach run --temp-profile --setpref network.stricttransportsecurity.preloadlist=false

then I type rustls.org in the URL bar and hit enter.

Without the patch, I get a connection error. With the patch, it redirects via HTTPS to a github URL. I still need to automate testing somehow, but I think this should do the job.

Attachment #9142383 - Attachment is obsolete: true

If it's a non-trivial amount of work to get it added to badssl.com, I wouldn't bother. While that would be nice, automated tests should be sufficient and this is more about convenience than fixing a security problem.

It is, unfortunately, as it would need an additional IP address. It sounds like you've got some good temporary options, but lemme know if you decide this is something that is worth keeping around.

I spent some time trying to automate testing this patch with browser mochitests, but I'm not convinced it's a good idea. The mochitests route all HTTP connections to the same server, which means the host- and port-based multiplexing happens at the HTTP layer and not at the socket layer so we really can't reject a connection to an individual port-80 host.

First, I tried hacking up the sslproxy, but that totally doesn't make sense since non-https connections don't go through it. I won't upload that attempted patch, but it's based on attachment 571410 [details] [diff] [review] in bug 599295.

Next, I tried modifying the mochitest server files to close connections when accessing a new http://fallback.example.com:80 entry I added to build/pgo/server-locations.txt, but it causes a different error, not NS_ERROR_CONNECTION_REFUSED (which makes sense... the connection is accepted, but later closed). I'm attaching the work for this attempted test in case anyone wants to look at it.

Since the mochitests listen on port 80, we probably can't set another one up and since 127.0.0.1 is already used I'm out of ideas of how to set up another server on port 80 that is accessible via mochitests. Maybe there's a way to use an xpcshell test and a one-off HTTP/HTTPS server to test this, but I'm not sure how that might work.

Does anyone have suggestions of how to create automated tests for this bug?

Attachment #9144408 - Attachment is patch: true

Could you launch a server on https://[::1]:443/ and try to connect to http://[::1]/? (Or https://127.0.0.13:443/)

In a private conversation, :darkspirit suggested using another loopback interface (like maybe 127.0.0.2 instead of 127.0.0.1) for this test so we can have different server behaviors based on port at the socket layer instead of relying on HTTP host multiplexing. I'll try to investigate later this week.

I investigated more, but it appears to be a substantial undertaking to test using xpcshell (requires making/standing up a server) or to add multi-IP support to mochitests, and this bug is too trivial in my opinion to warrant that much change to the test suites. I also looked more into the URI fixup tests, but this is an additional step beyond URI fixup, and URI fixup doesn't handle hosts that reject connections for which we want to try https upgrades (they are valid/live URIs).

mayhemer: do you happen to have any idea if there's a way to easily test nsDocShell with an HTTP server that can reject attempted connections on port 80? A manual test is described in comment 46, and browser mochitest didn't do this out of the box.

In summary, my goal is to:

  1. attempt connection to some host via urlbar entry (unspecified port/scheme, defaulting to 80/http),
  2. let the server reject the attempted connection on port 80 (can be a quick socket accept()/close()), then
  3. check if nsDocShell re-attempts or "fixes up" to a connection to the same host name using 443/https.

The test can either make a successful connection via https, or the server can reject it too -- I just need to test that nsDocShell tries 443/https if the host is alive but server rejects connections to 80/http.

Flags: needinfo?(honzab.moz)
Attached patch Tests (obsolete) — Splinter Review

Keeler, my hero! Yes, that works beautifully. It's been so long since I wrote a test for Firefox, would you be so kind as to give me a bit of feedback on the short tests?

Attachment #9142386 - Attachment is obsolete: true
Attachment #9144408 - Attachment is obsolete: true
Flags: needinfo?(mozbugs)
Flags: needinfo?(honzab.moz)
Attachment #9145610 - Flags: feedback?(dkeeler)
Comment on attachment 9145610 [details] [diff] [review]
Tests

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

I'm certainly not an expert in these kinds of tests, but this looks good to me in general. I left a couple of comments.

::: build/pgo/server-locations.txt
@@ +319,5 @@
>  # Host for testing APIs whitelisted for mozilla.org
>  https://www.mozilla.org:443
> +
> +# Hosts for fallback tests Bug 1002724
> +https://fallback.example.com:443  privileged

Looks like we don't need this any longer.

::: docshell/test/browser/browser.ini
@@ +152,5 @@
>  skip-if = !e10s # e10s specific test.
>  [browser_tab_replace_while_loading.js]
>  skip-if = (os == 'linux' && bits == 64 && os_version == '18.04') # Bug 1604237
>  [browser_browsing_context_discarded.js]
> +[browser_bug1002724.js]

nit: might be good to give this a more descriptive name like `browser_fall_back_to_https.js` or something

::: docshell/test/browser/browser_bug1002724.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +

It would be good to include a short summary of what this test tests.

@@ +10,5 @@
> +const bug1002724_tests = [
> +  {
> +    original: "example.com",
> +    expected: "http://example.com",
> +    explanation: "Should load HTTPS version of example.com",

`HTTP`, not `HTTPS`, right?
Attachment #9145610 - Flags: feedback?(dkeeler) → feedback+
Attached patch TestsSplinter Review

Thanks, :keeler!

Attachment #9145610 - Attachment is obsolete: true

Based on the try run, looks like a number of tests are failing because they're upgrading localhost or 127.0.0.1 URLs to https.
For reference, some of the browser tests that are failing due to some URL starting with https instead of http:

  • browser/base/content/test/general/browser_decoderDoctor.js
  • services/fxaccounts/tests/browser/browser_device_connected.js
  • services/fxaccounts/tests/browser/browser_verify_login.js
  • toolkit/components/passwordmgr/test/browser/browser_doorhanger_autofill_then_save_password.js
  • toolkit/mozapps/extensions/test/browser/browser_html_detail_view.js

I originally thought this had something to do with the speculative loads pre-fetching https versions and then the docshell preferring those over http, so I removed the speculative loading bits but the tests still failed (because https instead of http). So probably it's not due to the speculative load bits.

If I run mochitests with --no-autorun, then manually try to load localhost or 127.0.0.1 without my patches applied, the connections are refused (even when example.com and 127.0.0.1:8888 do load). So that leads me to think these failing tests are expecting a url that doesn't necessarily load but gets built and put into the URL bar (which my patch will try to rewrite if the connection is refused).

But it's not clear to me why localhost and 127.0.0.1 don't properly load mochitest stuff despite being present in server-locations.txt:
https://searchfox.org/mozilla-central/source/build/pgo/server-locations.txt#65
https://searchfox.org/mozilla-central/source/build/pgo/server-locations.txt#317

Dana, do you happen to know off the top of your head why these URLs won't load but other ones do? This feels related to your 127.0.0.2 suggestion...

Flags: needinfo?(dkeeler)

Sorry for my naive question:
(1) Does the current patch upgrade all failed connections if mAllowKeywordFixup is true? It doesn't seem to check for top frame.

(2) If above does not apply and these tests just want their error page, they should either
a) disable a browser.fixup.fallback-to-https pref before running or
b) be changed to a hard-coded domain that won't be upgraded, e.g. http://offline.invalid/.

A dedicated pref might anyway be useful for debugging.

(1) Does the current patch upgrade all failed connections if mAllowKeywordFixup is true?

Yes, I was attempting to upgrade as much as possible when port 80 connections are rejected. Maybe that's a mistake, but the test failures are all top-level URLs (you can see them in the url bar as the test runs). I'm still confused, however, about why localhost:80 doesn't serve data for the mochitests.

(2) If above does not apply and these tests just want their error page, they should either
a) disable a browser.fixup.fallback-to-https pref before running or
b) be changed to a hard-coded domain that won't be upgraded, e.g. http://offline.invalid/.

A dedicated pref might anyway be useful for debugging.

This seems reasonable, but I'd hate to add more complexity to the browser if we don't need to. Maybe I'll make another patch for the pref just in case. Thanks for the idea!

It looks like the test infrastructure doesn't listen on port 80 while running, so it makes sense that we can't connect to it. That said, I don't know why the test suite attempts to load those URLs (and why the tests pass even if those fail to load?). Maybe those tests need to be fixed? e.g. maybe we need to specify 127.0.0.1:8888 here: https://searchfox.org/mozilla-central/rev/dc4560dcaafd79375b9411fdbbaaebb0a59a93ac/browser/base/content/test/general/browser_decoderDoctor.js#188

Flags: needinfo?(dkeeler)

Good idea! I tweaked the failed tests (changing the hostname or scheme worked like a charm locally) and am running on try again. Still a bunch of fails, but I'm not certain my patches caused these since many appear to be M-fis tests or intermittent oranges.

Still investigating, here's the try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=30df1250f3f81c089f46e22175f23c10dcdd0e98

It appears most of the failures in that try run were intermittent and likely due to my random selection of a mozilla-central revision upon which to apply my patches. There are only a couple of test failures I'm worried about:

  • My new test is failing on Mac OS X
  • toolkit/components/reader/test/browser_readerMode.js is timing out (that might be due to another revision's changes, not sure how it is related)

I'm not sure how to investigate the Mac OS X failure (don't have Mac OS), and could use help from someone who does and is willing to apply the patches in this bug or in the try run linked above in comment 61.

Looks pretty good rebased on yesterday's mozilla-central:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6630da75f16538784f8be93ebe7cb7d471e2ad61

The Mac OS X timeouts seem to occur in various tests of the same chunk, so I'm not sure if that's a big problem.

I requested review from various people on D75088, D75087, and D75086 because I want folks who know stuff about those tests to verify these changes are indeed appropriate for those tests (see comment 60).

:smaug - let me know if you would like to offload the review to someone else who can approve docshell changes. I wasn't sure who to target and :ckerschb recommended you.

:jaws - please take a look at the minor changes in D75086 and let me know if it's ok to change hostnames in those test URLs.

Flags: needinfo?(jaws)
Attachment #9147962 - Attachment description: Bug 1002724 - add pref for fallback to https. r?smaug → Bug 1002724 - add pref for fallback to https. r=smaug
Attachment #9147963 - Attachment description: Bug 1002724 - Test that HTTPS is tried if typed host name doesn't respond via HTTP. r?smaug → Bug 1002724 - Test that HTTPS is tried if typed host name doesn't respond via HTTP. r=smaug
Attachment #9147964 - Attachment description: Bug 1002724 - use resolvable URL in fxaccounts browser chrome tests. r?rfkelly → Bug 1002724 - use resolvable URL in fxaccounts browser chrome tests. r=rfkelly
Attachment #9147961 - Attachment description: Bug 1002724 - try https if http connections are rejected. r?smaug → Bug 1002724 - try https if http connections are rejected. r=smaug

:mstriemer - do you think this itty-bitty URL change in the browser_html_detail_view.js test (see D75088) is acceptable? I can't imagine it will cause any problems...

Flags: needinfo?(mstriemer)

Thanks for the review, :jaws!

Status: NEW → ASSIGNED
Flags: needinfo?(jaws)
Flags: needinfo?(mstriemer)
Attachment #9147966 - Attachment description: Bug 1002724 - use resolvable URL in extension html_detail_view test. r?mstriemer → Bug 1002724 - use resolvable URL in extension html_detail_view test. r=mstriemer
Attachment #9147965 - Attachment description: Bug 1002724 - use resolvable URL in browser decoderDoctor test. r?jaws → Bug 1002724 - use resolvable URL in browser decoderDoctor test. r=jaws
Pushed by mozilla@sidstamm.com:
https://hg.mozilla.org/integration/autoland/rev/4ebad0d2ca0a
try https if http connections are rejected. r=smaug
https://hg.mozilla.org/integration/autoland/rev/b60ba645f1f4
add pref for fallback to https. r=smaug
https://hg.mozilla.org/integration/autoland/rev/01cbf611158a
Test that HTTPS is tried if typed host name doesn't respond via HTTP. r=smaug
https://hg.mozilla.org/integration/autoland/rev/da26540ecee5
use resolvable URL in fxaccounts browser chrome tests. r=rfkelly
https://hg.mozilla.org/integration/autoland/rev/d481cf074d3b
use resolvable URL in browser decoderDoctor test. r=jaws
Pushed by mozilla@sidstamm.com:
https://hg.mozilla.org/integration/autoland/rev/56ba616e2644
use resolvable URL in extension html_detail_view test. r=mstriemer

Backed out for bustage on nsDocShell.cpp

backout: https://hg.mozilla.org/integration/autoland/rev/7ecaba5a83ce08a17eaa5b9b7976ed8f8e44894c

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d481cf074d3baa856f8bdf8b50676fbbfa9b4158&searchStr=build&selectedTaskRun=QaMEfIJ1Sd2nYKsKgtKABw-0

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=302745457&repo=autoland&lineNumber=33442

[task 2020-05-18T12:59:40.117Z] 12:59:40 INFO - In file included from Unified_cpp_docshell_base0.cpp:92:
[task 2020-05-18T12:59:40.117Z] 12:59:40 ERROR - /builds/worker/checkouts/gecko/docshell/base/nsDocShell.cpp:6121:13: error: ignoring return value of function declared with 'nodiscard' attribute [-Werror,-Wunused-result]
[task 2020-05-18T12:59:40.117Z] 12:59:40 INFO - NS_MutateURI(url)
[task 2020-05-18T12:59:40.117Z] 12:59:40 INFO - ^~~~~~~~~~~~~~~~~
[task 2020-05-18T12:59:40.117Z] 12:59:40 INFO - 1 error generated.
[task 2020-05-18T12:59:40.117Z] 12:59:40 INFO - /builds/worker/checkouts/gecko/config/rules.mk:746: recipe for target 'Unified_cpp_docshell_base0.o' failed
[task 2020-05-18T12:59:40.117Z] 12:59:40 ERROR - make[4]: *** [Unified_cpp_docshell_base0.o] Error 1
[task 2020-05-18T12:59:40.117Z] 12:59:40 INFO - make[4]: Leaving directory '/builds/worker/workspace/obj-build/docshell/base'
[task 2020-05-18T12:59:40.117Z] 12:59:40 INFO - /builds/worker/checkouts/gecko/config/recurse.mk:74: recipe for target 'docshell/base/target-objects' failed
[task 2020-05-18T12:59:40.117Z] 12:59:40 ERROR - make[3]: *** [docshell/base/target-objects] Error 2
[task 2020-05-18T12:59:40.117Z] 12:59:40 INFO - make[3]: *** Waiting for unfinished jobs....

Flags: needinfo?(mozbugs)
Flags: needinfo?(mozbugs)
Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c60c48d170e1
try https if http connections are rejected. r=smaug
https://hg.mozilla.org/integration/autoland/rev/782808a05ff8
add pref for fallback to https. r=smaug
https://hg.mozilla.org/integration/autoland/rev/1c57f8717b15
Test that HTTPS is tried if typed host name doesn't respond via HTTP. r=smaug
https://hg.mozilla.org/integration/autoland/rev/e868f1e0af0e
use resolvable URL in fxaccounts browser chrome tests. r=rfkelly
https://hg.mozilla.org/integration/autoland/rev/37473a8ba1fd
use resolvable URL in browser decoderDoctor test. r=jaws
https://hg.mozilla.org/integration/autoland/rev/e487b4cd9223
use resolvable URL in extension html_detail_view test. r=mstriemer

Argh, looks like the Mac OS X timeouts are consistent and need to be fixed prior to trying to land this again. I'm not sure how to debug this without a Mac (I don't have easy access to one), except maybe using try as a test machine... which sounds really annoying.

:keeler - do you know if there are issues with mochitests using 127.0.0.2 on mac (exclusively)?

Flags: needinfo?(mozbugs) → needinfo?(dkeeler)

What I'm seeing on my mac is the TCP SYN packets to 127.0.0.2 are timing out instead of getting reset/closed. After some googling, it looks like loopback addresses other than 127.0.0.1 aren't enabled by default on macOS (?) so maybe this test isn't even possible as-is (or maybe we want to modify the patch to try upgrading to https on timeouts as well?). So one approach right now would be to mark that test as skip-if = os == 'mac' in the test manifest. Another thing we could potentially do is to modify our test machines to add an interface alias for that address. I don't know how feasible that is, though.

Flags: needinfo?(dkeeler)

Skipping on macOS sounds right. Fallback on timeout could be a further enhancement.

Thanks, Dana. I was afraid of that.... seems a bit overkill to me to change the test infrastructure for this little bug.

:smaug - are you still ok with these docshell changes if we don't test on Mac? If you are, I will edit the ini file to skip the browser chrome tests on Mac and re-flag you for review on the test (or not if you say it's ok)...but I promise to wait for a try run that doesn't fail on Mac before trying to land it again!

Flags: needinfo?(bugs)

Is there any other way to test this on Mac? But if not, I think skipping on mac might not be totally horrible, since the code as such is cross-platform.

Flags: needinfo?(bugs)

Short of some pretty serious changes to the test framework, we can't shoehorn this into browser chrome tests on Mac, and I couldn't find another way to test this patch in general without standing up a duplicate HTTP/HTTPS server -- seems ridiculous for one test.

We could try to tweak the test infrastructure environment on OS X to open up 127.0.0.2 (see comment 81), but I have no idea how to do that. I'd like to land this change with tests disabled on Mac, then I'll file a lower-priority follow-up bug to explore enabling the tests on Mac.

:smaug and I discussed this out of band and we're going to land this without tests on mac until we figure out how to test it on mac. I'll file a follow-up bug to investigate running that test on OS X when this code lands.

Blocks: 1639941
Pushed by mozilla@sidstamm.com:
https://hg.mozilla.org/integration/autoland/rev/11515373dbd2
try https if http connections are rejected. r=smaug
https://hg.mozilla.org/integration/autoland/rev/9b6944448186
add pref for fallback to https. r=smaug
https://hg.mozilla.org/integration/autoland/rev/71cdc10cc959
Test that HTTPS is tried if typed host name doesn't respond via HTTP. r=smaug
https://hg.mozilla.org/integration/autoland/rev/d643f8c4a1a1
use resolvable URL in fxaccounts browser chrome tests. r=rfkelly
https://hg.mozilla.org/integration/autoland/rev/cb1683a32099
use resolvable URL in browser decoderDoctor test. r=jaws
https://hg.mozilla.org/integration/autoland/rev/22855ae7fa00
use resolvable URL in extension html_detail_view test. r=mstriemer
You need to log in before you can comment on or make changes to this bug.