Fix in process PPAPI decryption.

Fix in process PPAPI decryption.

TEST=EME content decryption via ExternalClearKey works in-process.


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=172211

[Tom Finegan, 2012-12-06 18:06:29]

PTAL, it's just a single line. I need to add in some logging to make sure I
haven't caused resource leaks in out of process. I'm not sure why that even
works given what's going on here: We create a resource with a ref count of 1 and
pass it through the CDM API, and once the plugin releases that ref it should be
getting destroyed in all modes. Being broken only in-process seems like magic to
me.

[Tom Finegan, 2012-12-06 18:15:38]

On 2012/12/06 18:06:29, Tom Finegan wrote:
> PTAL, it's just a single line. I need to add in some logging to make sure I
> haven't caused resource leaks in out of process. I'm not sure why that even
> works given what's going on here: We create a resource with a ref count of 1
and
> pass it through the CDM API, and once the plugin releases that ref it should
be
> getting destroyed in all modes. Being broken only in-process seems like magic
to
> me.

Live object count remains stable throughout runs in out of process mode. I think
the change is safe.

[xhwang, 2012-12-06 18:17:22]

+dmichael for insights.

https://codereview.chromium.org/11477002/diff/1/webkit/plugins/ppapi/content_...
File webkit/plugins/ppapi/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/11477002/diff/1/webkit/plugins/ppapi/content_...
webkit/plugins/ppapi/content_decryptor_delegate.cc:1026: *resource =
media_resource;
Shouldn't this line add the reference too? +dmichael for insights.

[Tom Finegan, 2012-12-06 18:32:59]

https://codereview.chromium.org/11477002/diff/1/webkit/plugins/ppapi/content_...
File webkit/plugins/ppapi/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/11477002/diff/1/webkit/plugins/ppapi/content_...
webkit/plugins/ppapi/content_decryptor_delegate.cc:1026: *resource =
media_resource;
On 2012/12/06 18:17:23, xhwang wrote:
> Shouldn't this line add the reference too? +dmichael for insights.

