1333142 - Audit all AsyncOpen2 implementations for leakiness

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Boris Zbarsky [:bzbarsky]]

When AsyncOpen2 got implemented, it seems to have introduces various leak footguns akin to the one that got tickled in bug 1333106.  For example, in addition to the HTTP bits I'm fixing in bug 1333106, nsBaseChannel::AsyncOpen nulls out mListener on failure codepaths, but nsBaseChannel::AsyncOpen2 does not.

I'm fixing the HTTP bits, but we should probably audit all other AsyncOpen2 implementations and ensure that they do the right thing.  

It doesn't help too much that it's not clear what the right thing _is_.  If the right thing is that callers should drop the channel ref as soon as AsyncOpen2 fails, then we need to audit the callers instead, of course.

Comment 1

[Patrick McManus [:mcmanus]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #0)
> When AsyncOpen2 got implemented, it seems to have introduces various leak
> footguns akin to the one that got tickled in bug 1333106.  For example, in
> addition to the HTTP bits I'm fixing in bug 1333106,
> nsBaseChannel::AsyncOpen nulls out mListener on failure codepaths, but
> nsBaseChannel::AsyncOpen2 does not.
> 
seems like it should. christoph?

> I'm fixing the HTTP bits, but we should probably audit all other AsyncOpen2
> implementations and ensure that they do the right thing.  
> 
> It doesn't help too much that it's not clear what the right thing _is_.  If
> the right thing is that callers should drop the channel ref as soon as
> AsyncOpen2 fails, then we need to audit the callers instead, of course.

they shouldn't have to do that - though it seems like a good idea

Comment 2

[Boris Zbarsky [:bzbarsky]]

> they shouldn't have to do that

Then we really need to audit all of our channels' AsyncOpen implementations carefully too.  I bet at least some forget to drop references on failure in some cases....

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Patrick McManus [:mcmanus] from comment #1)
> (In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #0)
> > When AsyncOpen2 got implemented, it seems to have introduces various leak
> > footguns akin to the one that got tickled in bug 1333106.  For example, in
> > addition to the HTTP bits I'm fixing in bug 1333106,
> > nsBaseChannel::AsyncOpen nulls out mListener on failure codepaths, but
> > nsBaseChannel::AsyncOpen2 does not.
> > 
> seems like it should. christoph?

Yes, code around AsyncOpen2() should behave exactly the same like asyncOpen() does/did. In that particular case AsyncOpen2() on failure should also result in nulling out mListener. 

> > I'm fixing the HTTP bits, but we should probably audit all other AsyncOpen2
> > implementations and ensure that they do the right thing.  

Pat, any chance I could convince Necko folks to look into auditing all of those callsites?

Comment 4

[Dragana Damjanovic [:dragana]]

Christoph has a question for you :)

Comment 5

[Patrick McManus [:mcmanus]]

I'll go through these

Comment 6

[Patrick McManus [:mcmanus]]

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

Comment 7

[Patrick McManus [:mcmanus]]

Attachment: audit asyncopen2 impls for callback release on failure

Comment 8

[Patrick McManus [:mcmanus]]

I found a few more instances of the same problem, and a couple cases where even asyncOpen() wasn't releasing the callbacks.

Comment 9

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5f610c22132f1af5c816c70fe7036986c9b23c1
Bug 1333142 - audit asyncopen2 impls for callback release on failure r=dragana

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b5f610c22132

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Patrick McManus [:mcmanus] from comment #8)
> I found a few more instances of the same problem, and a couple cases where
> even asyncOpen() wasn't releasing the callbacks.

Thanks a lot for auditing and fixing all of these!

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta approval on this when you get a chance.

Comment 13

[Patrick McManus [:mcmanus]]

I'm thinking just aurora on this one. These potential leaks are only negative test cases and now that the basic http channels are covered separately, are unlikely to be invoked at high volume. And on the risk side the patch is fairly broad - so we can let it bake for a cycle.

Comment 14

[Patrick McManus [:mcmanus]]

Comment on attachment 8835103 [details] [diff] [review]
audit asyncopen2 impls for callback release on failure

Approval Request Comment
[Feature/Bug causing the regression]: AsyncOpen2 Security Checks
[User impact if declined]: security error path leaks in icon, jar, wyciwyg, and addon created channels.
[Is this code covered by automated tests?]: 50/50
[Has the fix been verified in Nightly?]: in nightly
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:none
[Is the change risky?]: touches a fairly broad range of code, and correct related legacy leaks older than the asyncopen2 checks
[Why is the change risky/not risky?]:see above and comment 13
[String changes made/needed]:

Comment 15

[Gerry Chang [:gchang]]

Comment on attachment 8835103 [details] [diff] [review]
audit asyncopen2 impls for callback release on failure

Fix a leaking issue related AsyncOpen2. Aurora53+.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dde4995469df