890691 - SPDY connections block after a hard refresh

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect)

Description

[Maxime]

A page hosted on Domain A loads images from spdy enabled Domain B (see link).
After doing a ctrl-refresh, only the first image is loaded and Firefox gets seemingly stuck in the "Transferring data from..." state. 
Opening a new tab (middle clicking on one of the images for example) "unstucks" Firefox.

This only happens when domains A and B, or protocols, are different.

This behaviour appeared in FF18 as far as I know.

Comment 1

[Alice0775 White]

This happens with clean profile.

Regression window(beta)
Good:
http://hg.mozilla.org/releases/mozilla-beta/rev/f5b325a7d5c6
Mozilla/5.0 (X11; Linux i686; rv:18.0) Gecko/18 Firefox/18.0 ID:20121123132620
Bad:
http://hg.mozilla.org/releases/mozilla-beta/rev/d82726e5866f
Mozilla/5.0 (X11; Linux i686; rv:18.0) Gecko/18 Firefox/18.0 ID:20121126195203
Pushlog:
http://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=f5b325a7d5c6&tochange=d82726e5866f


Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/ab099c9e1a09
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121104012747
Bad:
http://hg.mozilla.org/mozilla-central/rev/2937fd8e35a1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121104152246
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ab099c9e1a09&tochange=2937fd8e35a1

Suspected:
82795635cc76	Patrick McManus — bug 779467 modify ClosePersistentConnections to accomodate sticky auth r=honzab

Comment 2

[Patrick McManus [:mcmanus]]

maxime, thanks for the really good testcase. I see the problem you describe clearly, but it looks like bbs and static are both running spdy/2 right now so the issue probably isn't due to mixing protocols. I'll take a deeper look.

Comment 3

[Patrick McManus [:mcmanus]]

Attachment: spdy latency problem with control refresh

Comment 4

[Patrick McManus [:mcmanus]]

(In reply to Patrick McManus [:mcmanus] from comment #3)
> Created attachment 772474 [details] [diff] [review]
> spdy latency problem with control refresh

so spdy didn't really have the concept of !ALLOW_KEEPALIVE, since its so inherent to the protocol. We certainly don't allow it to be negotiated by the server as we do for HTTP/1; but I decided to let it remain for the ctrl refresh scenario.

So the patch has these parts:

1] close down connections that have served !ALLOW_KEEPALIVE
2] allow more than 1 spdy connection if we are serving !ALLOW_KEEPALIVE
3] don't put a !ALLOW_KEEPALIVE transaction on a spdy session that has served a transaction already
4] don't let existing ALLOW_KEEPALIVE sessions to starve the need for more !ALLOW_KEEPALIVE sessions

Comment 5

[u408661]

Comment on attachment 772474 [details] [diff] [review]
spdy latency problem with control refresh

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

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +60,5 @@
>      , mCompletedProxyConnect(false)
>      , mLastTransactionExpectedNoContent(false)
>      , mIdleMonitoring(false)
>      , mProxyConnectInProgress(false)
> +    , mExperienced(false)

Maybe rename this mEverUsedNonNullTransaction (or maybe 'serviced' instead of 'used')? It's wordier, to be sure, but much more understandable. That would mean changing IsExperienced() to something similar, as well.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +1664,5 @@
> +        // connection to make room for a new one that can service a no KEEPALIVE
> +        // request
> +        unusedSpdyPersistentConnection->DontReuse();
> +    }
> +    

nit: trailing whitespace

Comment 6

[Patrick McManus [:mcmanus]]

Thanks - in the end I decided to keep the naming.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5af330b0bd0

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 772474 [details] [diff] [review]
spdy latency problem with control refresh

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

::: netwerk/protocol/http/nsHttpConnection.h
@@ +231,5 @@
>      bool                            mCompletedProxyConnect;
>      bool                            mLastTransactionExpectedNoContent;
>      bool                            mIdleMonitoring;
>      bool                            mProxyConnectInProgress;
> +    bool                            mExperienced;

The comment (really needed with this name..) should be here, not at the getter imo.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b5af330b0bd0

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

This doesn't meet criteria for tracking since it's been present in builds for so long but if the fix is low risk enough please nominate for uplift consideration at least to Aurora.

Comment 10

[u279076]

Maxime, can you please verify this is fixed for you in the latest Aurora builds?

Comment 11

[Maxime]

Seems to work fine in 25.0a2 (2013-09-10)

Thank you.

Comment 12

[u279076]

(In reply to Maxime from comment #11)
> Seems to work fine in 25.0a2 (2013-09-10)

Thanks Maxime, calling this verified fixed.