Ref counted objects bake my noodle. :(

It does, but we still need one more reference.

1. We create the resource, refcnt == 1
2. ScopedPPResource::operator= assignment above, refcnt == 2
... some time later, but not necessarily in this order ...
3. Caller of MakeMediaBufferResource exits, refcnt == 1
4. PPAPI glue finishes with the resource, refcnt == 0, resource destroyed.

[Tom Finegan, 2012-12-06 18:33:32]

On 2012/12/06 18:32:59, Tom Finegan wrote:
>
https://codereview.chromium.org/11477002/diff/1/webkit/plugins/ppapi/content_...
> File webkit/plugins/ppapi/content_decryptor_delegate.cc (right):
> 
>
https://codereview.chromium.org/11477002/diff/1/webkit/plugins/ppapi/content_...
> webkit/plugins/ppapi/content_decryptor_delegate.cc:1026: *resource =
> media_resource;
> On 2012/12/06 18:17:23, xhwang wrote:
> > Shouldn't this line add the reference too? +dmichael for insights.
> 
> Ref counted objects bake my noodle. :(
> 
> It does, but we still need one more reference.
> 
> 1. We create the resource, refcnt == 1
> 2. ScopedPPResource::operator= assignment above, refcnt == 2
> ... some time later, but not necessarily in this order ...
> 3. Caller of MakeMediaBufferResource exits, refcnt == 1
> 4. PPAPI glue finishes with the resource, refcnt == 0, resource destroyed.

In case it needs mentioning: the above is the behavior without this patch.

[xhwang, 2012-12-06 18:47:34]

On 2012/12/06 18:33:32, Tom Finegan wrote:
> On 2012/12/06 18:32:59, Tom Finegan wrote:
> >
>
https://codereview.chromium.org/11477002/diff/1/webkit/plugins/ppapi/content_...
> > File webkit/plugins/ppapi/content_decryptor_delegate.cc (right):
> > 
> >
>
https://codereview.chromium.org/11477002/diff/1/webkit/plugins/ppapi/content_...
> > webkit/plugins/ppapi/content_decryptor_delegate.cc:1026: *resource =
> > media_resource;
> > On 2012/12/06 18:17:23, xhwang wrote:
> > > Shouldn't this line add the reference too? +dmichael for insights.
> > 
> > Ref counted objects bake my noodle. :(
> > 
> > It does, but we still need one more reference.
> > 
> > 1. We create the resource, refcnt == 1
> > 2. ScopedPPResource::operator= assignment above, refcnt == 2
> > ... some time later, but not necessarily in this order ...
> > 3. Caller of MakeMediaBufferResource exits, refcnt == 1
> > 4. PPAPI glue finishes with the resource, refcnt == 0, resource destroyed.

Great. I guess I never really understood step 4. My question is: then how did
OOP work with this sequence?

> 
> In case it needs mentioning: the above is the behavior without this patch.

[Tom Finegan, 2012-12-06 19:04:36]

On 2012/12/06 18:47:34, xhwang wrote:
> On 2012/12/06 18:33:32, Tom Finegan wrote:
> > On 2012/12/06 18:32:59, Tom Finegan wrote:
> > >
> >
>
https://codereview.chromium.org/11477002/diff/1/webkit/plugins/ppapi/content_...
> > > File webkit/plugins/ppapi/content_decryptor_delegate.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/11477002/diff/1/webkit/plugins/ppapi/content_...
> > > webkit/plugins/ppapi/content_decryptor_delegate.cc:1026: *resource =
> > > media_resource;
> > > On 2012/12/06 18:17:23, xhwang wrote:
> > > > Shouldn't this line add the reference too? +dmichael for insights.
> > > 
> > > Ref counted objects bake my noodle. :(
> > > 
> > > It does, but we still need one more reference.
> > > 
> > > 1. We create the resource, refcnt == 1
> > > 2. ScopedPPResource::operator= assignment above, refcnt == 2
> > > ... some time later, but not necessarily in this order ...
> > > 3. Caller of MakeMediaBufferResource exits, refcnt == 1
> > > 4. PPAPI glue finishes with the resource, refcnt == 0, resource destroyed.
> 
> Great. I guess I never really understood step 4. My question is: then how did
> OOP work with this sequence?
> 

Luck... :)

A guess:

OOP was working because the PPAPI glue either always had 1 ref due to a buffer
being in transit to the plugin, or the local ScopedPPResource passed to
MakeMediaBufferResource() was still in scope at the call site. In other words,
and possibly an over simplification: steps 3 and 4 never completed before a
subsequent D&D attempt using the same resource reached step 2.


> > 
> > In case it needs mentioning: the above is the behavior without this patch.

[dmichael (off chromium), 2012-12-06 19:30:09]

https://codereview.chromium.org/11477002/diff/1/webkit/plugins/ppapi/content_...
File webkit/plugins/ppapi/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/11477002/diff/1/webkit/plugins/ppapi/content_...
webkit/plugins/ppapi/content_decryptor_delegate.cc:1009:
PpapiGlobals::Get()->GetResourceTracker()->AddRefResource(media_resource);
If you need an extra reference, then don't use the PassRef() constructor above.
Add a comment about how you need to pass a reference to the plugin, or
something. (See comment below)

https://codereview.chromium.org/11477002/diff/1/webkit/plugins/ppapi/content_...
webkit/plugins/ppapi/content_decryptor_delegate.cc:1026: *resource =
media_resource;
On 2012/12/06 18:32:59, Tom Finegan wrote:
> On 2012/12/06 18:17:23, xhwang wrote:
> > Shouldn't this line add the reference too? +dmichael for insights.
> 
> Ref counted objects bake my noodle. :(
> 
> It does, but we still need one more reference.
> 
> 1. We create the resource, refcnt == 1
> 2. ScopedPPResource::operator= assignment above, refcnt == 2
> ... some time later, but not necessarily in this order ...
> 3. Caller of MakeMediaBufferResource exits, refcnt == 1
> 4. PPAPI glue finishes with the resource, refcnt == 0, resource destroyed.

I guess I don't know what semantics you want. You have two options, generally:
1) You pass a reference to the plugin (e.g., when calling Decrypt).
2) You *don't* pass a reference to the plugin, and the plugin is required to
AddRefResource the PP_Resource if it wants it to live past the scope of the
call.

Looking at ppapi/cpp/content_decryptor_private.cc, you use the PP_Resource
PassRef constructor, so I think you're trying to do option 1. The proxy code on
the plugin side actually does pass a reference to the plugin (see
ppp_content_decryptor_private_proxy.cc, which calls AddProxyResource, whichs
gives a resource with ref-count of 1, which you never release). So the plugin
side of the proxy seems to fit with doing option 1.

In the host side of the proxy, it actually looks like you aren't accounting for
the extra ref-count that this CL adds. I'm wondering if your ref-counting will
be wrong on the host side with this patch?

I think probably you'll want to also tweak
ppp_content_decryptor_private_proxy.cc:Decrypt to use a ScopedPPResource to make
sure you release this ref count you're adding.

[Tom Finegan, 2012-12-06 22:59:02]

dmichael, PTAL:

You're correct as far as out of control ref counts is concerned. The initial fix
was causing huge ref counts in OOP (in process was fine). This seems to work
both ways.

https://codereview.chromium.org/11477002/diff/1/webkit/plugins/ppapi/content_...
File webkit/plugins/ppapi/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/11477002/diff/1/webkit/plugins/ppapi/content_...
webkit/plugins/ppapi/content_decryptor_delegate.cc:1009:
PpapiGlobals::Get()->GetResourceTracker()->AddRefResource(media_resource);
On 2012/12/06 19:30:09, dmichael wrote:
> If you need an extra reference, then don't use the PassRef() constructor
above.
> Add a comment about how you need to pass a reference to the plugin, or
> something. (See comment below)

I'm trying to make an optimization work: The decryptor delegate now reuses
PPB_Buffer_Dev resources, so removing PassRef() here won't help. This only
happens when the resource is created, and we always need an extra ref.

But, I think you point out the underlying cause of this problem in the next
comment...

https://codereview.chromium.org/11477002/diff/1/webkit/plugins/ppapi/content_...
webkit/plugins/ppapi/content_decryptor_delegate.cc:1026: *resource =
media_resource;
On 2012/12/06 19:30:09, dmichael wrote:
> On 2012/12/06 18:32:59, Tom Finegan wrote:
> > On 2012/12/06 18:17:23, xhwang wrote:
> > > Shouldn't this line add the reference too? +dmichael for insights.
> > 
> > Ref counted objects bake my noodle. :(
> > 
> > It does, but we still need one more reference.
> > 
> > 1. We create the resource, refcnt == 1
> > 2. ScopedPPResource::operator= assignment above, refcnt == 2
> > ... some time later, but not necessarily in this order ...
> > 3. Caller of MakeMediaBufferResource exits, refcnt == 1
> > 4. PPAPI glue finishes with the resource, refcnt == 0, resource destroyed.
> 
> I guess I don't know what semantics you want. You have two options, generally:
> 1) You pass a reference to the plugin (e.g., when calling Decrypt).
> 2) You *don't* pass a reference to the plugin, and the plugin is required to
> AddRefResource the PP_Resource if it wants it to live past the scope of the
> call.
> 
> Looking at ppapi/cpp/content_decryptor_private.cc, you use the PP_Resource
> PassRef constructor, so I think you're trying to do option 1. The proxy code
on
> the plugin side actually does pass a reference to the plugin (see
> ppp_content_decryptor_private_proxy.cc, which calls AddProxyResource, whichs
> gives a resource with ref-count of 1, which you never release). So the plugin
> side of the proxy seems to fit with doing option 1.
> 
> In the host side of the proxy, it actually looks like you aren't accounting
for
> the extra ref-count that this CL adds. I'm wondering if your ref-counting will
> be wrong on the host side with this patch?
> 
> I think probably you'll want to also tweak
> ppp_content_decryptor_private_proxy.cc:Decrypt to use a ScopedPPResource to
make
> sure you release this ref count you're adding.
> 

We want 2, but it's the browser (ContentDecryptorDelegate) that wants the buffer
to live past the scope of the call in this case. Passing the reference to the
plugin is the problem; it needs to remain owned by the scoper member in the
delegate.

Does the new patch look better?

[dmichael (off chromium), 2012-12-07 22:53:35]

lgtm

https://codereview.chromium.org/11477002/diff/1/webkit/plugins/ppapi/content_...
File webkit/plugins/ppapi/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/11477002/diff/1/webkit/plugins/ppapi/content_...
webkit/plugins/ppapi/content_decryptor_delegate.cc:1026: *resource =
media_resource;
On 2012/12/06 22:59:02, Tom Finegan wrote:
> On 2012/12/06 19:30:09, dmichael wrote:
> > On 2012/12/06 18:32:59, Tom Finegan wrote:
> > > On 2012/12/06 18:17:23, xhwang wrote:
> > > > Shouldn't this line add the reference too? +dmichael for insights.
> > > 
> > > Ref counted objects bake my noodle. :(
> > > 
> > > It does, but we still need one more reference.
> > > 
> > > 1. We create the resource, refcnt == 1
> > > 2. ScopedPPResource::operator= assignment above, refcnt == 2
> > > ... some time later, but not necessarily in this order ...
> > > 3. Caller of MakeMediaBufferResource exits, refcnt == 1
> > > 4. PPAPI glue finishes with the resource, refcnt == 0, resource destroyed.
> > 
> > I guess I don't know what semantics you want. You have two options,
generally:
> > 1) You pass a reference to the plugin (e.g., when calling Decrypt).
> > 2) You *don't* pass a reference to the plugin, and the plugin is required to
> > AddRefResource the PP_Resource if it wants it to live past the scope of the
> > call.
> > 
> > Looking at ppapi/cpp/content_decryptor_private.cc, you use the PP_Resource
> > PassRef constructor, so I think you're trying to do option 1. The proxy code
> on
> > the plugin side actually does pass a reference to the plugin (see
> > ppp_content_decryptor_private_proxy.cc, which calls AddProxyResource, whichs
> > gives a resource with ref-count of 1, which you never release). So the
plugin
> > side of the proxy seems to fit with doing option 1.
> > 
> > In the host side of the proxy, it actually looks like you aren't accounting
> for
> > the extra ref-count that this CL adds. I'm wondering if your ref-counting
will
> > be wrong on the host side with this patch?
> > 
> > I think probably you'll want to also tweak
> > ppp_content_decryptor_private_proxy.cc:Decrypt to use a ScopedPPResource to
> make
> > sure you release this ref count you're adding.
> > 
> 
> We want 2, but it's the browser (ContentDecryptorDelegate) that wants the
buffer
> to live past the scope of the call in this case. Passing the reference to the
> plugin is the problem; it needs to remain owned by the scoper member in the
> delegate.
Well, it doesn't really matter whether you pass a ref to the plugin or not. It
can always take a reference if it wants to. So make sure you account for that.
E.g., if you're assuming the plugin can't hold on to the buffer past the scope
of the call when you decide whether you can reuse the buffer, your buffer reuse
stuff is wrong (though it might not show in practice, if plugins never try to
keep the buffer).

