1354220 - HttpChannelChild doesn't properly null out backpointers

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=716b001284e89abcc51ecca5cd853ade9789079e&selectedJob=89262991

Comment 2

[Patrick McManus [:mcmanus]]

Attachment: Bug 1354220 - HttpChannelChild doesn't release listeners on fail case

Review commit: https://reviewboard.mozilla.org/r/127276/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127276/

Comment 3

[Patrick McManus [: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!

Comment 4

[Boris Zbarsky [: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....

Comment 5

[Patrick McManus [:mcmanus]]

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

Comment 6

[Patrick McManus [:mcmanus]]

  https://treeherder.mozilla.org/#/jobs?repo=try&evision=f89184cdc6c85245278d3ef161bdc5b4f922d94d

Comment 7

[Patrick McManus [:mcmanus]]

Comment on attachment 8855503 [details]
Bug 1354220 - HttpChannelChild doesn't release listeners on fail case

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127276/diff/1-2/

Comment 8

[Patrick McManus [:mcmanus]]

Comment on attachment 8855503 [details]
Bug 1354220 - HttpChannelChild doesn't release listeners on fail case

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127276/diff/2-3/

Comment 9

[Boris Zbarsky [:bzbarsky]]

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.

Comment 10

[Patrick McManus [:mcmanus]]

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?

Comment 11

[Boris Zbarsky [:bzbarsky]]

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 12

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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?

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Do we need to clean up callbacks and listener after HttpChannelChild::Redirect3Complete as well?

That is what bug 1341673 is all about...

Comment 14

[Patrick McManus [:mcmanus]]

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

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Redirecting to Andrew who is a lot more familiar with our necko integration at the moment.

Comment 16

[Patrick McManus [:mcmanus]]

Comment on attachment 8855503 [details]
Bug 1354220 - HttpChannelChild doesn't release listeners on fail case

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127276/diff/3-4/

Comment 17

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8855503 [details]
Bug 1354220 - HttpChannelChild doesn't release listeners on fail case

lgtm.

Comment 18

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(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.

Comment 19

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d830809047452b897a49f2d731fb049361a39cd

Comment 20

[Patrick McManus [:mcmanus]]

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

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7310b4caed29
HttpChannelChild doesn't release listeners on fail case r=schien

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7310b4caed29

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider backporting to 54 (and maybe ESR52) or should it ride the trains?

Comment 24

[Boris Zbarsky [:bzbarsky]]

We should consider backporting it, if the patch is deemed safe enough.  Leaks are bad for all sorts of user-relevant metrics.

Comment 25

[Patrick McManus [:mcmanus]]

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.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 27

[Patrick McManus [: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?

Comment 28

[Marion Daly [:mdaly]]

Clearing old need-info