shutdown leak: nsHttpHandler and nsHttpConnection objects

VERIFIED FIXED in mozilla1.3alpha



16 years ago
16 years ago


(Reporter: dbaron, Assigned: dbaron)




Firefox Tracking Flags

(Not tracked)


(Whiteboard: [patch])


(1 attachment, 2 obsolete attachments)

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.
Keywords: mlk
Priority: -- → P2
Target Milestone: --- → mozilla1.3alpha

Comment 2

16 years ago
Comment on attachment 102683 [details] [diff] [review]

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
Attachment #102683 - Flags: superreview+

Comment 3

16 years ago
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 on attachment 102683 [details] [diff] [review]

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

r=bbaetz either way
Attachment #102683 - Flags: review+
Created attachment 102746 [details] [diff] [review]

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?
Attachment #102683 - Attachment is obsolete: true
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

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

Comment 8

16 years ago
Comment on attachment 102746 [details] [diff] [review]

>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);

sr=darin w/ that and the RemoveElementAt change.  thx :)
Attachment #102746 - Flags: superreview+
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

16 years ago
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.
Fix checked in to trunk, 2002-11-06 05:01 PDT.
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 14

16 years ago
Will this be included in the 1.2 branch?

Comment 16

16 years ago
marking verified - confirmed patch checked in on trunk
QA Contact: httpqa → tever

Comment 17

16 years ago
this patch appears to have caused a shutdown crash.  see bug 179391.
You need to log in before you can comment on or make changes to this bug.