Last Comment Bug 588256 - e10s HTTP: clean up more aggressively in DocumentChannelCleanup
: e10s HTTP: clean up more aggressively in DocumentChannelCleanup
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: mozilla9
Assigned To: Nobody; OK to take it and work on it
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on: 588200 690425
Blocks: 558617
  Show dependency treegraph
 
Reported: 2010-08-17 18:49 PDT by Jason Duell [:jduell] (needinfo me)
Modified: 2011-09-29 10:54 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
release nsHttpChannel and HttpChannelParentListener after child::OnStopRequest (16.92 KB, patch)
2010-08-17 18:49 PDT, Jason Duell [:jduell] (needinfo me)
honzab.moz: review+
Details | Diff | Splinter Review
v2. Fix bitrot, cleaned up a little (11.52 KB, patch)
2011-08-25 22:29 PDT, Jason Duell [:jduell] (needinfo me)
josh: review+
Details | Diff | Splinter Review

Description Jason Duell [:jduell] (needinfo me) 2010-08-17 18:49:01 PDT
Created attachment 466877 [details] [diff] [review]
release nsHttpChannel and HttpChannelParentListener after child::OnStopRequest

Followup to 588200. 

For document-loads, release nsHttpChannel and HttpChannelParentListener after child::OnStopRequest called.

So it turns out that nsDocuments live for a while because of the history cache, and they hold onto their HttpChannelChild, so we're keeping PHttpChannel and friends around for quite a while (even after user has gone to a different page).  This patch just optimizes memory usage a bit by deleting everything (chrome, parent listener class) in chrome but the securityInfo, which is all we need to keep after OnStop.
Comment 1 Jason Duell [:jduell] (needinfo me) 2010-08-17 18:50:02 PDT
s/\(chrome,/(chrome nsHttpChannel/
Comment 2 Honza Bambas (:mayhemer) 2010-08-18 11:23:52 PDT
Comment on attachment 466877 [details] [diff] [review]
release nsHttpChannel and HttpChannelParentListener after child::OnStopRequest

> HttpChannelParent::OnStopRequest(nsIRequest *aRequest, 

>+  // Not required to break refcount cycle, but speeds up
>+  // HttpChannelParentListener cleanup.
>+  mChannelListener = 0;

We already release it in OnStartRequest, and we actually could do that in HttpChannelParent::RecvRedirect2Result (or newly Verify), up to you.

>+  nsCOMPtr<nsIAssociatedContentSecurity>  mSecurityInfo;

This is not security info, this is associated content security, please rename the member to mAssociatedContentSecurity.  The best would be to have nsCOMPtr<nsISupports> mSecurityInfo and QI where needed, but let's leave the logic as is for now as we need just the associated content security.

r=honzab with those fixed.
Comment 3 Josh Matthews [:jdm] (on vacation until Dec 5) 2011-08-02 11:01:00 PDT
Jason, is this patch still desirable?
Comment 4 Jason Duell [:jduell] (needinfo me) 2011-08-25 22:29:14 PDT
Created attachment 555960 [details] [diff] [review]
v2. Fix bitrot, cleaned up a little

This just fixes bitrot and cleans up a little (there's an extra state variable we didn't need--just leverage mKeptAlive).
Comment 5 Josh Matthews [:jdm] (on vacation until Dec 5) 2011-08-25 23:01:23 PDT
Comment on attachment 555960 [details] [diff] [review]
v2. Fix bitrot, cleaned up a little

My only beefs with this patch are cosmetic.

>+  // Normally we Send_delete in OnStopRequest: only do it with refcounting when
>+  // we need to hold onto IPDL channel for security info (in that case, IPDL
>+  // itself hold 1 reference, so we Send_delete when refCnt==1).

Normally we Send_delete in OnStopRequest, but when we need to retain the remote channel for security info IPDL itself holds 1 reference, so we Send_delete when refCnt==1.

>+    // IPDL channel still open, but only for updating security info.  Remote
>+    // necko channel will be deleted after this.

s/after this/when all references are dropped/ or s/after this/later/

>+#include "nsIAssociatedContentSecurity.h"

You should just be able to forward-declare nsIAssociatedContentSecurity and keep this header inclusion in the cpp file.
Comment 6 Jason Duell [:jduell] (needinfo me) 2011-08-26 12:19:36 PDT
Thanks--took all your comments, except I'm just using

   // IPDL channel still open, but only for updating security info.

i.e omit mention of when remote channel deleted.

Off to tryserver.
Comment 7 Jason Duell [:jduell] (needinfo me) 2011-09-02 13:05:56 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/8215d12a0015
Comment 8 Marco Bonardo [::mak] 2011-09-03 03:01:02 PDT
http://hg.mozilla.org/mozilla-central/rev/8215d12a0015

Note You need to log in before you can comment on or make changes to this bug.