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)
Tracking
()
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)
Comment 1•10 years ago
|
||
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...
Reporter | ||
Comment 2•10 years ago
|
||
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").
Reporter | ||
Comment 3•10 years ago
|
||
Requesting info from some front-end folks.
Comment 4•10 years ago
|
||
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
Comment 5•10 years ago
|
||
(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.
Reporter | ||
Updated•10 years ago
|
Reporter | ||
Comment 6•10 years ago
|
||
McManus, any thoughts on this? Seem like a worthwhile idea?
Comment 7•10 years ago
|
||
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.
Assignee | ||
Comment 8•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee | ||
Comment 9•10 years ago
|
||
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.
Comment 10•10 years ago
|
||
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?
Assignee | ||
Comment 11•10 years ago
|
||
(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.
Updated•9 years ago
|
Comment 13•9 years ago
|
||
What's needed to move forward here? There's a general push for more HTTPS this year, and fixing this would help a bit.
Assignee | ||
Comment 14•9 years ago
|
||
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.
Reporter | ||
Comment 15•9 years ago
|
||
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?
Comment 16•9 years ago
|
||
(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
Comment 18•8 years ago
|
||
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! ;-)
Comment 19•8 years ago
|
||
:keeler - can you revive this useful bug somehow? There is a patch.
Not really my bailiwick. Maybe Tanvi knows who could work on this and/or put it on the content security roadmap?
Comment 21•7 years ago
|
||
Panos, I heard there were plans in this direction. Is there a separate bug and/or can we reuse this patch?
Comment 22•7 years ago
|
||
Thanks for the ping, I'll try to raise the priority of this bug.
Updated•7 years ago
|
Comment 23•7 years ago
|
||
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.
Updated•7 years ago
|
Updated•7 years ago
|
Comment 24•7 years ago
|
||
(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.
Comment 25•7 years ago
|
||
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.
Comment 26•7 years ago
|
||
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.
Comment hidden (obsolete) |
Reporter | ||
Comment 28•7 years ago
|
||
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
Comment 29•7 years ago
|
||
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.
Comment 30•7 years ago
|
||
(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
Comment 31•7 years ago
|
||
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.
Comment 32•7 years ago
|
||
> 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.
Comment 33•7 years ago
|
||
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.
Comment 34•7 years ago
|
||
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! :)
Comment 35•7 years ago
|
||
(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?)
Comment 37•7 years ago
|
||
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...
Comment 38•7 years ago
|
||
(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?
Updated•6 years ago
|
Assignee | ||
Comment 39•6 years ago
|
||
(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 | ||
Comment 40•4 years ago
|
||
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 | ||
Comment 41•4 years ago
|
||
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.
Comment 42•4 years ago
|
||
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?
Comment 43•4 years ago
•
|
||
(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.
Comment 44•4 years ago
|
||
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?
Assignee | ||
Comment 45•4 years ago
|
||
(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.
Assignee | ||
Comment 46•4 years ago
|
||
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.
Comment 47•4 years ago
|
||
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.
Assignee | ||
Comment 48•4 years ago
|
||
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?
Assignee | ||
Updated•4 years ago
|
Comment 49•4 years ago
•
|
||
Could you launch a server on https://[::1]:443/ and try to connect to http://[::1]/? (Or https://127.0.0.13:443/)
Assignee | ||
Comment 50•4 years ago
|
||
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.
Assignee | ||
Comment 51•4 years ago
|
||
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:
- attempt connection to some host via urlbar entry (unspecified port/scheme, defaulting to 80/http),
- let the server reject the attempted connection on port 80 (can be a quick socket accept()/close()), then
- 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.
Hi Sid! We added support for 127.0.0.2
in the test server in https://hg.mozilla.org/mozilla-central/rev/f8632635a60c#l5.14 - would that work? (it gets used here: https://searchfox.org/mozilla-central/rev/54f965e51e4f77866bec42737978d40d4c1fdfc5/dom/webauthn/tests/browser/browser_webauthn_ipaddress.js#38)
Assignee | ||
Comment 53•4 years ago
|
||
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?
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?
Assignee | ||
Comment 55•4 years ago
|
||
Thanks, :keeler!
Assignee | ||
Comment 56•4 years ago
|
||
Limited try run to sanity check:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9023fdbdb425464e68fe50fe7b7023361d6ac27d
Assignee | ||
Comment 57•4 years ago
|
||
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...
Comment 58•4 years ago
|
||
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.
Assignee | ||
Comment 59•4 years ago
•
|
||
(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 abrowser.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
Assignee | ||
Comment 61•4 years ago
•
|
||
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
Assignee | ||
Comment 62•4 years ago
|
||
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.
Assignee | ||
Comment 63•4 years ago
•
|
||
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.
Assignee | ||
Comment 64•4 years ago
|
||
Assignee | ||
Comment 65•4 years ago
|
||
Depends on D75083
Assignee | ||
Comment 66•4 years ago
|
||
Depends on D75084
Assignee | ||
Comment 67•4 years ago
|
||
Depends on D75085
Assignee | ||
Comment 68•4 years ago
|
||
Depends on D75086
Assignee | ||
Comment 69•4 years ago
|
||
Depends on D75087
Assignee | ||
Comment 70•4 years ago
|
||
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.
Assignee | ||
Comment 71•4 years ago
|
||
: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.
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Assignee | ||
Comment 72•4 years ago
|
||
: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...
Assignee | ||
Comment 73•4 years ago
|
||
Thanks for the review, :jaws!
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Comment 74•4 years ago
|
||
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
Comment 75•4 years ago
|
||
Pushed by mozilla@sidstamm.com: https://hg.mozilla.org/integration/autoland/rev/56ba616e2644 use resolvable URL in extension html_detail_view test. r=mstriemer
Comment 76•4 years ago
|
||
Backed out for bustage on nsDocShell.cpp
backout: https://hg.mozilla.org/integration/autoland/rev/7ecaba5a83ce08a17eaa5b9b7976ed8f8e44894c
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....
Assignee | ||
Comment 77•4 years ago
|
||
Whoops, sorry about the bustage. Testing the fix:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=66e2f6528b5e29e2ad815f3d51150f05fc4f61cb
Comment 78•4 years ago
|
||
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
Comment 79•4 years ago
|
||
Backed out 6 changesets (Bug 1002724) for failing in browser_fall_back_to_https.js CLOSED TREE
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=302769989&repo=autoland&lineNumber=21131
Backout: https://hg.mozilla.org/integration/autoland/rev/807de85fcff40339ed6d24a19b62ca28ff7b2712
Assignee | ||
Comment 80•4 years ago
|
||
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)?
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.
Comment 82•4 years ago
|
||
Skipping on macOS sounds right. Fallback on timeout could be a further enhancement.
Assignee | ||
Comment 83•4 years ago
|
||
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!
Comment 84•4 years ago
|
||
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.
Assignee | ||
Comment 85•4 years ago
|
||
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.
Assignee | ||
Comment 86•4 years ago
|
||
Assignee | ||
Comment 87•4 years ago
|
||
: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.
Comment 88•4 years ago
|
||
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
Comment 89•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/11515373dbd2
https://hg.mozilla.org/mozilla-central/rev/9b6944448186
https://hg.mozilla.org/mozilla-central/rev/71cdc10cc959
https://hg.mozilla.org/mozilla-central/rev/d643f8c4a1a1
https://hg.mozilla.org/mozilla-central/rev/cb1683a32099
https://hg.mozilla.org/mozilla-central/rev/22855ae7fa00
Description
•