Closed Bug 1354220 Opened 3 years ago Closed 3 years ago

HttpChannelChild doesn't properly null out backpointers

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: mcmanus)

References

Details

(Keywords: memory-leak, Whiteboard: [necko-active])

Attachments

(1 file)

For example, it never nulls out mCallbacks.

It should probably be calling ReleaseListeners() instead of reimplementing it.

Note that it also needs to do this in its AsyncOpen2 impl if the security check fails.  This was missed in bug 1333142.

Fixing these bits is likely necessary, but is not sufficient, to fix bug 1341673.
Flags: needinfo?(mcmanus)
Comment on attachment 8855503 [details]
Bug 1354220 - HttpChannelChild doesn't release listeners on fail case

just double checking I understood the report correctly. Thanks!
Attachment #8855503 - Flags: feedback?(bzbarsky)
Comment on attachment 8855503 [details]
Bug 1354220 - HttpChannelChild doesn't release listeners on fail case

This part is necessary, but there are other places where HttpChannelChild nulls out some, but not all, of its callback members that I think need to be fixed as well....
Attachment #8855503 - Flags: feedback?(bzbarsky) → feedback-
so ::onstop does call releaseListeners()

::doOnStopRequest() does not seem to when called through the service worker intercept stream listener case.. will update. are you seeing others? :scien is probly right reviewer.

