905460 - convert legacy manual http ref counts to smart pointers

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

There are a slew of HTTP objects that have always been managed with hard reference pointers and the ADDREF and RELEASE macros. I'm generally ok with leaving legacy code that works untouched, but these are all in files and code paths that are actively developed - so they should be cleaned up and become smart pointers.

https://tbpl.mozilla.org/?tree=Try&rev=b08030487f08

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: make http objects use smart pointers

Comment 2

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 790563 [details] [diff] [review]
make http objects use smart pointers

Review of attachment 790563 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good r=me.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +278,3 @@
>      nsresult rv = PostEvent(&nsHttpConnectionMgr::OnMsgNewTransaction, priority, trans);
> +    if (NS_SUCCEEDED(rv))
> +        trans.forget();

I spent some time trying to determine if there was a way for PostEvent to be responsible for the release in failure cases here (i.e. it would be nice if we could just pass aTrans directly to PostEvent) ... but since this param is a void* and there isn't really a base class for classes implementing AddRef/Release ... this is probably the safest way to do this.

@@ +2255,5 @@
>              if (conn == ent->mYellowConnection)
>                  ent->OnYellowComplete();
> +
> +            // drop a reference that was held by list
> +            conn.get()->Release();

From the rest of the review, it looks like you're converting the pointers in stages, right? So, I'm going to throw out there the idea of using nsTArray<nsRefPtr<nsHttpConnection> > for mActiveConns and mIdleConns. This might allow you to avoid using AddRef and Release here directly.

Comment 3

[Patrick McManus [:mcmanus]]

