Don't retain NPNVWindowNPObject and NPNVPluginElementNPObject twice

Don't retain NPNVWindowNPObject and NPNVPluginElementNPObject twice

Both NPN_GetValue and WebPluginProxy take a reference on access, with
the latter conflating the caller's reference and its internal reference.
To be consistent with WebPluginImpl, let WebPluginProxy only manage its
internal reference and leave the plugin's reference to NPN_GetValue.
Document this in the WebPlugin interface.

Also release plugin-side proxies when WebPluginProxy is destroyed.

R=ananta
BUG=none
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=88773

[davidben, 2011-04-02 23:57:06]

This is what happens when you try to become nspluginwrapper's new upstream and
get in the habit of skimming the debug logs for problems whenever you use Flash.
:-)

The easiest way to see the reference counts climb is to watch a Hulu video under
nspluginwrapper with NPW_DEBUG=1 (they evidently request and release the window
object a lot), but you can also just write a quick test plugin. The objects get
retained twice instead of once.

Unit test may be worthwhile, but I don't know where those live; seems the
webkit/plugins/npapi/test/ ones only get used with test_shell?

[brettw, 2011-04-04 16:27:27]

I'd prefer to have John take a look at this.

[ananta, 2011-04-04 18:40:58]

On 2011/04/04 16:27:27, brettw wrote:
> I'd prefer to have John take a look at this.

I think it would be better if WebPluginProxy grabs a reference on the NPObject
when it retrieves it the first time and then returns the cached reference on
every call. This would ensure that we avoid creating a NPObjectProxy/Stub
combine for every call to NPN_GetValue made by the plugin, which is very
frequent.

-Ananta

[davidben, 2011-04-04 21:42:41]

On 2011/04/04 18:40:58, ananta wrote:
> I think it would be better if WebPluginProxy grabs a reference on the NPObject
> when it retrieves it the first time and then returns the cached reference on
> every call. This would ensure that we avoid creating a NPObjectProxy/Stub
> combine for every call to NPN_GetValue made by the plugin, which is very
> frequent.

Hrm. Isn't that what's doing? WebPluginProxy caches the NPObjectProxy in
plugin_element_ (and window_npobject_) and has a reference by virtue of creating
it. (Or I assume it does... I got referenceCounts of 2 when testing, and if I
managed to deallocate it instead, it should have crashed.) It just also took an
extra reference on access to a cached NPObject (but not when creating) and that
reference took a vacation to reference leak land. Or am I misunderstanding? I'm
not very familiar with this code.

(Oh hey, should ~WebPluginProxy be releasing those?)

[ananta, 2011-04-04 22:21:38]

On 2011/04/04 21:42:41, David Benjamin wrote:
> On 2011/04/04 18:40:58, ananta wrote:
> > I think it would be better if WebPluginProxy grabs a reference on the
NPObject
> > when it retrieves it the first time and then returns the cached reference on
> > every call. This would ensure that we avoid creating a NPObjectProxy/Stub
> > combine for every call to NPN_GetValue made by the plugin, which is very
> > frequent.
> 
> Hrm. Isn't that what's doing? WebPluginProxy caches the NPObjectProxy in
> plugin_element_ (and window_npobject_) and has a reference by virtue of
creating
> it. (Or I assume it does... I got referenceCounts of 2 when testing, and if I
> managed to deallocate it instead, it should have crashed.) It just also took
an
> extra reference on access to a cached NPObject (but not when creating) and
that
> reference took a vacation to reference leak land. Or am I misunderstanding?
I'm
> not very familiar with this code.
> 
> (Oh hey, should ~WebPluginProxy be releasing those?)

Hi David

Good point. NPObjectProxy::Create returns an NPObject with a refcount of 1. It
looks like this object should be freed in the destructor of the WebPluginProxy
object. Other than that your change looks fine to me.

-Ananta

[davidben, 2011-05-06 22:00:04]

Updated with the appropriate releases in the destructor. (Sorry that took a
while. Got a little busy with schoolwork and forgot.)

[ananta, 2011-05-06 22:06:50]

LGTM

[davidben, 2011-05-13 17:35:55]

jam: Have you had a chance to look at this? I think I need a review from someone
in content/OWNERS for webplugin_proxy.cc.

[commit-bot: I haz the power, 2011-06-11 07:18:20]

Try job failure for 6693052-9001 on linux for step net_unittests:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[commit-bot: I haz the power, 2011-06-12 00:04:13]

Change committed as 88773