Closed Bug 905460 Opened 6 years ago Closed 4 years ago

convert legacy manual http ref counts to smart pointers

Categories

(Core :: Networking: HTTP, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox48 --- fixed

People

(Reporter: mcmanus, Assigned: mcmanus)

References

Details

(Whiteboard: [necko-active])

Attachments

(2 files, 3 obsolete files)

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
Attachment #790563 - Flags: review?(sworkman)
Assignee: nobody → mcmanus
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.
Attachment #790563 - Flags: review?(sworkman) → review+
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/7d3ba33cc1ad
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
backed out in 907960
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #8362107 - Flags: review?(sworkman)
Attachment #790563 - Attachment is obsolete: true
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 :) ).
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(-)
Attachment #8374086 - Flags: review?(sworkman)
Attachment #8374086 - Attachment is obsolete: true
Attachment #8374086 - Flags: review?(sworkman)
Attachment #8362107 - Flags: review?(sworkman)
Attachment #8362107 - Attachment is obsolete: true
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 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.
Attachment #8374087 - Flags: review?(sworkman) → review+
(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.
(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.
(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?
(In reply to neil@parkwaycc.co.uk 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.
https://hg.mozilla.org/mozilla-central/rev/fc25d495f995
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: mozilla26 → mozilla30
Depends on: 973207
Depends on: 973608
backed out in 973207
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4f9f58d41eac
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [necko-active]
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.
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.
Attachment #8736363 - Flags: review?(dd.mozilla)
Attachment #8736363 - Flags: review?(dd.mozilla) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2204c405d07c
Status: REOPENED → RESOLVED
Closed: 6 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.