Fix resource leaks in CDM implementation.

webkit/plugins/ppapi/ppapi_plugin_instance.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/ppapi_plugin_instance.h"
 
 #include "base/bind.h"
 #include "base/debug/trace_event.h"
 #include "base/logging.h"
 #include "base/memory/linked_ptr.h"
@@ skipping 2160 matching lines @@
       key_system_string->value(),
       session_id_string->value(),
       static_cast<media::Decryptor::KeyError>(media_error),
       system_code);
 }
 
 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;
   }
+
+  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;
   }
   BufferAutoMapper mapper(enter.object());
   if (!mapper.data() || !mapper.size()) {
     decrypt_cb.Run(media::Decryptor::kError, NULL);
     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()));
   decrypted_buffer->SetTimestamp(base::TimeDelta::FromMicroseconds(
       block_info->tracking_info.timestamp));
   decrypt_cb.Run(media::Decryptor::kSuccess, decrypted_buffer);
 }
 
 void PluginInstance::DeliverFrame(PP_Instance instance,
                                   PP_Resource decrypted_frame,
@@ skipping 382 matching lines @@
   screen_size_for_fullscreen_ = gfx::Size();
   WebElement element = container_->element();
   element.setAttribute(WebString::fromUTF8(kWidth), width_before_fullscreen_);
   element.setAttribute(WebString::fromUTF8(kHeight), height_before_fullscreen_);
   element.setAttribute(WebString::fromUTF8(kBorder), border_before_fullscreen_);
   element.setAttribute(WebString::fromUTF8(kStyle), style_before_fullscreen_);
 }
 
 }  // namespace ppapi
 }  // namespace webkit