> 
> Does the new patch look better?
Yes, this looks self-consistent. You should probably think about your buffer
reuse stuff and make sure it's right, though.

Also, I would suggest adding some documentation about the reference counting
model to the C API (i.e., point out that the plugin must take a reference to the
buffer if it wants to keep it).

[Tom Finegan, 2012-12-08 07:14:24]

On 2012/12/07 22:53:35, dmichael wrote:
> lgtm
> 
>
https://codereview.chromium.org/11477002/diff/1/webkit/plugins/ppapi/content_...
> File webkit/plugins/ppapi/content_decryptor_delegate.cc (right):
> 
>
https://codereview.chromium.org/11477002/diff/1/webkit/plugins/ppapi/content_...
> webkit/plugins/ppapi/content_decryptor_delegate.cc:1026: *resource =
> media_resource;
> On 2012/12/06 22:59:02, Tom Finegan wrote:
> > On 2012/12/06 19:30:09, dmichael wrote:
> > > On 2012/12/06 18:32:59, Tom Finegan wrote:
> > > > On 2012/12/06 18:17:23, xhwang wrote:
> > > > > Shouldn't this line add the reference too? +dmichael for insights.
> > > > 
> > > > Ref counted objects bake my noodle. :(
> > > > 
> > > > It does, but we still need one more reference.
> > > > 
> > > > 1. We create the resource, refcnt == 1
> > > > 2. ScopedPPResource::operator= assignment above, refcnt == 2
> > > > ... some time later, but not necessarily in this order ...
> > > > 3. Caller of MakeMediaBufferResource exits, refcnt == 1
> > > > 4. PPAPI glue finishes with the resource, refcnt == 0, resource
destroyed.
> > > 
> > > I guess I don't know what semantics you want. You have two options,
> generally:
> > > 1) You pass a reference to the plugin (e.g., when calling Decrypt).
> > > 2) You *don't* pass a reference to the plugin, and the plugin is required
to
> > > AddRefResource the PP_Resource if it wants it to live past the scope of
the
> > > call.
> > > 
> > > Looking at ppapi/cpp/content_decryptor_private.cc, you use the PP_Resource
> > > PassRef constructor, so I think you're trying to do option 1. The proxy
code
> > on
> > > the plugin side actually does pass a reference to the plugin (see
> > > ppp_content_decryptor_private_proxy.cc, which calls AddProxyResource,
whichs
> > > gives a resource with ref-count of 1, which you never release). So the
> plugin
> > > side of the proxy seems to fit with doing option 1.
> > > 
> > > In the host side of the proxy, it actually looks like you aren't
accounting
> > for
> > > the extra ref-count that this CL adds. I'm wondering if your ref-counting
> will
> > > be wrong on the host side with this patch?
> > > 
> > > I think probably you'll want to also tweak
> > > ppp_content_decryptor_private_proxy.cc:Decrypt to use a ScopedPPResource
to
> > make
> > > sure you release this ref count you're adding.
> > > 
> > 
> > We want 2, but it's the browser (ContentDecryptorDelegate) that wants the
> buffer
> > to live past the scope of the call in this case. Passing the reference to
the
> > plugin is the problem; it needs to remain owned by the scoper member in the
> > delegate.
> Well, it doesn't really matter whether you pass a ref to the plugin or not. It
> can always take a reference if it wants to. So make sure you account for that.
> E.g., if you're assuming the plugin can't hold on to the buffer past the scope
> of the call when you decide whether you can reuse the buffer, your buffer
reuse
> stuff is wrong (though it might not show in practice, if plugins never try to
> keep the buffer).
> 
> > 
> > Does the new patch look better?
> Yes, this looks self-consistent. You should probably think about your buffer
> reuse stuff and make sure it's right, though.
> 

