Closed
Bug 1341572
Opened 8 years ago
Closed 8 years ago
Fix multiple HalfOpen socket for a single transaction
Categories
(Core :: Networking: HTTP, defect)
Core
Networking: HTTP
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: dragana, Assigned: dragana)
References
Details
(Whiteboard: [necko-active])
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Whiteboard: [necko-active]
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8840402 -
Flags: review?(mcmanus)
Comment 2•8 years ago
|
||
Comment on attachment 8840402 [details] [diff] [review]
bug_1341572_multiple_halfopen_for_single_transaction_v1.patch
Review of attachment 8840402 [details] [diff] [review]:
-----------------------------------------------------------------
I really like this arrangement - its a big improvement. Thanks for doing the work.
I have just one concern about a dangling ptr..
::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +636,5 @@
> }
>
> + if (trans &&
> + preferred->mPendingQ.Contains(trans,
> + nsHttpConnectionMgr::PendingTransactionInfo::MatchTransaction()))
reads easier as PendingTransactionInfo::MatchTransaction(), you might even think of making it a class of nsHttpConnectionMgr called PendingComparator which would make a lot of these callsites easier to understand imo
@@ +880,5 @@
> ent->mIdleConns.Length(), ent->mPendingQ.Length()));
>
> ProcessSpdyPendingQ(ent);
>
> + RefPtr<PendingTransactionInfo> pendingTransInfo;
I think this should be a raw ptr for the "now potentially destroyed" comment to apply towards the bottom of the function.
@@ +902,5 @@
> // (if found) for transactions referred by a half-open connection.
> + bool alreadyHalfOpenOrWaitingForTLS = false;
> + if (pendingTransInfo->mHalfOpen) {
> + MOZ_ASSERT(!pendingTransInfo->mActiveConn);
> + for (int32_t j = 0;
would unsigned work? if not, c++ casts on the next line pls.
@@ +918,5 @@
> + break;
> + }
> + }
> + if (!alreadyHalfOpenOrWaitingForTLS) {
> + // If we have not found the halfOpen socket, remove the pointer.
I'm not 100% sure, but this makes it sound like this is a free'd raw pointer and that's why it is being cleared. That's ok, but if that's true I think some new allocation could have been made and received the same pointer value.. it could have even ended up in ent->mHalfOpens[] and screwed up the above comparison, right?
if it were a refptr that won't be a concern (the value couldn't be recycled) - but I'm not sure if that would be a ref cycle or not. the activeconn below is a refptr fwiw.
@@ +955,1 @@
> // trans is now potentially destroyed
s/trans/pendingTransInfo
@@ +2033,5 @@
> ConditionallyStopTimeoutTick();
> }
>
> +void
> +nsHttpConnectionMgr::ReleaseClaimedSockets(nsConnectionEntry *ent,
iterators in ere should be unsigned or use c++ casts
@@ +2119,5 @@
> // Dispatch all the transactions we can
> for (index = 0;
> index < ent->mPendingQ.Length() && conn->CanDirectlyActivate();
> ++index) {
> + RefPtr<PendingTransactionInfo> pendingTransInfo = ent->mPendingQ[index];
if we can keep it to raw pointers just on the stack that's useful. refcounts are atomic and relatively expensive.. I'm happy to err on the side of using them, but I'm not seeing why the logic would change here..
::: netwerk/protocol/http/nsHttpConnectionMgr.h
@@ +497,5 @@
> + {}
> +
> + NS_INLINE_DECL_THREADSAFE_REFCOUNTING(PendingTransactionInfo)
> +
> + struct MatchTransaction {
its possible I just spent too many years with C, but can we call this a class with a public method please?
@@ +511,5 @@
> + nsHalfOpenSocket* mHalfOpen;
> + RefPtr<nsHttpConnection> mActiveConn;
> +
> + private:
> + virtual ~PendingTransactionInfo() {}
I like the private dtor
Attachment #8840402 -
Flags: review?(mcmanus)
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Patrick McManus [:mcmanus] from comment #2)
> @@ +918,5 @@
> > + break;
> > + }
> > + }
> > + if (!alreadyHalfOpenOrWaitingForTLS) {
> > + // If we have not found the halfOpen socket, remove the pointer.
>
> I'm not 100% sure, but this makes it sound like this is a free'd raw pointer
> and that's why it is being cleared. That's ok, but if that's true I think
> some new allocation could have been made and received the same pointer
> value.. it could have even ended up in ent->mHalfOpens[] and screwed up the
> above comparison, right?
>
You are right.
This is a bit tricky. mHalfOpens is array of raw pointers and halfOpenSocket is remove from the array in its destructor. If we use refPtr it will not be removed. I will look into this, i.e. if a halfOpenSocket is always properly Abandon().
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8840402 -
Attachment is obsolete: true
Attachment #8842508 -
Flags: review?(mcmanus)
Assignee | ||
Comment 5•8 years ago
|
||
A different solution for the problem from comment 3 is to remove reference to halfOpen from any PendingTransactionInfo when halfOpen is removed from nsConnectionEntry::mHalfOpens (in nsConnectionEntry::RemoveHalfOpen). In that case we can keep mHalfOpens being only raw pointers.
Assignee | ||
Comment 6•8 years ago
|
||
While testing this and looking through log, I notice:
if we have pending transactions but all of them are block("blocked by request context"), a conn will be idle after connect, although it is ssl and could use a nullTransaction for the handshake.
This is probably rare case.
Comment 7•8 years ago
|
||
(In reply to Dragana Damjanovic [:dragana] from comment #6)
> While testing this and looking through log, I notice:
> if we have pending transactions but all of them are block("blocked by
> request context"), a conn will be idle after connect, although it is ssl and
> could use a nullTransaction for the handshake.
>
> This is probably rare case.
could we whitelist those through somehow (in another patch)
Comment 8•8 years ago
|
||
Comment on attachment 8842508 [details] [diff] [review]
bug_1341572_multiple_halfopen_for_single_transaction_v2.patch
Review of attachment 8842508 [details] [diff] [review]:
-----------------------------------------------------------------
I'm concerned that this might leak in some error path. How confident are you?
It seems like an easy fix would be to leave the removehalfOpen() in the dtor, but change the PendingTransactionInfo::mHalfOpen from a RefPtr to a mfbt::WeakPtr.. that way its lifecycle would be unchanged but you would get an automatic clearing of the WeakPtr on dtor.
wdyt?
Attachment #8842508 -
Flags: review?(mcmanus)
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to Patrick McManus [:mcmanus] from comment #8)
> Comment on attachment 8842508 [details] [diff] [review]
> bug_1341572_multiple_halfopen_for_single_transaction_v2.patch
>
> Review of attachment 8842508 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> I'm concerned that this might leak in some error path. How confident are you?
>
> It seems like an easy fix would be to leave the removehalfOpen() in the
> dtor, but change the PendingTransactionInfo::mHalfOpen from a RefPtr to a
> mfbt::WeakPtr.. that way its lifecycle would be unchanged but you would get
> an automatic clearing of the WeakPtr on dtor.
>
> wdyt?
I pretty sure it will not leak, but your suggestion is good, we can remove loop as well. That loop runs very often :)
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to Patrick McManus [:mcmanus] from comment #7)
> (In reply to Dragana Damjanovic [:dragana] from comment #6)
> > While testing this and looking through log, I notice:
> > if we have pending transactions but all of them are block("blocked by
> > request context"), a conn will be idle after connect, although it is ssl and
> > could use a nullTransaction for the handshake.
> >
> > This is probably rare case.
>
> could we whitelist those through somehow (in another patch)
Yes, that was my intention to fix it in a separate bug, I just wrote it here not to forget.
Assignee | ||
Comment 11•8 years ago
|
||
Attachment #8842508 -
Attachment is obsolete: true
Attachment #8843885 -
Flags: review?(mcmanus)
Updated•8 years ago
|
Attachment #8843885 -
Flags: review?(mcmanus) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 13•8 years ago
|
||
rebased
Attachment #8843885 -
Attachment is obsolete: true
Attachment #8844872 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 14•8 years ago
|
||
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca1f19ad3de6
Fix multiple HalfOpen socket for a single transaction. r=mcmanus
Keywords: checkin-needed
Comment 15•8 years ago
|
||
sorry had to back this out for test failures in https://treeherder.mozilla.org/logviewer.html#?job_id=82454758&repo=mozilla-inbound
Flags: needinfo?(dd.mozilla)
Comment 16•8 years ago
|
||
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/033f1a67064d
Backed out changeset ca1f19ad3de6 for causing test failures in test_altsvc.js
Assignee | ||
Comment 17•8 years ago
|
||
Attachment #8844872 -
Attachment is obsolete: true
Flags: needinfo?(dd.mozilla)
Attachment #8845901 -
Flags: review+
Assignee | ||
Comment 18•8 years ago
|
||
Assignee | ||
Comment 19•8 years ago
|
||
try looks good.
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 21•8 years ago
|
||
Can you take a quick look, a lot of change because of UrgentStart. Loggically the code is the same only now we have 2 pending queues.
Attachment #8845901 -
Attachment is obsolete: true
Attachment #8848211 -
Flags: review?(mcmanus)
Assignee | ||
Comment 22•8 years ago
|
||
Updated•8 years ago
|
Attachment #8848211 -
Flags: review?(mcmanus) → review+
Assignee | ||
Comment 23•8 years ago
|
||
thanks!
Comment 24•8 years ago
|
||
Pushed by dd.mozilla@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1f93ab32013
Fix multiple HalfOpen socket for a single transaction. r=mcmanus
Comment 25•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•