1133177 - Cannot load certain https site using SPDY and HTTP/2 proxy

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Tatsuhiro Tsujikawa]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/40.0.2214.91 Safari/537.36

Steps to reproduce:

1. Set up SPDY or HTTP/2 secure proxy (node-spdyproxy or nghttpx + squid)
2. Instruct Firefox nightly to use the secure proxy
3. Navigate to https://www.libssh.org


Actual results:

The web page was not fully loaded.  From network developer tool attached in Firefox, some resources (e.g., js) were not loaded.  It just logged Request headers, and no Response headers.  It seems cleartext http site does not exhibit this issue.  Only several https sites I observed this issue.  Meanwhile, https://youtube.com, https://google.com, https://www.mozilla.org are all fine.
If we negotiate http/1.1 between firefox and proxy, site is fully rendered correctly.  If spdy/3.1 or h2-16 (h2-14) is negotiated between firefox and proxy, we have this issue.



Expected results:

Web site should be fully loaded normally.

Comment 1

[Patrick McManus [:mcmanus]]

I can confirm it. Weirdly the troubled resources appear to be on the https://www.libssh.org origin itself which is running h1 (inside the spdy tunnel).. the usual tricky cases involve spdy/h2 inside spdy/h2, which seems to be working fine.

Comment 2

[Michael Newton]

I see similar behaviour at https://http2rulez.com/ where everything loads perfectly and quickly aside from the stylesheets. The request appears to be made, but the stylesheets are never loaded.

No page rendering is done while waiting for these files to load, either. Once I stop page loading, I am shown the unstyled page.

Requesting the CSS files directly loads them just fine; a subsequent browse to the site loads them just fine from cache.

Screenshot: https://www.dropbox.com/s/lgda9wg7rcgqe5q/Screen%20Shot%202015-02-18%20at%202.26.01%20PM.png?dl=0

Comment 3

[Michael Newton]

Sorry, just noticed this bug relates to viewing through a proxy. Disregard, as I am not on a proxy server. Apologies for the bug spam.

Comment 4

[Patrick McManus [:mcmanus]]

we get into a state where we choose not to make another h2 CONNECT tunnel to libssh.org (h1 is carried inside), because we have 6 of them already open (all inside the single h2 connection to the proxy).. and then libssh.org is sometimes returning h1 responses with "connection closed", which tears down those tunnels instead of making them eligible for reuse.. there is a race condition in there somewhere where we don't notice and then establish a new tunnel.

Comment 5

[Patrick McManus [:mcmanus]]

Attachment: tunnel dispatch logging (1 of 2)

Comment 6

[Patrick McManus [:mcmanus]]

Attachment: https tunnel of h1 without pconn inside h2 session stall (2 of 2)

Comment 7

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d31781ff0b9a

patch 1 is just a bunch of housekeeping.. enhanced logging that let me track down the problem and removal of some old allocation failure checks from the days before infallible new.

patch 2 is the bugfix.

Comment 8

[u408661]

Comment on attachment 8566649 [details] [diff] [review]
https tunnel of h1 without pconn inside h2 session stall (2 of 2)

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

Just a few nits

::: netwerk/protocol/http/Http2Session.cpp
@@ +3328,5 @@
> +{
> +  MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread);
> +  nsHttpTransaction *trans = aHttpTransaction->QueryHttpTransaction();
> +  LOG(("Http2Session::MaybeReTunnel %p trans=%p\n", this, trans));
> +  nsHttpConnectionInfo *ci = aHttpTransaction->ConnectionInfo();

move this down closer to where it's used.

::: netwerk/protocol/http/SpdySession31.cpp
@@ +2713,5 @@
>  
>  void
> +SpdySession31::CreateTunnel(nsHttpTransaction *trans,
> +                            nsHttpConnectionInfo *ci,
> +                            nsIInterfaceRequestor *aCallbacks) 

nit: trailing whitespace

@@ +2768,5 @@
> +{
> +  MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread);
> +  nsHttpTransaction *trans = aHttpTransaction->QueryHttpTransaction();
> +  LOG(("SpdySession31::MaybeReTunnel %p trans=%p\n", this, trans));
> +  nsHttpConnectionInfo *ci = aHttpTransaction->ConnectionInfo();

move closer to use

::: netwerk/protocol/http/nsHttpTransaction.h
@@ +407,5 @@
>  private:
>      uint32_t mClassOfService;
> +
> +public:
> +    // setting mDontRouteViaWildCard to true means the transaction should only

comment needs rewording, since we no longer have mDontRouteViaWildCard :)

Comment 9

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f34ddbf1e490
https://hg.mozilla.org/integration/mozilla-inbound/rev/b465cd661b6b

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f34ddbf1e490
https://hg.mozilla.org/mozilla-central/rev/b465cd661b6b

Comment 11

[Tatsuhiro Tsujikawa]

I confirmed this issue was fixed with latest nightly.  Awesome, guys.  Thank you very much.

Comment 12

[Patrick McManus [:mcmanus]]

Comment on attachment 8566649 [details] [diff] [review]
https tunnel of h1 without pconn inside h2 session stall (2 of 2)

I am asking for approval gecko-37. I marked that beta but the channels are about to change versions, so it's confusing.

This is a bug in a feature that shipped as part of 33. It can lead to serious stalls when browsing when using HTTPS proxying - its good to backport the fix one interval.

Approval Request Comment
[Feature/regressing bug #]:https proxying (feature 378637)
[User impact if declined]: some origins will have resources intermittently fail to load when proxying over https - that feature landed in ff33
[Describe test coverage new/current, TreeHerder]: fix confirmed by reporter
[Risks and why]: its not a big patch, but we don't have automated proxy testing coverage in general beyond a few trivial things. It has been tested and confirmed by the reporter
[String/UUID change made/needed]: none

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8566649 [details] [diff] [review]
https tunnel of h1 without pconn inside h2 session stall (2 of 2)

This is a larger patch than I normally like to take on Beta but the fix has been on m-c for 6 days and the submitter has verified the fix (comment 11) in Nightly. Let's take this fix for Beta 2. Beta+

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f321e120f8e1
https://hg.mozilla.org/releases/mozilla-beta/rev/1c270f40087a