1897075 - Connection downgrades create new connection

Status: NEW

(Core :: Networking, enhancement, P2)

Description

[Simon Friedberger (:simonf)]

It seems fixing a URI creates a new connection (or load, sorry if I'm messing up the terminology). This causes a failed HTTPS upgrade: HTTP -> HTTPS -> HTTP to start a new connection and some consumers don't understand that.

Specifically, ContentPage is waiting for nsIWebProgressListener.STATE_STOP to determine that a connection has completed at XPCShellContentUtils.sys.mjs#80. Unfortunately, at DocumentLoadListener.cpp#2395 a new load is started when a URI fixup is taking place. This leads to a race between the replaced load finishing and the listener reacting to what it thinks is already complete.

There are a few other uses of STATE_STOP: https://searchfox.org/mozilla-central/search?q=STATE_STOP&path=&case=false&regexp=false so this might very well happen outside of tests.

Is there a better event for listeners to attach to or can we somehow make the downgrade transparent like the upgrade?

Comment 1

[Simon Friedberger (:simonf)]

Example tests that fail because of this would be `

• xpcshell-remote.toml:toolkit/components/extensions/test/xpcshell/test_ext_dnr_private_browsing.js
• uriloader/exthandler/tests/mochitest/browser_download_idn_blocklist.js

See https://bugzilla.mozilla.org/show_bug.cgi?id=1894662 for a patch to reproduce. This problem hasn't shown up so far because HTTPS->HTTP downgrades were broken in most cases when a proxy is used.

Comment 2

[Valentin Gosu [:valentin] (he/him)]

So if I understand the problem correctly, we try to connect to a HTTPS page, there is an error, we end up calling DocumentLoadListener::OnStartRequest which in turn triggers a new HTTP connection.
The problem happens because DocumentLoadListener::OnStopRequest still triggers, causing the load to be completed before the HTTP request comes in.

Ideally, it would have been possible to trigger a redirectTo to the downgraded URI, but we can't do that at this point because the OnStartRequest callback has been called, and any listeners in between nsHttpChannel and DocumentLoadListener might not like OnStartRequest being called more than once 🙂.

Another option might be to teach DocumentLoadListener how to do this additional internal redirect - not sure how difficult that would be.

Or if that doesn't work, we could make DocumentLoadListener ignore the calls to OnStartRequest/OnDataAvailable/OnStopRequest coming from the first channel, once we've fired off the downgraded load, and wait for the next OnStopRequest.

Comment 3

[Valentin Gosu [:valentin] (he/him)]

Nika, do you think any of my previous suggestions are a valid fix for this?

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Would it be possible for the downgrade to happen before the OnStartRequest is fired in the DocumentLoadListener or similar, such that we can do a redirectTo as you suggested?

I think I'm a bit confused as to how the other suggestions you made would help with the potential "any listeners in between nsHttpChannel and DocumentLoadListener might not like OnStartRequest being called more than once" issue you mentioned otherwise, as anything which continues handling this check in DocumentLoadListener seems like it by necessity can only be acting after those listeners are hit.

If we developed some way to allow DocumentLoadListener to do a smooth transition to a new load/channel under the hood after receiving OnStartRequest that could potentially be interesting, though I don't know how frequently we'd use it other than this (e.g. I don't think it'd be super practical to redirect from the failing channel into an error page load to make it a single load request, as we currently need things like the failing channel in the error page process).

Comment 5

[Valentin Gosu [:valentin] (he/him)]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #4)

Would it be possible for the downgrade to happen before the OnStartRequest is fired in the DocumentLoadListener or similar, such that we can do a redirectTo as you suggested?

I was thinking we could add a new "event listener" that would be called before onStartRequest, and DocumentLoadListener could have called RedirectTo. Something like this:

+---------------------+   +------------------+   +--------------------+
|nsHttpChannel        |   |nsHttpCompressConv|   |DocumentLoadListener|
|                     |   |                  |   |                    |
| OnStartRequest      |   |                  |   |                    |
|      ---------------+---+------------------+--->OnBeforeStartRequest|
|                     |   |                  |   |                    |
|       RedirectTo <--+---+------------------+---+-----               |
|         ...         |   |                  |   |                    |
|                     |   |                  |   |                    |
| OnStartRequest      |   |                  |   |                    |
|     ----------------+---+->OnStartRequest--+---+--->OnStartRequest  |
|                     |   |                  |   |                    |
+---------------------+   +------------------+   +--------------------+

I don't think it'd be super practical to redirect from the failing channel into an error page load to make it a single load request, as we currently need things like the failing channel in the error page process

That's a good point. Also I don't think redirect chain normally contains failed channels, so that might make things more complex.

Comment 6

[Simon Friedberger (:simonf)]

Since there are two cases, a failing channel which can be retried and a failing channel which cannot be retried, could we just have two separate events?

Comment 7

[Valentin Gosu [:valentin] (he/him)]

I think that should be possible. I don't know DocumentLoadListener well enough to actually attempt a fix, but I believe the fix should go there.
You could look at whether not calling RejectOpenPromise in DisconnectListeners avoids the race, since it's that bit of the code that triggers nsIWebProgressListener.STATE_STOP.
Or even better, I see there's a continueNavigating parameter being passed to DisconnectListeners, which controls whether we fire STATE_STOP in in the promise rejection code