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

RESOLVED FIXED in Firefox 15

Status

()

Core
Networking: HTTP
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

15 Branch
mozilla15
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox13 affected, firefox14 affected, firefox15 fixed)

Details

(Whiteboard: [spdy][http-conn][qa-])

Attachments

(1 attachment)

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
Blocks: 739522
Blocks: 753663
Created attachment 624160 [details] [diff] [review]
ptach 0
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Attachment #624160 - Flags: review?(honzab.moz)
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
Attachment #624160 - Flags: review?(honzab.moz) → review+
(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]
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.
status-firefox13: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → fixed
Target Milestone: --- → mozilla15
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [spdy] → [spdy][http-conn]

Updated

5 years ago
Whiteboard: [spdy][http-conn] → [spdy][http-conn][qa-]
You need to log in before you can comment on or make changes to this bug.