Fix multiple HalfOpen socket for a single transaction

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: dragana, Assigned: dragana)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(1 attachment, 5 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Whiteboard: [necko-active]
(Assignee)

Comment 1

2 years ago
Created attachment 8840402 [details] [diff] [review]
bug_1341572_multiple_halfopen_for_single_transaction_v1.patch
Attachment #8840402 - Flags: review?(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
Attachment #8840402 - Flags: review?(mcmanus)
(Assignee)

Comment 3

2 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

2 years ago
Created attachment 8842508 [details] [diff] [review]
bug_1341572_multiple_halfopen_for_single_transaction_v2.patch
Attachment #8840402 - Attachment is obsolete: true
Attachment #8842508 - Flags: review?(mcmanus)
(Assignee)

Comment 5

2 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

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

Comment 9

2 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

2 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)

Updated

2 years ago
Blocks: 1344171
(Assignee)

Comment 11

2 years ago
Created attachment 8843885 [details] [diff] [review]
bug_1341572_multiple_halfopen_for_single_transaction_v3.patch
Attachment #8842508 - Attachment is obsolete: true
Attachment #8843885 - Flags: review?(mcmanus)
Attachment #8843885 - Flags: review?(mcmanus) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
(Assignee)

Comment 13

2 years ago
Created attachment 8844872 [details] [diff] [review]
bug_1341572_multiple_halfopen_for_single_transaction_v3.patch

rebased
Attachment #8843885 - Attachment is obsolete: true
Attachment #8844872 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 14

2 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
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

2 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

2 years ago
Created attachment 8845901 [details] [diff] [review]
bug_1341572_multiple_halfopen_for_single_transaction_v3.patch
Attachment #8844872 - Attachment is obsolete: true
Flags: needinfo?(dd.mozilla)
Attachment #8845901 - Flags: review+
(Assignee)

Comment 19

2 years ago
try looks good.
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
(Assignee)

Comment 21

2 years ago
Created attachment 8848211 [details] [diff] [review]
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.
Attachment #8845901 - Attachment is obsolete: true
Attachment #8848211 - Flags: review?(mcmanus)
Attachment #8848211 - Flags: review?(mcmanus) → review+
(Assignee)

Comment 23

2 years ago
thanks!

Comment 24

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e1f93ab32013
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1355539
(Assignee)

Updated

a year ago
No longer depends on: 1355539
You need to log in before you can comment on or make changes to this bug.