Last Comment Bug 302737 - [FIX]Plugins leak ns4xPluginStreamListeners and whatever they entrain
: [FIX]Plugins leak ns4xPluginStreamListeners and whatever they entrain
Status: RESOLVED FIXED
: fixed1.8.0.7, fixed1.8.1, mlk
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: mozilla1.9alpha1
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
:
Mentors:
http://www.espn.com/
Depends on: 354124
Blocks: mlk1.8
  Show dependency treegraph
 
Reported: 2005-07-29 21:20 PDT by Boris Zbarsky [:bz] (Out June 25-July 6)
Modified: 2006-09-27 18:46 PDT (History)
8 users (show)
mbeltzner: blocking1.8.1+
dveditz: blocking1.8.0.7+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Like this, perhaps? (2.89 KB, patch)
2005-07-29 21:25 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
jst: review+
jst: superreview+
dveditz: approval1.8.0.7+
mtschrep: approval1.8.1+
Details | Diff | Review

Description Boris Zbarsky [:bz] (Out June 25-July 6) 2005-07-29 21:20:03 PDT
We end up leaking listeners that are allocated when a plugin calls NPN_GetURL,
with the following stack to the unbalanced addref:

NPN_GetURL (/home/bzbarsky/plugins/libflashplayer.so)
_geturl (../../../../../mozilla/modules/plugin/base/src/ns4xPlugin.cpp:964)
MakeNew4xStreamInternal(_NPP*, char const*, char const*, eNPPStreamTypeInternal,
int, void*, unsigned int, char const*, unsigned char)
(../../../../../mozilla/modules/plugin/base/src/ns4xPlugin.cpp:928)
ns4xPluginInstance::NewNotifyStream(nsIPluginStreamListener**, void*, int, char
const*) (../../../../../mozilla/modules/plugin/base/src/ns4xPluginInstance.cpp:1357)

And indeed, NewNotifyStream addrefs its our param and the caller never releases
it here.  Fixing that makes us crash, since
ns4xPluginStreamListener::CallURLNotify helpfully calls NS_RELEASE_THIS() with
the comment:

235   // Let's not leak this stream listener. Release the reference to the
stream listener 
236   // added for the notify callback in NewNotifyStream. 

Problem is, CallURLNotify has several early returns (in non-failure cases,
even).  So do the methods that call CallURLNotify.  The net result is that it's
pretty easy for us to leak this whole mess.

What I think should happen is that this NS_RELEASE_THIS() should be removed, and
that MakeNew4xStreamInternal should be just release its ptr.  To make sure this
is safe, though, have to trace through the ownership model between this class
and ns4xPluginInstance...
Comment 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-07-29 21:25:14 PDT
Created attachment 191025 [details] [diff] [review]
Like this, perhaps?

I'm guessing this is 1.9-type stuff at this point.... :(
Comment 2 Johnny Stenback (:jst, jst@mozilla.com) 2005-09-06 15:39:08 PDT
Comment on attachment 191025 [details] [diff] [review]
Like this, perhaps?

I'm not going to claim I completely understand the ownership model for the
objects in question here, but from looking at this change and the surrounding
code it *seems* like this is indeed what we want here.

r=jst to give this some mileage on the trunk...
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-09-14 08:20:04 PDT
Comment on attachment 191025 [details] [diff] [review]
Like this, perhaps?

jst, want to just make this an r+sr?
Comment 4 Johnny Stenback (:jst, jst@mozilla.com) 2005-09-14 09:11:07 PDT
Comment on attachment 191025 [details] [diff] [review]
Like this, perhaps?

Sure, sr=jst too.
Comment 5 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-09-14 11:22:08 PDT
Fixed on trunk.
Comment 6 Daniel Veditz [:dveditz] 2005-12-21 00:23:19 PST
Is this leak worth fixing for 1.8.0.1 or 1.8.1?
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-12-23 10:13:07 PST
No idea.  More importantly, I can't give a risk evaluation here; I have no idea what this patch could break.
Comment 8 Mike Schroepfer 2006-06-26 11:44:08 PDT
JST - thoughts on a branch patch for this?
Comment 9 Johnny Stenback (:jst, jst@mozilla.com) 2006-07-07 12:32:42 PDT
Seems worth doing. It's been on the trunk long enough IMO, and this patch applies cleanly (though with some offset) to the branch as well so IMO we could just land this.
Comment 10 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-07-19 17:08:53 PDT
jst, want to land it now that it's approved?
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-07-19 20:19:18 PDT
Fixed on branch.  Hadn't realized this got approved...
Comment 12 Daniel Veditz [:dveditz] 2006-08-09 15:26:59 PDT
Comment on attachment 191025 [details] [diff] [review]
Like this, perhaps?

approved for 1.8.0 branch, a=dveditz for drivers
Comment 13 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-08-12 10:02:55 PDT
Fixed for 1.8.0.7
Comment 14 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-09-25 20:38:21 PDT
So this caused regression bug 354124...  No idea why.  :(
Comment 15 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-09-25 22:17:35 PDT
If my analysis in bug 354124 is correct, we probably need to back this out of branches....  and add more plug-in tests to whatever pre-release test suites we run, or not touch plug-in code on branches anymore.

I also feel that this bug is just another indication that trunk is really not getting enough testing in the various modes we support to base decisions on "there haven't been any problems on trunk".

Note You need to log in before you can comment on or make changes to this bug.