I'm pretty certain the buffer reuse stuff is now correct with the delegate
hanging onto a reference. We just missed the problem initially because OOP
masked it.

> Also, I would suggest adding some documentation about the reference counting
> model to the C API (i.e., point out that the plugin must take a reference to
the
> buffer if it wants to keep it).

The pp::Buffer_Dev passed around on the plugin side remains in scope until after
the plugin is done with the input data. As things are currently implemented I
don't think the comment is necessary. I'm not really sure where it would go,
anyway-- once for each method that takes a resource arg, or in the interface
comment, maybe...

ddorwin, xhwang: WDYT re the comment. Also, PTAL. I think this is ready to land
excluding any discussion about comments. It would be nice to get the in process
fix landed.

Thanks!

[xhwang, 2012-12-08 08:07:14]

On 2012/12/08 07:14:24, Tom Finegan wrote:
> On 2012/12/07 22:53:35, dmichael wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/11477002/diff/1/webkit/plugins/ppapi/content_...
> > File webkit/plugins/ppapi/content_decryptor_delegate.cc (right):
> > 
> >
>
https://codereview.chromium.org/11477002/diff/1/webkit/plugins/ppapi/content_...
> > webkit/plugins/ppapi/content_decryptor_delegate.cc:1026: *resource =
> > media_resource;
> > On 2012/12/06 22:59:02, Tom Finegan wrote:
> > > On 2012/12/06 19:30:09, dmichael wrote:
> > > > On 2012/12/06 18:32:59, Tom Finegan wrote:
> > > > > On 2012/12/06 18:17:23, xhwang wrote:
> > > > > > Shouldn't this line add the reference too? +dmichael for insights.
> > > > > 
> > > > > Ref counted objects bake my noodle. :(
> > > > > 
> > > > > It does, but we still need one more reference.
> > > > > 
> > > > > 1. We create the resource, refcnt == 1
> > > > > 2. ScopedPPResource::operator= assignment above, refcnt == 2
> > > > > ... some time later, but not necessarily in this order ...
> > > > > 3. Caller of MakeMediaBufferResource exits, refcnt == 1
> > > > > 4. PPAPI glue finishes with the resource, refcnt == 0, resource
> destroyed.
> > > > 
> > > > I guess I don't know what semantics you want. You have two options,
> > generally:
> > > > 1) You pass a reference to the plugin (e.g., when calling Decrypt).
> > > > 2) You *don't* pass a reference to the plugin, and the plugin is
required
> to
> > > > AddRefResource the PP_Resource if it wants it to live past the scope of
> the
> > > > call.
> > > > 
> > > > Looking at ppapi/cpp/content_decryptor_private.cc, you use the
PP_Resource
> > > > PassRef constructor, so I think you're trying to do option 1. The proxy
> code
> > > on
> > > > the plugin side actually does pass a reference to the plugin (see
> > > > ppp_content_decryptor_private_proxy.cc, which calls AddProxyResource,
> whichs
> > > > gives a resource with ref-count of 1, which you never release). So the
> > plugin
> > > > side of the proxy seems to fit with doing option 1.
> > > > 
> > > > In the host side of the proxy, it actually looks like you aren't
> accounting
> > > for
> > > > the extra ref-count that this CL adds. I'm wondering if your
ref-counting
> > will
> > > > be wrong on the host side with this patch?
> > > > 
> > > > I think probably you'll want to also tweak
> > > > ppp_content_decryptor_private_proxy.cc:Decrypt to use a ScopedPPResource
> to
> > > make
> > > > sure you release this ref count you're adding.
> > > > 
> > > 
> > > We want 2, but it's the browser (ContentDecryptorDelegate) that wants the
> > buffer
> > > to live past the scope of the call in this case. Passing the reference to
> the
> > > plugin is the problem; it needs to remain owned by the scoper member in
the
> > > delegate.
> > Well, it doesn't really matter whether you pass a ref to the plugin or not.
It
> > can always take a reference if it wants to. So make sure you account for
that.
> > E.g., if you're assuming the plugin can't hold on to the buffer past the
scope
> > of the call when you decide whether you can reuse the buffer, your buffer
> reuse
> > stuff is wrong (though it might not show in practice, if plugins never try
to
> > keep the buffer).
> > 
> > > 
> > > Does the new patch look better?
> > Yes, this looks self-consistent. You should probably think about your buffer
> > reuse stuff and make sure it's right, though.
> > 
> 
> I'm pretty certain the buffer reuse stuff is now correct with the delegate
> hanging onto a reference. We just missed the problem initially because OOP
> masked it.
> 
> > Also, I would suggest adding some documentation about the reference counting
> > model to the C API (i.e., point out that the plugin must take a reference to
> the
> > buffer if it wants to keep it).
> 
> The pp::Buffer_Dev passed around on the plugin side remains in scope until
after
> the plugin is done with the input data. As things are currently implemented I
> don't think the comment is necessary. I'm not really sure where it would go,
> anyway-- once for each method that takes a resource arg, or in the interface
> comment, maybe...
> 
> ddorwin, xhwang: WDYT re the comment. Also, PTAL. I think this is ready to
land
> excluding any discussion about comments. It would be nice to get the in
process
> fix landed.
> 
> Thanks!

Since now in ppapi code we don't explicit add reference or trying to keep a
reference around, I don't think we need to add comments about ref-counting. This
CL lgtm. Tom: did you double check that in both in-process and out-of-process,
the refcount is 1 after the PPP call finished?

[Tom Finegan, 2012-12-09 20:11:23]

