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)
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: mcmanus, Assigned: mcmanus)
Details
Attachments
(1 file)
2.52 KB,
patch
|
sworkman
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=9a37a9e1c82a
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8415413 -
Flags: review?(sworkman)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Comment 3•10 years ago
|
||
(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.
Assignee | ||
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
remote: https://hg.mozilla.org/mozilla-central/rev/b227a707080f
Assignee | ||
Updated•10 years ago
|
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.
Description
•