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"
 #include "base/message_loop.h"
 #include "base/stl_util.h"
 #include "base/stringprintf.h"
 #include "base/time.h"
 #include "base/utf_offset_string_conversions.h"
 #include "base/utf_string_conversions.h"
 #include "media/base/decoder_buffer.h"
 #include "media/base/decryptor_client.h"
 #include "ppapi/c/dev/ppb_find_dev.h"
 #include "ppapi/c/dev/ppb_zoom_dev.h"
 #include "ppapi/c/dev/ppp_find_dev.h"
 #include "ppapi/c/dev/ppp_selection_dev.h"
+#include "ppapi/c/dev/ppb_testing_dev.h"

[ddorwin, 2012/09/05 16:17:51]

This doesn't seem right.

[Tom Finegan, 2012/09/05 16:41:56]

Done.

 #include "ppapi/c/dev/ppp_text_input_dev.h"
 #include "ppapi/c/dev/ppp_zoom_dev.h"
 #include "ppapi/c/pp_rect.h"
 #include "ppapi/c/ppb_audio_config.h"
 #include "ppapi/c/ppb_core.h"
 #include "ppapi/c/ppb_gamepad.h"
 #include "ppapi/c/ppp_input_event.h"
 #include "ppapi/c/ppp_instance.h"
 #include "ppapi/c/ppp_messaging.h"
 #include "ppapi/c/ppp_mouse_lock.h"
@@ skipping 2135 matching lines @@
 
   DCHECK(decryptor_client_);
   decryptor_client_->KeyError(
       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,
+                                  PP_Resource decrypted_block_arg,

[ddorwin, 2012/09/05 16:17:51]

Prefer to keep parameter names nice (and consistent with .h file). Use _local or
_scoped below instead.

[Tom Finegan, 2012/09/05 16:41:56]

Done.

                                   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) {
     decrypt_cb.Run(media::Decryptor::kNoKey, NULL);

[ddorwin, 2012/09/05 16:17:51]

PP_DCHECK that the decrypted_block is not valid since we're not dereferencing
it. Same below.

[Tom Finegan, 2012/09/05 16:41:56]

Done.

     return;
   }
+
   if (block_info->result != PP_DECRYPTRESULT_SUCCESS) {
     decrypt_cb.Run(media::Decryptor::kError, NULL);
     return;
   }
+
+  ScopedPPResource decrypted_block(ScopedPPResource::PassRef(),
+                                   decrypted_block_arg);
   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;
   }
 
   scoped_refptr<media::DecoderBuffer> decrypted_buffer(
       media::DecoderBuffer::CopyFrom(

[ddorwin, 2012/09/05 16:17:51]

We should try to avoid copies. With this and the Clear Key implementation, we
are copying decrypted buffers twice before we even get to the media stack.

Is there a way to take ownership of the shared memory while destroying the
buffer? Then we could pass ownership.

@dmichael: Are there any existing patterns for this?

I don't want to block this fix on finding a solution, but we should at least add
a TODO.

[Tom Finegan, 2012/09/05 16:41:56]

Added TODO.

           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,
                                   const PP_DecryptedBlockInfo* block_info) {
   // TODO(tomfinegan): To be implemented after completion of v0.1 of the
@@ skipping 380 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