Closed
Bug 588256
Opened 14 years ago
Closed 13 years ago
e10s HTTP: clean up more aggressively in DocumentChannelCleanup
Categories
(Core :: Networking: HTTP, defect)
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: jduell.mcbugs, Unassigned)
References
Details
(Whiteboard: [inbound])
Attachments
(1 file, 1 obsolete file)
11.52 KB,
patch
|
jdm
:
review+
|
Details | Diff | Splinter Review |
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.
Attachment #466877 -
Flags: review?(honzab.moz)
Reporter | ||
Comment 1•14 years ago
|
||
s/\(chrome,/(chrome nsHttpChannel/
Comment 2•14 years ago
|
||
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.
Attachment #466877 -
Flags: review?(honzab.moz) → review+
Comment 3•13 years ago
|
||
Jason, is this patch still desirable?
Reporter | ||
Comment 4•13 years ago
|
||
This just fixes bitrot and cleans up a little (there's an extra state variable we didn't need--just leverage mKeptAlive).
Attachment #466877 -
Attachment is obsolete: true
Attachment #555960 -
Flags: review?(josh)
Comment 5•13 years ago
|
||
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.
Attachment #555960 -
Flags: review?(josh) → review+
Reporter | ||
Comment 6•13 years ago
|
||
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.
Reporter | ||
Comment 7•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/8215d12a0015
Whiteboard: [inbound]
Comment 8•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/8215d12a0015
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in
before you can comment on or make changes to this bug.
Description
•