On 2012/12/08 08:07:14, xhwang wrote:
> On 2012/12/08 07:14:24, Tom Finegan wrote:
> > On 2012/12/07 22:53:35, dmichael wrote:
> > > lgtm
> > > 
> > >
> >
>
https://codereview.chromium.org/11477002/diff/1/webkit/plugins/ppapi/content_...
> > > File webkit/plugins/ppapi/content_decryptor_delegate.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/11477002/diff/1/webkit/plugins/ppapi/content_...
> > > webkit/plugins/ppapi/content_decryptor_delegate.cc:1026: *resource =
> > > media_resource;
> > > On 2012/12/06 22:59:02, Tom Finegan wrote:
> > > > On 2012/12/06 19:30:09, dmichael wrote:
> > > > > On 2012/12/06 18:32:59, Tom Finegan wrote:
> > > > > > On 2012/12/06 18:17:23, xhwang wrote:
> > > > > > > Shouldn't this line add the reference too? +dmichael for insights.
> > > > > > 
> > > > > > Ref counted objects bake my noodle. :(
> > > > > > 
> > > > > > It does, but we still need one more reference.
> > > > > > 
> > > > > > 1. We create the resource, refcnt == 1
> > > > > > 2. ScopedPPResource::operator= assignment above, refcnt == 2
> > > > > > ... some time later, but not necessarily in this order ...
> > > > > > 3. Caller of MakeMediaBufferResource exits, refcnt == 1
> > > > > > 4. PPAPI glue finishes with the resource, refcnt == 0, resource
> > destroyed.
> > > > > 
> > > > > I guess I don't know what semantics you want. You have two options,
> > > generally:
> > > > > 1) You pass a reference to the plugin (e.g., when calling Decrypt).
> > > > > 2) You *don't* pass a reference to the plugin, and the plugin is
> required
> > to
> > > > > AddRefResource the PP_Resource if it wants it to live past the scope
of
> > the
> > > > > call.
> > > > > 
> > > > > Looking at ppapi/cpp/content_decryptor_private.cc, you use the
> PP_Resource
> > > > > PassRef constructor, so I think you're trying to do option 1. The
proxy
> > code
> > > > on
> > > > > the plugin side actually does pass a reference to the plugin (see
> > > > > ppp_content_decryptor_private_proxy.cc, which calls AddProxyResource,
> > whichs
> > > > > gives a resource with ref-count of 1, which you never release). So the
> > > plugin
> > > > > side of the proxy seems to fit with doing option 1.
> > > > > 
> > > > > In the host side of the proxy, it actually looks like you aren't
> > accounting
> > > > for
> > > > > the extra ref-count that this CL adds. I'm wondering if your
> ref-counting
> > > will
> > > > > be wrong on the host side with this patch?
> > > > > 
> > > > > I think probably you'll want to also tweak
> > > > > ppp_content_decryptor_private_proxy.cc:Decrypt to use a
ScopedPPResource
> > to
> > > > make
> > > > > sure you release this ref count you're adding.
> > > > > 
> > > > 
> > > > We want 2, but it's the browser (ContentDecryptorDelegate) that wants
the
> > > buffer
> > > > to live past the scope of the call in this case. Passing the reference
to
> > the
> > > > plugin is the problem; it needs to remain owned by the scoper member in
> the
> > > > delegate.
> > > Well, it doesn't really matter whether you pass a ref to the plugin or
not.
> It
> > > can always take a reference if it wants to. So make sure you account for
> that.
> > > E.g., if you're assuming the plugin can't hold on to the buffer past the
> scope
> > > of the call when you decide whether you can reuse the buffer, your buffer
> > reuse
> > > stuff is wrong (though it might not show in practice, if plugins never try
> to
> > > keep the buffer).
> > > 
> > > > 
> > > > Does the new patch look better?
> > > Yes, this looks self-consistent. You should probably think about your
buffer
> > > reuse stuff and make sure it's right, though.
> > > 
> > 
> > I'm pretty certain the buffer reuse stuff is now correct with the delegate
> > hanging onto a reference. We just missed the problem initially because OOP
> > masked it.
> > 
> > > Also, I would suggest adding some documentation about the reference
counting
> > > model to the C API (i.e., point out that the plugin must take a reference
to
> > the
> > > buffer if it wants to keep it).
> > 
> > The pp::Buffer_Dev passed around on the plugin side remains in scope until
> after
> > the plugin is done with the input data. As things are currently implemented
I
> > don't think the comment is necessary. I'm not really sure where it would go,
> > anyway-- once for each method that takes a resource arg, or in the interface
> > comment, maybe...
> > 
> > ddorwin, xhwang: WDYT re the comment. Also, PTAL. I think this is ready to
> land
> > excluding any discussion about comments. It would be nice to get the in
> process
> > fix landed.
> > 
> > Thanks!
> 
> Since now in ppapi code we don't explicit add reference or trying to keep a
> reference around, I don't think we need to add comments about ref-counting.
This
> CL lgtm. Tom: did you double check that in both in-process and out-of-process,
> the refcount is 1 after the PPP call finished?

I added a bunch of DVLOGs in ContentDecryptorDelegate and ResourceTracker. Ref
counts look good in and out of process. Checking the box...

