Audit all AsyncOpen2 implementations for leakiness

RESOLVED FIXED in Firefox 53

Status

()

Core
Networking
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: bz, Assigned: mcmanus)

Tracking

({mlk})

Trunk
mozilla54
Points:
---

Firefox Tracking Flags

(firefox52 wontfix, firefox53 fixed, firefox54 fixed)

Details

(Whiteboard: [necko-next])

Attachments

(1 attachment)

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.
Flags: needinfo?(mcmanus)
Flags: needinfo?(ckerschb)
(Assignee)

Comment 1

9 months ago
(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
Flags: needinfo?(mcmanus)
> 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....
(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?
Flags: needinfo?(ckerschb)
Christoph has a question for you :)
Flags: needinfo?(mcmanus)
Whiteboard: [necko-next]
(Assignee)

Comment 5

8 months ago
I'll go through these
Flags: needinfo?(mcmanus)
(Assignee)

Comment 6

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b08c3da1b3a2
status-firefox52: --- → affected
status-firefox54: --- → affected
Keywords: mlk
(Assignee)

Comment 7

8 months ago
Created attachment 8835103 [details] [diff] [review]
audit asyncopen2 impls for callback release on failure
Attachment #8835103 - Flags: review?(dd.mozilla)
(Assignee)

Updated

8 months ago
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
(Assignee)

Comment 8

8 months ago
I found a few more instances of the same problem, and a couple cases where even asyncOpen() wasn't releasing the callbacks.
Attachment #8835103 - Flags: review?(dd.mozilla) → review+
(Assignee)

Comment 9

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

Comment 10

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b5f610c22132
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(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!
Please request Aurora/Beta approval on this when you get a chance.
Flags: needinfo?(mcmanus)
(Assignee)

Comment 13

8 months ago
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.
Flags: needinfo?(mcmanus)
status-firefox52: affected → wontfix
(Assignee)

Comment 14

8 months ago
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]:
Attachment #8835103 - Flags: approval-mozilla-aurora?
Comment on attachment 8835103 [details] [diff] [review]
audit asyncopen2 impls for callback release on failure

Fix a leaking issue related AsyncOpen2. Aurora53+.
Attachment #8835103 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 16

8 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/dde4995469df
status-firefox53: affected → fixed
You need to log in before you can comment on or make changes to this bug.