1341572 - Fix multiple HalfOpen socket for a single transaction

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Comment 1

[Dragana Damjanovic [:dragana]]

Attachment: bug_1341572_multiple_halfopen_for_single_transaction_v1.patch

Comment 2

[Patrick McManus [:mcmanus]]

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

Comment 3

[Dragana Damjanovic [:dragana]]

(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().

Comment 4

[Dragana Damjanovic [:dragana]]

Attachment: bug_1341572_multiple_halfopen_for_single_transaction_v2.patch

Comment 5

[Dragana Damjanovic [:dragana]]

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.

Comment 6

[Dragana Damjanovic [:dragana]]

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

[Patrick McManus [:mcmanus]]

(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

[Patrick McManus [:mcmanus]]

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?

Comment 9

[Dragana Damjanovic [:dragana]]

(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 :)

Comment 10

[Dragana Damjanovic [:dragana]]

(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.

Comment 11

[Dragana Damjanovic [:dragana]]

Attachment: bug_1341572_multiple_halfopen_for_single_transaction_v3.patch

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 13

[Dragana Damjanovic [:dragana]]

Attachment: bug_1341572_multiple_halfopen_for_single_transaction_v3.patch

rebased

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca1f19ad3de6
Fix multiple HalfOpen socket for a single transaction. r=mcmanus

Comment 15

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures in https://treeherder.mozilla.org/logviewer.html#?job_id=82454758&repo=mozilla-inbound

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/033f1a67064d
Backed out changeset ca1f19ad3de6 for causing test failures in test_altsvc.js

Comment 17

[Dragana Damjanovic [:dragana]]

Attachment: bug_1341572_multiple_halfopen_for_single_transaction_v3.patch

Comment 18

[Dragana Damjanovic [:dragana]]

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c1e62cba5a5b9e85f266d49c68002e7e484ca66

Comment 19

[Dragana Damjanovic [:dragana]]

try looks good.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 21

[Dragana Damjanovic [:dragana]]

Attachment: bug_1341572_multiple_halfopen_for_single_transaction_v3.patch

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.

Comment 22

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=79da51ab814fa1f312d853f187f735867152b614

Comment 23

[Dragana Damjanovic [:dragana]]

thanks!

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1f93ab32013
Fix multiple HalfOpen socket for a single transaction. r=mcmanus

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e1f93ab32013