[commit-bot: I haz the power, 2012-12-09 20:12:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tomfinegan@chromium.org/11477002/12001

[commit-bot: I haz the power, 2012-12-09 21:34:49]

Retried try job too often on mac_rel for step(s) unit_tests

[dmichael (off chromium), 2012-12-10 17:47:32]

This change lgtm; but I'm not convinced about my other comments...

On Sat, Dec 8, 2012 at 12:14 AM, <tomfinegan@chromium.org> wrote:

> On 2012/12/07 22:53:35, dmichael wrote:
>
>> lgtm
>>
>
>
> https://codereview.chromium.**org/11477002/diff/1/webkit/**
>
plugins/ppapi/content_**decryptor_delegate.cc<https://codereview.chromium.org/11477002/diff/1/webkit/plugins/ppapi/content_decryptor_delegate.cc>
>
>> File webkit/plugins/ppapi/content_**decryptor_delegate.cc (right):
>>
>
>
> https://codereview.chromium.**org/11477002/diff/1/webkit/**
>
plugins/ppapi/content_**decryptor_delegate.cc#**newcode1026<https://codereview.chromium.org/11477002/diff/1/webkit/plugins/ppapi/content_decryptor_delegate.cc#newcode1026>
>
>> webkit/plugins/ppapi/content_**decryptor_delegate.cc:1026: *resource =
>> media_resource;
>> On 2012/12/06 22:59:02, Tom Finegan wrote:
>> > On 2012/12/06 19:30:09, dmichael wrote:
>> > > On 2012/12/06 18:32:59, Tom Finegan wrote:
>> > > > On 2012/12/06 18:17:23, xhwang wrote:
>> > > > > Shouldn't this line add the reference too? +dmichael for insights.
>> > > >
>> > > > Ref counted objects bake my noodle. :(
>> > > >
>> > > > It does, but we still need one more reference.
>> > > >
>> > > > 1. We create the resource, refcnt == 1
>> > > > 2. ScopedPPResource::operator= assignment above, refcnt == 2
>> > > > ... some time later, but not necessarily in this order ...
>> > > > 3. Caller of MakeMediaBufferResource exits, refcnt == 1
>> > > > 4. PPAPI glue finishes with the resource, refcnt == 0, resource
>>
> destroyed.
>
>> > >
>> > > I guess I don't know what semantics you want. You have two options,
>> generally:
>> > > 1) You pass a reference to the plugin (e.g., when calling Decrypt).
>> > > 2) You *don't* pass a reference to the plugin, and the plugin is
>> required
>>
> to
>
>> > > AddRefResource the PP_Resource if it wants it to live past the scope
>> of
>>
> the
>
>> > > call.
>> > >
>> > > Looking at ppapi/cpp/content_decryptor_**private.cc, you use the
>> PP_Resource
>> > > PassRef constructor, so I think you're trying to do option 1. The
>> proxy
>>
> code
>
>> > on
>> > > the plugin side actually does pass a reference to the plugin (see
>> > > ppp_content_decryptor_private_**proxy.cc, which calls
>> AddProxyResource,
>>
> whichs
>
>> > > gives a resource with ref-count of 1, which you never release). So the
>> plugin
>> > > side of the proxy seems to fit with doing option 1.
>> > >
>> > > In the host side of the proxy, it actually looks like you aren't
>>
> accounting
>
>> > for
>> > > the extra ref-count that this CL adds. I'm wondering if your
>> ref-counting
>> will
>> > > be wrong on the host side with this patch?
>> > >
>> > > I think probably you'll want to also tweak
>> > > ppp_content_decryptor_private_**proxy.cc:Decrypt to use a
>> ScopedPPResource
>>
> to
>
>> > make
>> > > sure you release this ref count you're adding.
>> > >
>> >
>> > We want 2, but it's the browser (ContentDecryptorDelegate) that wants
>> the
>> buffer
>> > to live past the scope of the call in this case. Passing the reference
>> to
>>
> the
>
>> > plugin is the problem; it needs to remain owned by the scoper member in
>> the
>> > delegate.
>> Well, it doesn't really matter whether you pass a ref to the plugin or
>> not. It
>> can always take a reference if it wants to. So make sure you account for
>> that.
>> E.g., if you're assuming the plugin can't hold on to the buffer past the
>> scope
>> of the call when you decide whether you can reuse the buffer, your buffer
>>
> reuse
>
>> stuff is wrong (though it might not show in practice, if plugins never
>> try to
>> keep the buffer).
>>
>
>  >
>> > Does the new patch look better?
>> Yes, this looks self-consistent. You should probably think about your
>> buffer
>> reuse stuff and make sure it's right, though.
>>
>
>
> I'm pretty certain the buffer reuse stuff is now correct with the delegate
> hanging onto a reference. We just missed the problem initially because OOP
> masked it.

I'm not worried about the reference counting anymore; that's probably fine.
Imagine for the moment that you're implementing this API for any random
plugin that wants to use it. (I know this isn't the case for your APIs, but
it might help you understand where I'm coming from). Imagine the following
situation:
1) Chrome calls Decrypt() on the plugin, providing the encrypted buffer.
2) The plugin decides it wants to keep that buffer around, so it AddRefs it
(or simply copies the pp::Buffer_Dev, which does the same thing).
The plugin probably expects the data in that buffer not to change. But I
think you will happily reuse the buffer and write to it. For most of our
APIs, this would be unacceptable. We would only be allowed to reuse buffers
that we know the plugin is completely done with. For example, we reuse
PPB_ImageData resources. When the last reference count is released for an
ImageData, ImageData::LastPluginRefWasDeleted gets called:
https://code.google.com/searchframe#OAMlx_jo-ck/src/ppapi/proxy/ppb_image_dat...
And *that's *when we know we can reuse the ImageData again.

I didn't catch this earlier, but the CDM buffer reuse code doesn't account
for this.

Will we always be in full control of all CDM implementations? If so, we can
maybe get away with this, but we need to *at least* comment in the PPP APIs
that the buffers get reused, so CDMs should not retain the Buffer beyond
the scope of the calls to Decrypt or DecryptAndDecode (or whatever other
methods are affected).

I'd like to see us do a more general shared memory pool that would fix this
for you guys, but I don't think it's happening soon.


>
>  Also, I would suggest adding some documentation about the reference
>> counting
>> model to the C API (i.e., point out that the plugin must take a reference
>> to
>>
> the
>
>> buffer if it wants to keep it).
>>
>
> The pp::Buffer_Dev passed around on the plugin side remains in scope until
> after
> the plugin is done with the input data. As things are currently
> implemented I
> don't think the comment is necessary. I'm not really sure where it would
> go,
> anyway-- once for each method that takes a resource arg, or in the
> interface
> comment, maybe...
>
The point I was making is: you are making a choice w.r.t. reference
counting semantics, and it could easily be "pass ref" or "don't pass a
ref". I think it's important to tell plugin authors which behavior it has,
so they know what to implement without digging in to our code. Now that I
thought more about the buffer reuse stuff I talked about above, I think the
comment should be something like "The plugin must not take a reference to
the buffer".


>
> ddorwin, xhwang: WDYT re the comment. Also, PTAL. I think this is ready to
> land
> excluding any discussion about comments. It would be nice to get the in
> process
> fix landed.
>
> Thanks!
>
>
https://codereview.chromium.**org/11477002/<https://codereview.chromium.org/1...
>

[xhwang, 2012-12-10 18:13:18]

