Closed Bug 1215552 Opened 4 years ago Closed 4 years ago

nsHttpConnectionMgr Post Event shouldn't manually ref count

Categories

(Core :: Networking: HTTP, defect)

32 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: mcmanus, Assigned: mcmanus)

Details

Attachments

(1 file)

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.
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 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.
Attachment #8674925 - Flags: review?(hurley) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5be3b99bc4e57159f9456fdef3de0cfe0c0e805
bug 1215552 - nsHttpConnectionMgr::PostEvent shouldnt manually ref count r=hurley
https://hg.mozilla.org/mozilla-central/rev/c5be3b99bc4e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.