1031114 - Ensure that all nsIChannels release as many references as possible during OnStopRequest

Status: NEW

(Core :: Networking, defect, P3)

Description

[Jason Duell]

Per bug 1015662 comment 14 and 15, we should audit all nsIChannels (not just in /netwerk, but also things like nsJARChannel, nsJSChannel, and even JS implementations if there are any) and make sure that they all release references in OnStopRequest to at least (mListener, mListenerContext, mCallbacks, mLoadGroup (rather than just removing the channel from the loadGroup yet keeping the mLoadGroup pointer), and possibly mOwner.

Adding some sort of FreeReferences() function in nsBaseChannel and HttpBaseChannel might be a good idea here.

If anyone can think of additional things to throw in the list, chime in.

Comment 1

[Jason Duell]

bz: is it OK to null out mOwner in onStopRequest?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Not obviously.  getChannelPrincipal depends on mOwner, and I don't have very good confidence that it's never called after necko has called OnStopRequest.  Most simply, extensions can install streamlisteners via nsITraceableChannel and delay delivery of all the streamlistener callbacks to their final recipient.  :(

Comment 3

[johnsullivan.pem]

Attachment: Potential Patch

I'm still not super confident I audited all the necessary code, but I did look at every file that had all of the terms OnStopRequest, mListener, mListenerContext, and mCallbacks in it. I'm also not super confident that I know what these channels are used for.

I did create a patch that passes `./mach xpcshell-test netwerk/test` (there's a failure on the tip of master already that was unaffected by my change though), and navigating around with the browser works fine.

What do you think?

Comment 4

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8465030 [details] [diff] [review]
Potential Patch

Review of attachment 8465030 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like the right kind of thing to me. But I'm going to defer to Jason because he knows these channels better than I do :)

Push a build to try with xpcshell tests and mochitests for one or two platforms to see what happens.

::: modules/libjar/nsJARChannel.cpp
@@ +1010,2 @@
>      }
> +    mListenerContext = 0;

s/0/nullptr/

Comment 5

[Jason Duell]

Comment on attachment 8465030 [details] [diff] [review]
Potential Patch

Review of attachment 8465030 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like you forgot HttpChannelChild?

   http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#548

We don't free mLoadGroup or mCallbacks there.

Also, have you looked outside /netwerk?   The code that start this little project (nsJSProtocolHandler--see bug 1015662) still needs to be fixed, too:

  http://mxr.mozilla.org/mozilla-central/source/dom/src/jsurl/nsJSProtocolHandler.cpp#784

There may be other weird nsIChannel implementations that we don't know about.  Try grepping the tree for "public nsIChannel" and '->OnStopRequest' (for C++ code) and '.onStopRequest' (for JS) 

I'm +r-ing this patch (fix the 2 nits and you can just mark the next version +r and obsolete this one).  Let's do the rest of this bug in a separate patch so there's no need to re-review what you've done so far, which all looks good.  That said, a try run is definitely a good idea :)

::: netwerk/protocol/ftp/FTPChannelChild.cpp
@@ +484,5 @@
>      mIsPending = false;
>      AutoEventEnqueuer ensureSerialDispatch(mEventQ);
> +
> +    if (mListener) {
> +      (void)mListener->OnStopRequest(this, mListenerContext, aChannelStatus);

Might as well get rid of old K&R style "(void)": that's very old school and not used generally in Mozilla code.

@@ +491,3 @@
>      mListenerContext = nullptr;
>  
> +    mCallbacks = nullptr;

No need for blank line between Context = null and callbacks=null

Comment 6

[johnsullivan.pem]

Attachment: Bug 1021114.patch

This is my last day and I unfortunately never got back to this because a couple other bugs were placed at a higher priority, but I did make the changes requested and had trouble getting it through try. My notes on this were:

> I can't tell why the mochitest failed. I got Hit `MOZ_CRASH(DOMFile not thread-safe) at
> c:\Users\johnsullivan\Desktop\gecko-dev\content\base\src\nsDOMFile.cpp:155`. But here's also
> some `###!!! [Child][MessageChannel] Error: Channel error: cannot send/recv`.

It seems we have to be extra careful about which variables we null. Regardless, this patch is what I had at the moment. Whoever picks this up should start with this because it'll save you some time finding what areas of the code need to be touched. The last time I sent it through try it failed though, so keep that in mind.

Comment 7

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 8

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258