On 2012/12/10 17:47:32, dmichael wrote:
> This change lgtm; but I'm not convinced about my other comments...
> 
> On Sat, Dec 8, 2012 at 12:14 AM, <mailto:tomfinegan@chromium.org> wrote:
> 
> > On 2012/12/07 22:53:35, dmichael wrote:
> >
> >> lgtm
> >>
> >
> >
> > https://codereview.chromium.**org/11477002/diff/1/webkit/**
> >
>
plugins/ppapi/content_**decryptor_delegate.cc<https://codereview.chromium.org/11477002/diff/1/webkit/plugins/ppapi/content_decryptor_delegate.cc>
> >
> >> File webkit/plugins/ppapi/content_**decryptor_delegate.cc (right):
> >>
> >
> >
> > https://codereview.chromium.**org/11477002/diff/1/webkit/**
> >
>
plugins/ppapi/content_**decryptor_delegate.cc#**newcode1026<https://codereview.chromium.org/11477002/diff/1/webkit/plugins/ppapi/content_decryptor_delegate.cc#newcode1026>
> >
> >> webkit/plugins/ppapi/content_**decryptor_delegate.cc:1026: *resource =
> >> media_resource;
> >> On 2012/12/06 22:59:02, Tom Finegan wrote:
> >> > On 2012/12/06 19:30:09, dmichael wrote:
> >> > > On 2012/12/06 18:32:59, Tom Finegan wrote:
> >> > > > On 2012/12/06 18:17:23, xhwang wrote:
> >> > > > > Shouldn't this line add the reference too? +dmichael for insights.
> >> > > >
> >> > > > Ref counted objects bake my noodle. :(
> >> > > >
> >> > > > It does, but we still need one more reference.
> >> > > >
> >> > > > 1. We create the resource, refcnt == 1
> >> > > > 2. ScopedPPResource::operator= assignment above, refcnt == 2
> >> > > > ... some time later, but not necessarily in this order ...
> >> > > > 3. Caller of MakeMediaBufferResource exits, refcnt == 1
> >> > > > 4. PPAPI glue finishes with the resource, refcnt == 0, resource
> >>
> > destroyed.
> >
> >> > >
> >> > > I guess I don't know what semantics you want. You have two options,
> >> generally:
> >> > > 1) You pass a reference to the plugin (e.g., when calling Decrypt).
> >> > > 2) You *don't* pass a reference to the plugin, and the plugin is
> >> required
> >>
> > to
> >
> >> > > AddRefResource the PP_Resource if it wants it to live past the scope
> >> of
> >>
> > the
> >
> >> > > call.
> >> > >
> >> > > Looking at ppapi/cpp/content_decryptor_**private.cc, you use the
> >> PP_Resource
> >> > > PassRef constructor, so I think you're trying to do option 1. The
> >> proxy
> >>
> > code
> >
> >> > on
> >> > > the plugin side actually does pass a reference to the plugin (see
> >> > > ppp_content_decryptor_private_**proxy.cc, which calls
> >> AddProxyResource,
> >>
> > whichs
> >
> >> > > gives a resource with ref-count of 1, which you never release). So the
> >> plugin
> >> > > side of the proxy seems to fit with doing option 1.
> >> > >
> >> > > In the host side of the proxy, it actually looks like you aren't
> >>
> > accounting
> >
> >> > for
> >> > > the extra ref-count that this CL adds. I'm wondering if your
> >> ref-counting
> >> will
> >> > > be wrong on the host side with this patch?
> >> > >
> >> > > I think probably you'll want to also tweak
> >> > > ppp_content_decryptor_private_**proxy.cc:Decrypt to use a
> >> ScopedPPResource
> >>
> > to
> >
> >> > make
> >> > > sure you release this ref count you're adding.
> >> > >
> >> >
> >> > We want 2, but it's the browser (ContentDecryptorDelegate) that wants
> >> the
> >> buffer
> >> > to live past the scope of the call in this case. Passing the reference
> >> to
> >>
> > the
> >
> >> > plugin is the problem; it needs to remain owned by the scoper member in
> >> the
> >> > delegate.
> >> Well, it doesn't really matter whether you pass a ref to the plugin or
> >> not. It
> >> can always take a reference if it wants to. So make sure you account for
> >> that.
> >> E.g., if you're assuming the plugin can't hold on to the buffer past the
> >> scope
> >> of the call when you decide whether you can reuse the buffer, your buffer
> >>
> > reuse
> >
> >> stuff is wrong (though it might not show in practice, if plugins never
> >> try to
> >> keep the buffer).
> >>
> >
> >  >
> >> > Does the new patch look better?
> >> Yes, this looks self-consistent. You should probably think about your
> >> buffer
> >> reuse stuff and make sure it's right, though.
> >>
> >
> >
> > I'm pretty certain the buffer reuse stuff is now correct with the delegate
> > hanging onto a reference. We just missed the problem initially because OOP
> > masked it.
> 
> I'm not worried about the reference counting anymore; that's probably fine.
> Imagine for the moment that you're implementing this API for any random
> plugin that wants to use it. (I know this isn't the case for your APIs, but
> it might help you understand where I'm coming from). Imagine the following
> situation:
> 1) Chrome calls Decrypt() on the plugin, providing the encrypted buffer.
> 2) The plugin decides it wants to keep that buffer around, so it AddRefs it
> (or simply copies the pp::Buffer_Dev, which does the same thing).
> The plugin probably expects the data in that buffer not to change. But I
> think you will happily reuse the buffer and write to it. For most of our
> APIs, this would be unacceptable. We would only be allowed to reuse buffers
> that we know the plugin is completely done with. For example, we reuse
> PPB_ImageData resources. When the last reference count is released for an
> ImageData, ImageData::LastPluginRefWasDeleted gets called:
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/ppapi/proxy/ppb_image_dat...
> And *that's *when we know we can reuse the ImageData again.
> 
> I didn't catch this earlier, but the CDM buffer reuse code doesn't account
> for this.
> 
> Will we always be in full control of all CDM implementations? If so, we can
> maybe get away with this, but we need to *at least* comment in the PPP APIs
> that the buffers get reused, so CDMs should not retain the Buffer beyond
> the scope of the calls to Decrypt or DecryptAndDecode (or whatever other
> methods are affected).
> 
> I'd like to see us do a more general shared memory pool that would fix this
> for you guys, but I don't think it's happening soon.
> 
I agree with the points you make here. When I implemented the renderer side
shared
memory reuse, I knew that *our* plugin (the CdmWrapper) doesn't keep
the buffer around, and I took advantage of that. I agree that if some other
people
want to use these APIs to implement another plugin, this could be a problem.
It'll be nice to replace the current implementation with a more general approach
(with some no-longer-needed callback). But, since I don't see anybody
implementing
another plugin for chromium now or any time soon, I might do that later. Will
that
be okay?

