1215552 - nsHttpConnectionMgr Post Event shouldn't manually ref count

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

This is inspired by bug 1213332 (and in turn 905460).

A lot of the complexity of reference management in nsHttpConnectionMgr comes from use of void * in PostEvent().. most arguments of that form are really reference counted (some of them as xpcom objects, but not all) and the caller of postevent has to add the ref (and drop it again if postevent fails), and the actual callback function needs to drop the ref.. some of the callback functions are called from different paths, adding to the complexity.

its par for the course with legacy code.

I added a new virtual base class that can be implemented by anything that can be represented by a nsRefPtr (basically an nsISupports for a xpcom and non xpcom ref counted objects), and PostEvent can use that to simplify things considerably by holding the necessary references itself.

Comment 1

[Patrick McManus [:mcmanus]]

the patch is more complicated than I would like, but I think the net effect simplifies the code.

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

some of the churn is about making dtors be private for the reference counted classes, and the opportunity to take related code out of the huge .h file and get it into the .cpp file as long as it was being updated.

Comment 2

[Patrick McManus [:mcmanus]]

Attachment: nsHttpConnectionMgr::PostEvent shouldnt manually ref count

Comment 3

[u408661]

Comment on attachment 8674925 [details] [diff] [review]
nsHttpConnectionMgr::PostEvent shouldnt manually ref count

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

Happy day, less manual refcount managemenet! Mostly nits in the comments below, but the last few are a bit beyond the nit stage (not much, though).

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +156,5 @@
> +    bool mBool;
> +
> +private:
> +    virtual ~BoolWrapper() {}
> +    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(BoolWrapper)

total nit: I'd like to see at least one blank line before this (and maybe even put it in the "public" block above), because this makes it look like AddRef/Release are going to be private (even though the macro expands to make the appropriate protections) which makes me wonder at first glance how it will ever work properly :) It's fine for those of us who are experienced with the code, but anyone who comes in new may be thoroughly confused.

@@ +197,5 @@
>  }
>  
> +class ConnEvent : public nsRunnable
> +{
> +  public:

nit: align w/{ for consistency w/in file

@@ +211,5 @@
> +        (mMgr->*mHandler)(mIParam, mVParam);
> +        return NS_OK;
> +    }
> +
> +  private:

nit: same here

@@ +398,5 @@
>      bool mAllow1918;
>  
> +private:
> +    virtual ~SpeculativeConnectArgs() {}
> +    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(SpeculativeConnectArgs)

Same nit here as with BoolWrapper

@@ +482,1 @@
>      : mConn(aConn), mUpgradeListener(aListener) {}

nit: this should also be indented a bit more now

@@ +484,5 @@
>      nsRefPtr<nsAHttpConnection> mConn;
>      nsCOMPtr<nsIHttpUpgradeListener> mUpgradeListener;
> +private:
> +    virtual ~nsCompleteUpgradeData() {}
> +    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(nsCompleteUpgradeData)

same appears-private nit here :)

@@ +1202,5 @@
>      nsHttpConnectionMgr::PipelineFeedbackInfoType mInfo;
>      uint32_t mData;
> +private:
> +    ~nsHttpPipelineFeedback() {}
> +    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(nsHttpPipelineFeedback)

same private/public nit here :)

@@ +2369,5 @@
>  {
>      MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread);
>      LOG(("nsHttpConnectionMgr::OnMsgReschedTransaction [trans=%p]\n", param));
>  
>      nsHttpTransaction *trans = (nsHttpTransaction *) param;

Change this to static_cast like you did above?

@@ +2457,3 @@
>  {
>      MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread);
>      nsHttpConnectionInfo *ci = (nsHttpConnectionInfo *) param;

static_cast?

@@ +2682,3 @@
>  {
>      MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread);
>      nsCompleteUpgradeData *data = (nsCompleteUpgradeData *) param;

static_cast?

@@ +2744,3 @@
>  {
>      MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread);
>      nsHttpPipelineFeedback *fb = (nsHttpPipelineFeedback *)param;

static_cast?

@@ +3438,5 @@
>          } else {
>              // otherwise just put this in the persistent connection pool
>              LOG(("nsHalfOpenSocket::OnOutputStreamReady no transaction match "
>                   "returning conn %p to pool\n", conn.get()));
> +            nsRefPtr<nsHttpConnection> deleteProtector(conn);

I don't think we need the deleteProtector here any more - conn is already a refptr, and OnMsgReclaimConnection doesn't NS_RELEASE(conn) any more, so the refcounts should work out fine.

Comment 4

[Patrick McManus [:mcmanus]]

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

Comment 5

[Patrick McManus [:mcmanus]]

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

Comment 6

[Patrick McManus [:mcmanus]]

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

Comment 7

[Patrick McManus [:mcmanus]]

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

Comment 8

[Patrick McManus [:mcmanus]]

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

Comment 9

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5be3b99bc4e57159f9456fdef3de0cfe0c0e805
bug 1215552 - nsHttpConnectionMgr::PostEvent shouldnt manually ref count r=hurley

Comment 10

[Carsten Book [:Tomcat]]

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