Last Comment Bug 755467 - SpdySession::RestrictConnections() should use conn->EverUsedSpdy()
: SpdySession::RestrictConnections() should use conn->EverUsedSpdy()
Status: RESOLVED FIXED
[spdy][http-conn][qa-]
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: 15 Branch
: x86_64 Linux
: -- normal (vote)
: mozilla15
Assigned To: Patrick McManus [:mcmanus]
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on:
Blocks: 739522 753663
  Show dependency treegraph
 
Reported: 2012-05-15 13:03 PDT by Patrick McManus [:mcmanus]
Modified: 2012-08-14 08:13 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
affected
fixed


Attachments
ptach 0 (2.27 KB, patch)
2012-05-15 13:07 PDT, Patrick McManus [:mcmanus]
honzab.moz: review+
Details | Diff | Splinter Review

Description Patrick McManus [:mcmanus] 2012-05-15 13:03:06 PDT
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] 2012-05-15 13:07:44 PDT
Created attachment 624160 [details] [diff] [review]
ptach 0
Comment 2 Honza Bambas (:mayhemer) 2012-05-15 13:18:09 PDT
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] 2012-05-15 13:22:26 PDT
(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] 2012-05-15 13:42:21 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.