Fix resource leaks in CDM implementation.

webkit/plugins/ppapi/ppapi_plugin_instance.cc

Index: webkit/plugins/ppapi/ppapi_plugin_instance.cc
diff --git a/webkit/plugins/ppapi/ppapi_plugin_instance.cc b/webkit/plugins/ppapi/ppapi_plugin_instance.cc
index 66940e831b73866c0cdccee620dd2e5427b20599..962851ff43c8d3c9f990508d532a8f2b83c54373 100644
--- a/webkit/plugins/ppapi/ppapi_plugin_instance.cc
+++ b/webkit/plugins/ppapi/ppapi_plugin_instance.cc
@@ -2178,25 +2178,27 @@ void PluginInstance::DeliverBlock(PP_Instance instance,
                                   PP_Resource decrypted_block,
                                   const PP_DecryptedBlockInfo* block_info) {
   DCHECK(block_info);
-
   DecryptionCBMap::iterator found = pending_decryption_cbs_.find(
       block_info->tracking_info.request_id);
-
   if (found == pending_decryption_cbs_.end())
     return;
   media::Decryptor::DecryptCB decrypt_cb = found->second;
   pending_decryption_cbs_.erase(found);
 
   if (block_info->result == PP_DECRYPTRESULT_DECRYPT_NOKEY) {
+    DCHECK(decrypted_block == 0);

[dmichael (off chromium), 2012/09/05 17:13:06]

Don't we get here via a call from the CDM plugin? If so, we can't trust the
inputs, and we shouldn't crash on bad input.

[Tom Finegan, 2012/09/05 19:21:43]

Done.

[ddorwin, 2012/09/06 08:39:01]

I suggested this. My concern was that we might leak buffers on errors. If it's a
DCHECK, then we crash in debug mode if a CDM is misbehaving. CDMs aren't just
random pieces of code, so it seems fine to crash in debug if they have a bug.

     decrypt_cb.Run(media::Decryptor::kNoKey, NULL);
     return;
   }
   if (block_info->result != PP_DECRYPTRESULT_SUCCESS) {
+    DCHECK(decrypted_block == 0);
     decrypt_cb.Run(media::Decryptor::kError, NULL);
     return;
   }
-  EnterResourceNoLock<PPB_Buffer_API> enter(decrypted_block, true);
 
+  ScopedPPResource decrypted_block_scoped(ScopedPPResource::PassRef(),
+                                          decrypted_block);

[dmichael (off chromium), 2012/09/05 17:13:06]

Why is this necessary? The plugin isn't passing us a reference. If the plugin
still has a reference, we shouldn't take it away. If it's leaking, it probably
means that the plugin side has an extra reference.

[Tom Finegan, 2012/09/05 19:21:43]

The resource passed back from the plugin has a ref count of one, but the plugin
no longer has access to it. It's been handed to the browser so the decrypted
data can be sent to the media stack. I _think_ this is correct as-is-- without
it we still leak.

+  EnterResourceNoLock<PPB_Buffer_API> enter(decrypted_block, true);
   if (!enter.succeeded()) {
     decrypt_cb.Run(media::Decryptor::kError, NULL);
     return;
@@ -2207,6 +2209,8 @@ void PluginInstance::DeliverBlock(PP_Instance instance,
     return;
   }
 
+  // TODO(tomfinegan): Find a way to take ownership of the shared memory
+  // managed by the PPB_Buffer_Dev, and avoid the extra copy.
   scoped_refptr<media::DecoderBuffer> decrypted_buffer(
       media::DecoderBuffer::CopyFrom(
           reinterpret_cast<const uint8*>(mapper.data()), mapper.size()));