755467 - SpdySession::RestrictConnections() should use conn->EverUsedSpdy()

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

The patch for 739522 was meant to detect cases where HTTP and SPDY were mixed to the same host and allow 1 new connection when that was true.

It keyed "spdy" off of nsHttpConnection::CanDirectlyActivate() which returns true on spdy connections that can accept more transactions. But it returns false for spdy connections that are shutting down.. while that might be desirable, the purpose of that change was just to find spdy connections, which can be done through mEverUsedSpdy

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: ptach 0

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 624160 [details] [diff] [review]
ptach 0

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

It's questionable whether you should then set mEverUsedSpdy at the bottom of nsHttpConnection::StartSpdy(), when the connection really used SPDY.  

Same for mUsingSpdy, but that is a larger change.

For the test, this OK.

r=honzab

::: netwerk/protocol/http/nsHttpConnection.h
@@ +164,5 @@
>      void BeginIdleMonitoring();
>      void EndIdleMonitoring();
>  
>      bool UsingSpdy() { return mUsingSpdy; }
> +    bool EverUsedSpdy(){ return mEverUsedSpdy; }

Fix the space

Comment 3

[Patrick McManus [:mcmanus]]

(In reply to Honza Bambas (:mayhemer) from comment #2)
> Comment on attachment 624160 [details] [diff] [review]
> ptach 0
> 
> Review of attachment 624160 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It's questionable whether you should then set mEverUsedSpdy at the bottom of
> nsHttpConnection::StartSpdy(), when the connection really used SPDY.  
> 

setting them at the top is right.. NPN has returned spdy/2 so we should only talk spdy. The real bug might be that those error paths don't talk spdy without the spdysession that they're having trouble allocating :) [fortunately those shouldn't be able to happen due to anything other than programming errors]

Comment 4

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/mozilla-central/rev/5c90425ab44e

if this addresses 753663 we'll port it.. but lets find out what it does (if anything) to the nightly crash stats first.