In the implementation of the plugin side shared memory reuse, since we can't
control the media pipeline, we have the callback to notify that some buffer is
no longer needed. So that part should be okay.

> 
> >
> >  Also, I would suggest adding some documentation about the reference
> >> counting
> >> model to the C API (i.e., point out that the plugin must take a reference
> >> to
> >>
> > the
> >
> >> buffer if it wants to keep it).
> >>
> >
> > The pp::Buffer_Dev passed around on the plugin side remains in scope until
> > after
> > the plugin is done with the input data. As things are currently
> > implemented I
> > don't think the comment is necessary. I'm not really sure where it would
> > go,
> > anyway-- once for each method that takes a resource arg, or in the
> > interface
> > comment, maybe...
> >
> The point I was making is: you are making a choice w.r.t. reference
> counting semantics, and it could easily be "pass ref" or "don't pass a
> ref". I think it's important to tell plugin authors which behavior it has,
> so they know what to implement without digging in to our code. Now that I
> thought more about the buffer reuse stuff I talked about above, I think the
> comment should be something like "The plugin must not take a reference to
> the buffer".
> 
> 
> >
> > ddorwin, xhwang: WDYT re the comment. Also, PTAL. I think this is ready to
> > land
> > excluding any discussion about comments. It would be nice to get the in
> > process
> > fix landed.
> >
> > Thanks!
> >
> >
>
https://codereview.chromium.**org/11477002/%3Chttps://codereview.chromium.org...>
> >

[ddorwin, 2012-12-10 18:47:07]

Thanks for clarifying and catching this, dmichael.

I think a comment makes sense because of how we reuse buffers. Maybe on the
Deliver*() methods we should say the plugin must no longer be holding a
reference to the encrypted buffer provided in the corresponding Decrypt*().

The reason for this is that the Deliver*() call is what the renderer uses as a
trigger that the encrypted buffer is no longer in use. This works because we
only call Decrypt*() once per stream type.


A generic buffer pool or LastPluginRefWasDeleted() call would be nice, but we'd
need to think about the performance implications if we have to add one or more
additional IPCs, especially synchronous ones, for each frame. Maybe we can
provide buffer management infrastructure that can be rolled into an APIs
existing methods.

[dmichael (off chromium), 2012-12-10 19:43:12]

On Mon, Dec 10, 2012 at 11:47 AM, <ddorwin@chromium.org> wrote:

> Thanks for clarifying and catching this, dmichael.
>
> I think a comment makes sense because of how we reuse buffers. Maybe on the
> Deliver*() methods we should say the plugin must no longer be holding a
> reference to the encrypted buffer provided in the corresponding Decrypt*().
>
> The reason for this is that the Deliver*() call is what the renderer uses
> as a
> trigger that the encrypted buffer is no longer in use. This works because
> we
> only call Decrypt*() once per stream type.
>
SGTM. I think for CDM, a good comment will suffice for now.


>
> A generic buffer pool or LastPluginRefWasDeleted() call would be nice, but
> we'd
> need to think about the performance implications if we have to add one or
> more
> additional IPCs, especially synchronous ones, for each frame. Maybe we can
> provide buffer management infrastructure that can be rolled into an APIs
> existing methods.
>
We might try to look at a more general buffer pool solution after 25, and
we can involve you guys if you want.


>
>
https://codereview.chromium.**org/11477002/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2012-12-10 20:55:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tomfinegan@chromium.org/11477002/29002

[commit-bot: I haz the power, 2012-12-10 20:55:16]

Presubmit check for 11477002-29002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
PPAPI IDL Diff detected: Run the generator.

***************
--- OLD ../c/private/ppb_content_decryptor_private.h 
+++ NEW ../c/private/ppb_content_decryptor_private.h 
@@ -5,4 +5,4 @@
 
-/* From private\ppb_content_decryptor_private.idl,
- *   modified Mon Dec 10 12:36:42 2012.
+/* From private/ppb_content_decryptor_private.idl,
+ *   modified Mon Dec 10 12:55:13 2012.
  */
Error C Header : Failed to generate range 0 : 17.
***************

Presubmit checks took 2.2s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[commit-bot: I haz the power, 2012-12-10 21:19:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tomfinegan@chromium.org/11477002/31003

[commit-bot: I haz the power, 2012-12-10 21:19:50]

Presubmit check for 11477002-31003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
PPAPI IDL Diff detected: Run the generator.

***************
--- OLD ../c/private/ppb_content_decryptor_private.h 
+++ NEW ../c/private/ppb_content_decryptor_private.h 
@@ -5,4 +5,4 @@
 
-/* From private\ppb_content_decryptor_private.idl,
- *   modified Mon Dec 10 13:16:23 2012.
+/* From private/ppb_content_decryptor_private.idl,
+ *   modified Mon Dec 10 13:19:43 2012.
  */
Error C Header : Failed to generate range 0 : 17.
***************

Presubmit checks took 2.4s to calculate.

[commit-bot: I haz the power, 2012-12-10 21:48:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tomfinegan@chromium.org/11477002/32005

[commit-bot: I haz the power, 2012-12-11 00:41:24]

Change committed as 172211

[dmichael (off chromium), 2012-12-13 19:20:35]

https://chromiumcodereview.appspot.com/11477002/diff/32005/ppapi/proxy/ppp_co...
File ppapi/proxy/ppp_content_decryptor_private_proxy.cc (left):

https://chromiumcodereview.appspot.com/11477002/diff/32005/ppapi/proxy/ppp_co...
ppapi/proxy/ppp_content_decryptor_private_proxy.cc:92: }
I was confused when reading the CL...  I think this AddRef still needs to
happen. What you're doing here is making sure that the Buffer Resource stays
valid long enough for the Plugin to take it. The messages you send (e.g., to
Decrypt) are asynchronous. If you don't AddRef them before sending them, I think
it's possible that the Buffer will be released before the plugin is done with
it.

I think you want to AddRef here (or maybe just before you send it, to make it
easier to balance?).

Maybe AddRefing right before you send, then RemoveRef on the plugin side right
after you call the related PPP function.