174131 - shutdown leak: nsHttpHandler and nsHttpConnection objects

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect, P2)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

nsHttpHandler and nsHttpConnection objects have a circular ownership model
(ugh!).  The nsHttpHandler owns a reference to all connections in its arrays
mActiveConnections and mIdleConnections.  The connections all own a reference to
the nsHttpHandler (added in the connection's constructor and released in the
connection's destructor).

This means that the code, in the nsHttpHandler destructor, to call
DropConnections to release the connections never gets called because the
destructor won't get called until the connections are released.

I'll attach a patch that calls DropConnections from the XPCOM shutdown observer
as well (but leaves in the calls in the destructor, although perhaps they should
be removed and replaced with an assertion that the arrays are empty).  It also
fixes what looks to me like a logic error in nsHttpHandler::PurgeDeadConnections.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 2

[Darin Fisher]

Comment on attachment 102683 [details] [diff] [review]
patch

it was my fix for bug 143821 that introduced this cycle.  still, i would
have expected the "profile-before-change" and/or "session-logout" events
to sufficiently break this ownership cycle.


>Index: nsHttpHandler.cpp

>-        nsHttpConnection *conn = (nsHttpConnection *) mIdleConnections[i];
>+        nsHttpConnection *conn = NS_STATIC_CAST(nsHttpConnection *,
>+                mIdleConnections[i]);

does this really help?	the linebreak seems to hurt readability
more than the STATIC_CAST helps IMO :(


>             NS_RELEASE(conn);
>+            --i;
>         }

thanks for catching this!  how about removing the ++i from the
|for| statement and adding a |else ++i;| instead?  might also
be good to iterate over this list backwards to minimize the 
cost of remove, but whatever ;-)

otherwise, sr=darin

Comment 3

[Darin Fisher]

oh, and i agree.. the calls to DropConnections in ~nsHttpHandler don't make any
sense.  they should be removed and replaced with an assertion.

Comment 4

[Bradley Baetz (:bbaetz)]

Comment on attachment 102683 [details] [diff] [review]
patch

What darin said (although I don't mind with --i, personally)

r=bbaetz either way

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

I think this addresses all the comments.  However, a few more notes / comments
/ questions:

Darin's comment about iterating through the array backwards made me realize
that when going backwards, there's no need to monkey with the index, so I did
that instead.

I put a printf in nsHttpHandler::Observe, and the only notification I got in
the entire run of Mozilla was "xpcom-shutdown".  Is that bad?

Should the code I moved from the destructor to nsHttpHandler::Observe need to
hold mConnectionLock, since it's no longer in the destructor?

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, and I just changed the RemoveElement(conn) to RemoveElementAt(i), which
doesn't need to search the array (making the whole thing an O(N^2) algorithm!)
in my tree, but I'm not going to bother attaching a new patch for just that
(unless I need no further changes).

Comment 7

[Darin Fisher]

cc'ing kaie since your observation may explain the NSS shutdown problem he fixed
by moving DropConnections to the network-teardown event.

Comment 8

[Darin Fisher]

Comment on attachment 102746 [details] [diff] [review]
patch

>Index: nsHttpHandler.cpp

>+        LOG(("dropping active connections...\n"));
>+        DropConnections(mActiveConnections);
>+
>+        LOG(("dropping idle connections...\n"));
>+        DropConnections(mIdleConnections);

yup, this does need to be protected by mConnectionLock.  it is very
possible for connections to access the http handler from the socket
thread.  i can't believe i forgot about that!!

  {
      nsAutoLock lock(mConnectionLock);
      ...
      DropConnections(...);
  }

sr=darin w/ that and the RemoveElementAt change.  thx :)

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

The changes from attachment 102746 [details] [diff] [review] are:
 * locking, per comment 8 (and comment 5)
 * RemoveElementAt, per comment 6

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 102793 [details] [diff] [review]
patch

Transferring r and sr.

Comment 11

[Bradley Baetz (:bbaetz)]

Wouldn't it make more sense for DropConnections to grab the lock itsself, now
that all the callers explicitly grab the lock first? There arne't any reentrency
issues with the lock, are there?

Comment 12

[Darin Fisher]

no, i think this is better because we want to call DropConnections twice, once
for the idle connections and once for the active connections.  if we enter and
leave the lock between, we could end up missing some connections.  this is
unlikely in practice because all the pending transactions should be canceled by
this point, but it would be a hidden dependency if we didn't do it this way.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to trunk, 2002-11-06 05:01 PDT.

Comment 14

[Mike Giancola]

Will this be included in the 1.2 branch?

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No.

Comment 16

[Tom Everingham]

marking verified - confirmed patch checked in on trunk

Comment 17

[Darin Fisher]

this patch appears to have caused a shutdown crash.  see bug 179391.