Closed Bug 1341572 Opened 3 years ago Closed 3 years ago

Fix multiple HalfOpen socket for a single transaction

Categories

(Core :: Networking: HTTP, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: dragana, Assigned: dragana)

References

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 5 obsolete files)

No description provided.
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Whiteboard: [necko-active]
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)
(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().
Attachment #8840402 - Attachment is obsolete: true
Attachment #8842508 - Flags: review?(mcmanus)
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.
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.
(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 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)
(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 :)
(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.
Blocks: 1344171
Attachment #8842508 - Attachment is obsolete: true
Attachment #8843885 - Flags: review?(mcmanus)
Attachment #8843885 - Flags: review?(mcmanus) → review+
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
rebased
Attachment #8843885 - Attachment is obsolete: true
Attachment #8844872 - Flags: review+
Keywords: checkin-needed
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
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)
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
Attachment #8844872 - Attachment is obsolete: true
Flags: needinfo?(dd.mozilla)
Attachment #8845901 - Flags: review+
try looks good.
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
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)
Attachment #8848211 - Flags: review?(mcmanus) → review+
thanks!
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
https://hg.mozilla.org/mozilla-central/rev/e1f93ab32013
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1355539
No longer depends on: 1355539
You need to log in before you can comment on or make changes to this bug.