Fix in process PPAPI decryption.

webkit/plugins/ppapi/content_decryptor_delegate.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "webkit/plugins/ppapi/content_decryptor_delegate.h"
 
 #include "base/callback_helpers.h"
 #include "base/debug/trace_event.h"
 #include "base/message_loop_proxy.h"
 #include "media/base/audio_decoder_config.h"
@@ skipping 986 matching lines @@
              << " bytes to fit input.";
     media_resource = ScopedPPResource(
         ScopedPPResource::PassRef(),
         PPB_Buffer_Impl::Create(pp_instance_, media_resource_size));
     if (!media_resource.get()) {
       media_resource_size = 0;
       return false;
     }
   }
 
+  // Take an extra reference on the resource to ensure that the reference count
+  // does not reach zero when the PPAPI glue releases its reference.
+  PpapiGlobals::Get()->GetResourceTracker()->AddRefResource(media_resource);

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

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)

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

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...

+
   EnterResourceNoLock<PPB_Buffer_API> enter(media_resource, true);
   if (enter.failed()) {
     media_resource.Release();
     media_resource_size = 0;
     return false;
   }
 
   BufferAutoMapper mapper(enter.object());
   if (!mapper.data() || mapper.size() < static_cast<size_t>(size)) {
     media_resource.Release();
     media_resource_size = 0;
     return false;
   }
   memcpy(mapper.data(), data, size);
 
   *resource = media_resource;

[xhwang, 2012/12/06 18:17:23]

Shouldn't this line add the reference too? +dmichael for insights.

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

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.

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

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]

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]

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).
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).

   return true;
 }
 
 void ContentDecryptorDelegate::FreeBuffer(uint32_t buffer_id) {
   if (buffer_id)
     free_buffers_.push(buffer_id);
 }
 
 void ContentDecryptorDelegate::SetBufferToFreeInTrackingInfo(
     PP_DecryptTrackingInfo* tracking_info) {
   DCHECK_EQ(tracking_info->buffer_id, 0u);
 
   if (free_buffers_.empty())
     return;
 
   tracking_info->buffer_id = free_buffers_.front();
   free_buffers_.pop();
 }
 
 }  // namespace ppapi
 }  // namespace webkit