...from bug 82415, an extra ADDREF was put in for byte ranges requests in nsPluginStreamListenerPeer::SetUpStreamListener. The 4x plugin instance should probably keep a reference to the stream peer until it gets destroyed.

Setting milestone to mozilla0.9.2

This causes leaks of plugin instances, plugin streams and peer streams whether or not they use byte range requests. The addref is based on whether or not the http channel has an "accept-ranges" response header. What a plugin does with a stream is not part of the condition that forces the leak.

adding dougt. Sean, I don't see this huge leak. Andrei, there are lifetime issues [we leak] with Byte Range streams. The stream needs an owner but I'm not clear on what it should be because I'm clear of the lifetime of a byte range stream. Who would be a good owner?

peter, I was under the impression that there was some kind of hacky notification that the plugin gives us so that we can drop the stream... something like a request that is not segmented [0-2,1-3]. If this is true, we should just release the stream at that point... and document the heck out of this.

>Sean, I don't see this huge leak. How are you looking for it? Here's what I'm experiencing: 1. nsPluginStreamListenerPeer has an extra addref that prevents nsPluginStreamListenerPeer's from ever being deleted. 2. If nsPluginStreamListenerPeer doesn't get deleted, it's destructor is not executed. 3. nsPluginStreamListenerPeer::~nsPluginStreamListenerPeer is supposed to do: 1357 NS_IF_RELEASE(mURL); 1358 NS_IF_RELEASE(mOwner); 1359 NS_IF_RELEASE(mInstance); 1360 NS_IF_RELEASE(mPStreamListener); 1361 NS_IF_RELEASE(mHost); 1362 NS_IF_RELEASE(mPluginStreamInfo); 4. Since it is not run, all of these members leak: mURL, mOwner, mInstance, mPStreamListener, mHost, mPluginStreamInfo mInstance is a pointer to an nsIPluginInstance, mPStreamListener is a pointer to a stream generated by the plugin instance for reception of the requested url.

Sean, you are right!! We are leaking tons of stuff in EVERY case. This needs to be fixed ASAP. This latest patch should fix it for all cases but RequestRead. Andrei, can I get your r= for THIS patch until I can figure out how to fix it for all cases.

OK, r=av

verbal sr=attinasi from Marc pending changing NS_ADDREF(mPluginStreamListener); Now seeking drivers approval. Once again: this patch does not FIX this bug, but makes the leak not so widespread. I think I have an idea of how to totally fix this bug, but I'd like to check one thing in at a time.

a= asa@mozilla.org for checkin of the leak fix to the trunk. (on behalf of drivers)

a= asa@mozilla.org for checkin of the leak fix to the trunk. (on behalf of drivers)

This patch is a next generation patch for fixing the real bug, byte range stream leaking. Still needs some work plus the ADDREFF in the host needs to be removed. My thinking is this: The general case of keeping track of which instance has open which byte range streams is very difficult because at the stream creation time, I don't a way for the host to know if the peer's stream is going to be a byte range stream (unless someone can clue me in) or not. When I finnally know I have a byte range stream, I don't have easy access to the host. Anyway, it so happens in pluginviewer, it's much easier to do AND Adobe is a full-page plugin anyway.

The following patch adds a nsVoidArray member to nsActivePlugin for storing "listener peers" that are going to stay around for the entire instance. Currently, we want to do this byte range streams that happen for Acrobat. Since Acrobat is a full-page plugin, this only works for full page plugin streams. Any byte requets after the first will still be broken for streams that are started a different way (unless we add the logic to those in the host). Anyway, Doug, perhaps you can piggy back off something like that in nsActivePlugin, maybe when nsPluginHostImpl::NewTempFilename is called?

We may want to do the same thing in nsPluginHostImpl::NewEmbededPluginStream. Andrei, what do you think? Not in NewPluginURLStream because then a plugin can create many streams that won't get destroyed until the instance stops.

I think the patch is OK, good job, Peter. I would not agree with your comment about nasty cast but this is not important. If approved, this patch should probably go in _after_ Doug's patch in bug 54689. Or may be we should create a consolidated patch?

just land them both at the same time. There is only one conflict that I got which was quickly resolved.

Marking FIXED with patch from bug 54689 checked in.

verif (stamped)