PPAPI: Really force-free NPObjects on Instance destruction.

PPAPI: Really force-free NPObjects on Instance destruction.

(There still seems to be a memory leak with this patch; I may have to check our NPObject reference counting next.)

BUG=114023
TEST=


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

[brettw, 2012-02-16 23:40:45]

This seems plausible. It would take me an hour to get how this works back into
my head. So be on the lookout for regressions.

LGTM

http://codereview.chromium.org/9403039/diff/2001/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/9403039/diff/2001/chrome/test/ui/ppapi_uitest....
chrome/test/ui/ppapi_uitest.cc:409: TEST_PPAPI_IN_PROCESS_AND_RELOAD(Instance)
I think the macros are getting out of control. Do you think we'll be using this
mode on other tests? I'd say just code up the behavior you want for this test.

[dmichael (off chromium), 2012-02-17 22:45:09]

On my ride home last night I realized my "fix" wasn't quite right.

(+viettrungluu for his Flash expertise)

http://codereview.chromium.org/9403039/diff/2001/webkit/plugins/ppapi/host_va...
File webkit/plugins/ppapi/host_var_tracker.cc (left):

http://codereview.chromium.org/9403039/diff/2001/webkit/plugins/ppapi/host_va...
webkit/plugins/ppapi/host_var_tracker.cc:103: NPObjectToNPObjectVarMap
np_object_map_copy = *np_object_map;
I realized this copy really *is* necessary (now). Consider an in-process,
trusted plugin. When we force-release an object, its PPP_Class_Deallocate
function might force-release another object and thus delete it from our map
while we're iterating.

I fixed this case, added a test, and improved the existing comment.

http://codereview.chromium.org/9403039/diff/2001/webkit/plugins/ppapi/host_va...
webkit/plugins/ppapi/host_var_tracker.cc:113: DCHECK(np_object_map->empty());
Note that as-is, PPP_Class_Deallocate could create a new object for our instance
and thus leave one or more objects in the map (violating this assertion). To
protect against that, we could disallow creating any more objects for this
instance while we're in the process of cleaning out the instance.

Should I be that paranoid, or should we assume (since we control the trusted
plugins) that we just won't do that?

[brettw, 2012-02-17 23:08:23]

On Fri, Feb 17, 2012 at 2:45 PM,  <dmichael@chromium.org> wrote:
> On my ride home last night I realized my "fix" wasn't quite right.
>
> (+viettrungluu for his Flash expertise)
>
>
>
http://codereview.chromium.org/9403039/diff/2001/webkit/plugins/ppapi/host_va...
> File webkit/plugins/ppapi/host_var_tracker.cc (left):
>
>
http://codereview.chromium.org/9403039/diff/2001/webkit/plugins/ppapi/host_va...
> webkit/plugins/ppapi/host_var_tracker.cc:103: NPObjectToNPObjectVarMap
> np_object_map_copy = *np_object_map;
> I realized this copy really *is* necessary (now). Consider an
> in-process, trusted plugin. When we force-release an object, its
> PPP_Class_Deallocate function might force-release another object and
> thus delete it from our map while we're iterating.
>
> I fixed this case, added a test, and improved the existing comment.
>
>
http://codereview.chromium.org/9403039/diff/2001/webkit/plugins/ppapi/host_va...
> webkit/plugins/ppapi/host_var_tracker.cc:113:
> DCHECK(np_object_map->empty());
> Note that as-is, PPP_Class_Deallocate could create a new object for our
> instance and thus leave one or more objects in the map (violating this
> assertion). To protect against that, we could disallow creating any more
> objects for this instance while we're in the process of cleaning out the
> instance.
>
> Should I be that paranoid, or should we assume (since we control the
> trusted plugins) that we just won't do that?

I think we should be paranoid, some of this stuff might be
controllable by script on the page or in Flash.

Brett

[viettrungluu, 2012-02-17 23:31:29]

+piman, who knows the subtleties of Flash and scripting better than me

[dmichael (off chromium), 2012-02-24 20:29:50]

Okay, should be ready for review now. This patch:
- Decrements the reference count of the instance object so that it will get
destroyed. If the plugin holds no references to its instance object, it gets
destroyed just before the instance.
- force-frees NPObjects that the plugin leaks, but does not destroy the
associated plugin-side objects.
- Adds testing for the above.
...for in-process plugins. I'm hopeful that out-of-process is doing the right
thing but failing the new tests just due to asynchrony. I'll have to look in to
that later.

Sorry it's grown a bit complex. It's a little difficult to test this behavior.

[brettw, 2012-02-28 00:18:26]

lgtm

http://codereview.chromium.org/9403039/diff/14001/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_webplugin_impl.cc (right):

http://codereview.chromium.org/9403039/diff/14001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_webplugin_impl.cc:96:
::ppapi::PpapiGlobals::Get()->GetVarTracker()->ReleaseVar(instance_object_);
Can you also clear the instance_object_ var here?

[commit-bot: I haz the power, 2012-02-28 21:19:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/9403039/24001

[commit-bot: I haz the power, 2012-02-28 22:09:20]

Try job failure for 9403039-24001 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-02-28 22:23:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/9403039/25007

[commit-bot: I haz the power, 2012-02-29 03:48:26]

Change committed as 124106