Closed Bug 755467 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Networking: HTTP, defect)

15 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla15
Tracking Status
firefox13 --- affected
firefox14 --- affected
firefox15 --- fixed

People

(Reporter: mcmanus, Assigned: mcmanus)

References

Details

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

Attachments

(1 file)

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
Attached patch ptach 0Splinter Review
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.
Target Milestone: --- → mozilla15
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [spdy] → [spdy][http-conn]
Whiteboard: [spdy][http-conn] → [spdy][http-conn][qa-]
You need to log in before you can comment on or make changes to this bug.