(there are some other places that mess with only mListener + pals intentionally afaict - they aren't trying to clean up all references.)
Flags: needinfo?(schien)
The ones I saw offhand were DoOnStartRequest and DoOnStopRequest.

By the way, if you're in this stuff already, you might be interested in bug 1341673 too.
i'm not really sure that the sw code wants to null out everything when instering the listener in doOnStart (and in the cancel case that will devolve to onStop). bkelly?
Flags: needinfo?(bkelly)
As long as things get nulled out by the time (e.g. right after) OnStopRequest is called on the original listener, it's fine by me.
Comment on attachment 8855503 [details]
Bug 1354220 - HttpChannelChild doesn't release listeners on fail case

https://reviewboard.mozilla.org/r/127276/#review130154

Do we need to clean up callbacks and listener after HttpChannelChild::Redirect3Complete as well? I didn't see we clean up these back pointer but maybe it hides somewhere else.

::: netwerk/protocol/http/HttpChannelChild.cpp:1111
(Diff revision 3)
>    if (mListener) {
>      mListener->OnStopRequest(aRequest, aContext, mStatus);
>    }
>    mOnStopRequestCalled = true;
>  
> -  mListener = nullptr;
> +  ReleaseListeners();

We should remove the ReleseListeners in OnStopRequest since it is called right after DoOnStopRequest.

::: netwerk/protocol/http/HttpChannelChild.cpp:2220
(Diff revision 3)
>  HttpChannelChild::AsyncOpen2(nsIStreamListener *aListener)
>  {
>    nsCOMPtr<nsIStreamListener> listener = aListener;
>    nsresult rv = nsContentSecurityManager::doContentSecurityCheck(this, listener);
> -  NS_ENSURE_SUCCESS(rv, rv);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    ReleaseListeners();

Should we call `AsyncAbort(rv)` if failed here?
Attachment #8855503 - Flags: review?(schien) → review+
Assignee: nobody → mcmanus
Whiteboard: [necko-active]
> Do we need to clean up callbacks and listener after HttpChannelChild::Redirect3Complete as well?

That is what bug 1341673 is all about...
Comment on attachment 8855503 [details]
Bug 1354220 - HttpChannelChild doesn't release listeners on fail case

https://reviewboard.mozilla.org/r/127276/#review130154

I think you're right. so I added that.

It makes me wonder more about bz's comment in onstartrequest(). that does clean up the basic listeners but not the callbacks that ReleaseListeners() would do. I'm not entirely certain whether this channel is ever needed after being intercepted for its security callbacks or not - that's why I'm hesitant to add the full releaseListeners() call there. thoughts?

> We should remove the ReleseListeners in OnStopRequest since it is called right after DoOnStopRequest.

sure. I changed it to a comment to make it clear though.

> Should we call `AsyncAbort(rv)` if failed here?

I don't think so. async abort calls onstop/onstart and is for, well, async.. this situation is for where AsyncOpen() is returning a fail code and the listener contract (onstart/onstop) will never go into effect
Redirecting to Andrew who is a lot more familiar with our necko integration at the moment.
Flags: needinfo?(bkelly) → needinfo?(bugmail)
Attachment #8855503 - Flags: review+ → review?(schien)
Comment on attachment 8855503 [details]
Bug 1354220 - HttpChannelChild doesn't release listeners on fail case

lgtm.
Attachment #8855503 - Flags: review?(schien) → review+
(In reply to Patrick McManus [:mcmanus] from comment #14)
> Comment on attachment 8855503 [details]
> Bug 1354220 - HttpChannelChild doesn't release listeners on fail case
> 
> https://reviewboard.mozilla.org/r/127276/#review130154
> 
> I think you're right. so I added that.
> 
> It makes me wonder more about bz's comment in onstartrequest(). that does
> clean up the basic listeners but not the callbacks that ReleaseListeners()
> would do. I'm not entirely certain whether this channel is ever needed after
> being intercepted for its security callbacks or not - that's why I'm
> hesitant to add the full releaseListeners() call there. thoughts?
I don't know the right answer, to be honest. ni? jdm about our interception implementation.
> 
> > We should remove the ReleseListeners in OnStopRequest since it is called right after DoOnStopRequest.
> 
> sure. I changed it to a comment to make it clear though.
> 
> > Should we call `AsyncAbort(rv)` if failed here?
> 
> I don't think so. async abort calls onstop/onstart and is for, well, async..
> this situation is for where AsyncOpen() is returning a fail code and the
> listener contract (onstart/onstop) will never go into effect

Yeah right, should not call onstart/onstop under this circumstance.
Flags: needinfo?(josh)
I'm going to land this and if jdm or andrew think we should also do something in DoOnStart() we can do that in a followup
Pushed by mcmanus@ducksong.com:
https://hg.mozilla.org/integration/autoland/rev/7310b4caed29
HttpChannelChild doesn't release listeners on fail case r=schien
https://hg.mozilla.org/mozilla-central/rev/7310b4caed29
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Is this something we should consider backporting to 54 (and maybe ESR52) or should it ride the trains?
Flags: needinfo?(mcmanus)
Keywords: mlk
We should consider backporting it, if the patch is deemed safe enough.  Leaks are bad for all sorts of user-relevant metrics.
its relatively safe and every fix by definition has a benefit to being uplifted.

but it doesn't fix a recent feature or regression or a heinously serious problem. Personally I would ride the trains with it.
Flags: needinfo?(mcmanus)
> but it doesn't fix a recent feature or regression

It does, because this leak is e10s-specific.  So it regressed when we shipped e10s to everyone, which just happened in 52, no?

> or a heinously serious problem

Can you quantify that?  Which objects are leaked when we leak in the testcases in bug 1341673?  Are those leaks hit in the wild?  That's assuming this patch fixes those leaks, of course.
Flags: needinfo?(mcmanus)
I'm not going to argue it's not a fix and therefore doesn't make things better. That's why its fixed :)

I also really wish we had a consistent set of uplift criteria - there are a lot of opinions on how to handle this and I'm generally ok with release drivers deciding what they want. I've never understood why release-drivers do not (also) set a? and ask for risk assessments as necessary.

e10s was initially shipped in 48 (with a growing population up to 52) - the only indication of this problem is some orange in the CI. If its severely impacting the release population the evidence for that is not in this bug. That's what I was getting at. (not saying its not possible - all bugs suck - and certainly not saying they won't benefit from it.. again, it's a fix.)

If we want to backport every fix, then we should make the uplift criteria "low risk bugs and no features" and do so. I'd be ok with that (or "all leak or locking bugs, etc.. whatever" )- indeed it would be a much more deterministic system than speculating about use cases in the asbsence of hard data like crash-stats.. but I don't feel like that's the question that was asked. maybe it should be?
Flags: needinfo?(mcmanus)
Flags: needinfo?(josh)
Clearing old need-info
Flags: needinfo?(bugmail)
You need to log in before you can comment on or make changes to this bug.