Closed Bug 1003935 Opened 10 years ago Closed 10 years ago

Call DontReuse on Reclaimed Connections that still have transactions

Categories

(Core :: Networking: HTTP, defect)

29 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: mcmanus, Assigned: mcmanus)

Details

Attachments

(1 file)

nsHttpConnection::BeginIdleMonitoring() asserts that there is no transaction on the connection.. a couple of odd situations can trigger that assert - such as a very well timed reset that triggers the httptransaction::restart logic - the connction is released while the transaction is still on it.

I think the same thing can happen when transactions are canceled.

I don't think anything really bad can come of it - but a reclaimed connection that still has a transaction on it is certainly something that shouldn't be reused.
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
(In reply to Patrick McManus [:mcmanus] from comment #0)
> I don't think anything really bad can come of it - but a reclaimed
> connection that still has a transaction on it is certainly something that
> shouldn't be reused.

Calling DontReuse for transactions that have connections makes sense to me, but can you explain more about the "very well timed reset" please? A quick timeline of events from reset to the transaction still being on the connection might be helpful. Want to understand it more before signing off on it.
this is the assert in question

https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpConnection.cpp#1219

and I think its right. Now if the assert is violated I don't think anything really bad will happen - we'll hang onto a reference to a transaction for a while until the connection is activated again and the reference will be overwritten. So cleaning this up is a matter of enforcing known state a little better - I don't think it is going to fix anything user visible other than a tiny amount of footprint bloat.

but that's not what you asked :)

I was able to do it by changing the c++ code in my sandox to return NET_RESET from writesegments on any transaction with a restart count of 0 _and_ changing the check in nsHttpTransaction.cpp that protects the Restart() logic to be simply if (!mReceivedData).. in effect - every transaction was forced to take a restart.  The well timed reset I referred to earlier would simply be a natural one that triggered the some conditions without my patches - which is certainly possible.

one part of the patch covers that case explicitly, the second part covers it more generically in case there is another path.. which certainly seems possible because reclaim can just be triggered by dropping a reference.
Comment on attachment 8415413 [details] [diff] [review]
call dontreuse on reclaimed conns with transactions

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

Cool - thanks for the explanation.
Attachment #8415413 - Flags: review?(sworkman) → review+
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.