(In reply to Steve Workman [:sworkman] from comment #2)
> Comment on attachment 790563 [details] [diff] [review]
>
> From the rest of the review, it looks like you're converting the pointers in
> stages, right? So, I'm going to throw out there the idea of using
> nsTArray<nsRefPtr<nsHttpConnection> > for mActiveConns and mIdleConns. This
> might allow you to avoid using AddRef and Release here directly.

yes, that would be a good patch too.. its just a little less mechanical than this bigger one.

Comment 4

[Patrick McManus [:mcmanus]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7d3ba33cc1ad

Comment 5

[[e-mail]]

(In reply to Steve Workman from comment #2)
> I spent some time trying to determine if there was a way for PostEvent to be
> responsible for the release in failure cases here (i.e. it would be nice if
> we could just pass aTrans directly to PostEvent) ... but since this param is
> a void* and there isn't really a base class for classes implementing
> AddRef/Release ... this is probably the safest way to do this.
There is nsISupports but it doesn't help you here because sadly all sorts of junk gets passed in as param.

> From the rest of the review, it looks like you're converting the pointers in
> stages, right? So, I'm going to throw out there the idea of using
> nsTArray<nsRefPtr<nsHttpConnection> > for mActiveConns and mIdleConns. This
> might allow you to avoid using AddRef and Release here directly.
As mcmanus points out he has to be careful because the nsTArray<nsRefPtr> will drop the reference when the element is removed but the manual refcounting sometimes used the value after removing it and before dropping the reference.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7d3ba33cc1ad

Comment 7

[Patrick McManus [:mcmanus]]

backed out in 907960

Comment 8

[Patrick McManus [:mcmanus]]

dusting this one out of my queue.. obviously from the stack traces an nsHttpConnection object was being double freed.

I think I know the problem here.. ~nsHttpConnectionMgr.cpp:1833 you'll find NS_RELEASE(handle->mConn) .. but I had changed the type of mConn from nsHttpConnection * to be nsRefPtr<nsHttpConnection>.. so the net result would be a double release because NS_RELEASE expands to foo->Relase() (release 1) and foo = nullptr (release 2 if foo is now a smart ptr).

I had converted the other uses of it away from NS_ADDREF/NS_RELEASE - just missed this one.

There were some other remaining ADDREF/RELEASE macros in use that I also cleaned up for sanity (even if they are effectively still manually reference counting).

I'll post it for review again when try approves.

Comment 9

[Patrick McManus [:mcmanus]]

Attachment: convert legacy manual http ref counts to smart pointers

Comment 10

[Patrick McManus [:mcmanus]]

https://tbpl.mozilla.org/?tree=Try&rev=9bce4fc34a60

Comment 11

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8362107 [details] [diff] [review]
convert legacy manual http ref counts to smart pointers

Review of attachment 8362107 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +553,5 @@
>  
>      conn->Close(NS_ERROR_ABORT);
> +
> +    // the conn held a ref on the idle list that can now be released
> +    conn->Release();

So, NS_RELEASE would also set conn to nullptr. It's not used in the rest of the function, but it's safer to null it anyway, right?

I see this conversion to ->Release() in a few places - I assume you're making this change to remove NS_ADDREF/RELEASE from the code, right? Otherwise, I'd suggest just sticking with NS_RELEASE or using a smart pointer, but I see that a smart pointer is a little bit cumbersome (two conn->forget() calls in this function, at line 546 and 542). And you mentioned "mechanical patch" previously. No perfect solution; up to you. (Although I'd still like to see conn nulled out :) ).

Comment 12

[Patrick McManus [:mcmanus]]

Attachment: [PATCH 1/1] - necko cleanups: manual ref counts, 32 bit

From d67dcefe8a38163ec01af0bebf4417f4e484cf0d Mon Sep 17 00:00:00 2001
 formatters, whitespace r?sworkman
---
 netwerk/protocol/http/nsHttpConnection.cpp     |   8 +-
 netwerk/protocol/http/nsHttpConnectionInfo.cpp |   2 +-
 netwerk/protocol/http/nsHttpConnectionMgr.cpp  | 286 +++++++++++--------------
 netwerk/protocol/http/nsHttpConnectionMgr.h    |  18 +-
 netwerk/protocol/http/nsHttpHandler.cpp        |  20 +-
 netwerk/protocol/http/nsHttpHandler.h          |   2 +-
 netwerk/protocol/http/nsHttpPipeline.cpp       |  39 ++--
 netwerk/protocol/http/nsHttpPipeline.h         |   2 +-
 netwerk/protocol/http/nsHttpTransaction.cpp    |  17 +-
 netwerk/protocol/http/nsHttpTransaction.h      |   7 +-
 10 files changed, 177 insertions(+), 224 deletions(-)

Comment 13

[Patrick McManus [:mcmanus]]

Attachment: necko cleanups: manual ref counts, 32 bit formatters, whitespace

Comment 14

[Patrick McManus [:mcmanus]]

As long as the ref counts on those lists were manually managed I preferred the ->Release(), ->AddRef() over .forget/dont_AddRef which seemed to be actually confusing things.

but it became clear that we should bite the bullet and convert mIdleConns, mActiveConns, and mPendingQ to be nsTArray<<nsRefPtr<foo> > afterall and drop the manual counting. That's where we wanted to end up anyhow - and an incremental strategy wasn't making anybody happy.. so I added that to this patch and had to add a few nsRefPtr<foo> deleteProtector(bar) style reference holders to make things work out when they were removed from array, closed, and then released. So the review got a bit more complicated.

Doing all of this kind of turned this into a generic cleanup patch, so a bunch of formatting changes are coming along for the ride.

Comment 15

[Patrick McManus [:mcmanus]]

https://tbpl.mozilla.org/?tree=Try&rev=57ff58a0a592

Comment 16

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8374087 [details] [diff] [review]
necko cleanups: manual ref counts, 32 bit formatters, whitespace

Review of attachment 8374087 [details] [diff] [review]:
-----------------------------------------------------------------

Really like these changes, Pat, and appreciate the time put into nsHttpConnectionMgr.cpp and the various arrays.

Just some minor comments. r=me

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +534,5 @@
>  {
>      MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread);
>      LOG(("nsHttpConnectionMgr::CloseIdleConnection %p conn=%p",
>           this, conn));
> +    nsRefPtr<nsHttpConnection> deleteProtector(conn);

Can you also add a comment in nsHttpConnection::OnInputStreamReady where CloseIdleConnection is called please? Something like "Must return immediately; deleting 'this'".

@@ +1840,5 @@
>         on a nsHttpPipeline object is not a real HTTP pipeline */
>  
>      nsRefPtr<nsHttpPipeline> pipeline = new nsHttpPipeline();
>      pipeline->AddTransaction(firstTrans);
> +    *result = pipeline.forget().get();

not pipeline.forget(result);?

@@ +1935,5 @@
>  void
>  nsHttpConnectionMgr::AddActiveConn(nsHttpConnection *conn,
>                                     nsConnectionEntry *ent)
>  {
> +    // list holds a ref

Don't think we need this comment.

@@ +2030,5 @@
>  
>      // Put the leftovers back in the pending queue and get rid of the
>      // transactions we dispatched
>      leftovers.SwapElements(ent->mPendingQ);
>      leftovers.Clear();

Just out of interest, any reason why not RemoveElementsAt(0, index-1) rather than using the leftovers array? Or use a while loop and remove single elements as you go?

@@ +2528,5 @@
> +nsHttpConnectionMgr::
> +InsertTransactionSorted(nsTArray<nsRefPtr<nsHttpTransaction> > &pendingQ,
> +                        nsHttpTransaction *trans)
> +{
> +    // insert into queue with smallest valued number first.  search in reverse

Since you're moving this, can you capitalise 'insert' and 'search', and remove the redundant space before 'search'.

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +385,1 @@
>          mConnMgr = new nsHttpConnectionMgr();

Braces.

Comment 17

[Patrick McManus [:mcmanus]]

(In reply to Steve Workman [:sworkman] from comment #16)

> 
> @@ +2030,5 @@
> >  
> >      // Put the leftovers back in the pending queue and get rid of the
> >      // transactions we dispatched
> >      leftovers.SwapElements(ent->mPendingQ);
> >      leftovers.Clear();
> 
> Just out of interest, any reason why not RemoveElementsAt(0, index-1) rather
> than using the leftovers array? 

some of the elements in that range actually end up in the leftovers array (see the caps() check with the continue)

Or use a while loop and remove single
> elements as you go?

its an ordered queue - so you need to process it from the front to the back. In the normal case (where everything is accepted and nothing goes to leftovers) that would involved O(n^2) memmoves of the array vs the leftovers case which is basically a nop.

Comment 18

[Patrick McManus [:mcmanus]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fc25d495f995

Comment 19

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Patrick McManus [:mcmanus] from comment #17)
> (In reply to Steve Workman [:sworkman] from comment #16)
> > Just out of interest, any reason why not RemoveElementsAt(0, index-1) rather
> > than using the leftovers array? 
> 
> some of the elements in that range actually end up in the leftovers array
> (see the caps() check with the continue)

Ah, yes. I totally missed that. Makes perfect sense.

> Or use a while loop and remove single
> > elements as you go?
> 
> its an ordered queue - so you need to process it from the front to the back.
> In the normal case (where everything is accepted and nothing goes to
> leftovers) that would involved O(n^2) memmoves of the array vs the leftovers
> case which is basically a nop.

Of course. I was confusing the deletion of the objects with the array of pointers.

Comment 20

[[e-mail]]

(In reply to Patrick McManus from comment #14)
> patch and had to add a few nsRefPtr<foo> deleteProtector(bar) style
kungFuDeathGrip is out of fashion?

Comment 21

[Patrick McManus [:mcmanus]]

(In reply to [e-mail] from comment #20)
> (In reply to Patrick McManus from comment #14)
> > patch and had to add a few nsRefPtr<foo> deleteProtector(bar) style
> kungFuDeathGrip is out of fashion?

I find it amateurish and offensive. ymmv.

Comment 22

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

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

Comment 23

[Patrick McManus [:mcmanus]]

backed out in 973207
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4f9f58d41eac

Comment 24

[Patrick McManus [:mcmanus]]

fwiw it appears most of this patch has been applied piecemeal in the last 2 years. only connmgr and pipeline are outstanding. iirc I suspect it bounced because of something in connmgr.

Comment 25

[Patrick McManus [:mcmanus]]

The previous version of this patch (comment 22) had a problem with not updating connectionHandle::TakeConnection (the class had an ns prefix at the time).. which explains the poisoned nshttpconnection crashes it caused.

the patch has seriously bitrotted and I have updated it.

unfortunately doing has not uncovered any real problems in the current reference counting implementation other than its antiquated style.

Comment 26

[Patrick McManus [:mcmanus]]

Attachment: Convert http legacy refs to smart pointers

Comment 27

[Patrick McManus [:mcmanus]]

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

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2204c405d07c

Comment 29

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/2204c405d07c

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2204c405d07c