Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10909068:
Fix resource leaks in CDM implementation. (Closed)
|
Created:
8 years, 3 months ago by Tom Finegan Modified:
8 years, 3 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix resource leaks in CDM implementation.
Two fixes:
1) Add a PassRef constructor to pp::Buffer_Dev to deal with reference counting
issues related to passing PP_Resources across the IPC proxy, and use it to
avoid adding unnecessary references on PP_Resources.
2) Use the PassRef ScopedPPResource contructor in PluginInstance::Decrypt to
avoid added an unnecessary reference on the buffer resource before it's
passed to the plugin interface code.
BUG=146200
TEST=PPB_Testing_Dev::GetLiveObjectsForInstance shows a stable live
object count.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155145
Patch Set 1 #Patch Set 2 : Remove includes from my debug runs... #
Total comments: 9
Patch Set 3 : Use ScopedPPResource in PluginInstance::DeliverBlock #
Total comments: 12
Patch Set 4 : Address ddorwin's comments. #
Total comments: 7
Patch Set 5 : Work done per dmichael's comments #
Total comments: 6
Patch Set 6 : I think we have a winner this time... #Patch Set 7 : Fix leaked ref in CdmWrapper::DeliverBlock #
Total comments: 4
Patch Set 8 : Hopefully all set for CQ #Patch Set 9 : Same patchset as number 8, hopefully this one is viewable... #
Total comments: 4
Patch Set 10 : Updates per last set of comments. #
Messages
Total messages: 19 (0 generated)
dmichael, These three small changes stabilize the live object count, and should hopefully be all that's required to fix crbug.com/146200. PTAL/Thanks!
ddorwin, xhwang, PTAL as well; I'll need a webkit/media owner's LGTM. Thanks!
LGTM for webkit/media OWNERS. I defer to dmichael for ppapi refcounting issues. http://codereview.chromium.org/10909068/diff/2001/webkit/plugins/ppapi/ppapi_... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10909068/diff/2001/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2214: decrypt_cb.Run(media::Decryptor::kError, NULL); Why aren't you returning early (if(!succeeded)) anymore? http://codereview.chromium.org/10909068/diff/2001/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2218: PluginModule::GetCore()->ReleaseResource(decrypted_block); Does the browser not always take ownership? What happens if one of the errors above occurs? The plugin won't know about the failure or release it.
http://codereview.chromium.org/10909068/diff/2001/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10909068/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:237: pp::Module::Get()->core()->ReleaseResource(encrypted_buffer.pp_resource()); Is this ReleaseResource called because we are passing in pp::Buffer_Dev by value? Can we pass by const reference to avoid this? If we really need to call it here, move it right after cdm_->Decrypt() call? http://codereview.chromium.org/10909068/diff/2001/webkit/plugins/ppapi/ppapi_... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10909068/diff/2001/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2218: PluginModule::GetCore()->ReleaseResource(decrypted_block); On 2012/09/05 08:44:44, ddorwin wrote: > Does the browser not always take ownership? What happens if one of the errors > above occurs? The plugin won't know about the failure or release it. Agreed. Is there something like scoped_resource we can use?
Using ScopedPPResource (hopefully) addresses all existing comments. Leak remains fixed in my check out. http://codereview.chromium.org/10909068/diff/2001/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10909068/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:237: pp::Module::Get()->core()->ReleaseResource(encrypted_buffer.pp_resource()); On 2012/09/05 09:26:55, xhwang wrote: > Is this ReleaseResource called because we are passing in pp::Buffer_Dev by > value? Can we pass by const reference to avoid this? > > If we really need to call it here, move it right after cdm_->Decrypt() call? ReleaseResource must be called in some manner. The browser owns the last reference on the PP_Resource. Using scoper to hide the gory details in latest patch.
LGTM % nits. Go ahead and commit if you get dmichael's approval. http://codereview.chromium.org/10909068/diff/11001/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10909068/diff/11001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:23: #include "ppapi/c/dev/ppb_testing_dev.h" This doesn't seem right. http://codereview.chromium.org/10909068/diff/11001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2179: PP_Resource decrypted_block_arg, Prefer to keep parameter names nice (and consistent with .h file). Use _local or _scoped below instead. http://codereview.chromium.org/10909068/diff/11001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2192: decrypt_cb.Run(media::Decryptor::kNoKey, NULL); PP_DCHECK that the decrypted_block is not valid since we're not dereferencing it. Same below. http://codereview.chromium.org/10909068/diff/11001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2216: media::DecoderBuffer::CopyFrom( 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.
http://codereview.chromium.org/10909068/diff/11001/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10909068/diff/11001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:23: #include "ppapi/c/dev/ppb_testing_dev.h" On 2012/09/05 16:17:51, ddorwin wrote: > This doesn't seem right. Done. http://codereview.chromium.org/10909068/diff/11001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2179: PP_Resource decrypted_block_arg, On 2012/09/05 16:17:51, ddorwin wrote: > Prefer to keep parameter names nice (and consistent with .h file). Use _local or > _scoped below instead. Done. http://codereview.chromium.org/10909068/diff/11001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2192: decrypt_cb.Run(media::Decryptor::kNoKey, NULL); On 2012/09/05 16:17:51, ddorwin wrote: > PP_DCHECK that the decrypted_block is not valid since we're not dereferencing > it. Same below. Done. http://codereview.chromium.org/10909068/diff/11001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2216: media::DecoderBuffer::CopyFrom( On 2012/09/05 16:17:51, ddorwin wrote: > 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. Added TODO.
This reference counting stuff in the proxy is tricky. These comments are based on how I *think* things should work. I need to dig a little bit more to make sure it works that way :-p. Sending resources over a PPP interface is fairly rare, so it's possible we have some underlying bugs. One thing you need to know is that the host side and the plugin side hold there own distinct reference counts, and each has its own ResourceTracker. The host (i.e., the renderer process) side is "truth". If a resource gets to 0 on the host side, it will be deleted. The plugin side of the proxy has its own reference count that should reflect what the plugin is doing. But the proxy should be written to make sure that so long as the plugin is using the resource, the host will have a reference on it. http://codereview.chromium.org/10909068/diff/2001/ppapi/proxy/ppp_content_dec... File ppapi/proxy/ppp_content_decryptor_private_proxy.cc (left): http://codereview.chromium.org/10909068/diff/2001/ppapi/proxy/ppp_content_dec... ppapi/proxy/ppp_content_decryptor_private_proxy.cc:146: core->AddRefResource(encrypted_block); tl;dr: Keep this. What you're missing is the corresponding dereference. I think you still need this. You want to make sure that the resource lives long enough that it's still alive when the plugin receives it. Since you're sending an asynchronous message (as you should), it's reasonably likely that the browser will be done with this call before the plugin can handle the PpapiMsg_PPPContentDecryptor_Decrypt message. What you want to do is add the corresponding RemoveRef after the plugin has gotten the message (see comment later in the file). It might actually work for now. I think the IPC magic under the hood might hold open the shared memory, since you're sending a shared memory handle. But it might cause trouble that you have a HostResource on the other side that's no longer valid. That'll probably bite you later if you start reusing the buffer resources. http://codereview.chromium.org/10909068/diff/2001/ppapi/proxy/ppp_content_dec... ppapi/proxy/ppp_content_decryptor_private_proxy.cc:323: } At this point, you need to tell the renderer to drop the reference it added on the plugin's behalf. Doing ReleaseResource() on the plugin-side ResourceTracker won't work, since what you really want is to release a reference on the renderer/host side. There's an example of this in ppb_image_data_proxy.cc; I'll copy/paste that part here for your reference: """ // The renderer sent us a reference with the message. If the image data was // still cached in our process, the proxy still holds a reference so we can // remove the one the renderer just sent is. If the proxy no longer holds a // reference, we released everything and we should also release the one the // renderer just sent us. dispatcher()->Send(new PpapiHostMsg_PPBCore_ReleaseResource( API_ID_PPB_CORE, old_image_data)); """ http://codereview.chromium.org/10909068/diff/11001/ppapi/proxy/ppp_content_de... File ppapi/proxy/ppp_content_decryptor_private_proxy.cc (left): http://codereview.chromium.org/10909068/diff/11001/ppapi/proxy/ppp_content_de... ppapi/proxy/ppp_content_decryptor_private_proxy.cc:180: const PP_EncryptedBlockInfo* encrypted_block_info) { You'll have to do the same here (and other places you send a Buffer, if any). http://codereview.chromium.org/10909068/diff/11001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10909068/diff/11001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:237: pp::Module::Get()->core()->ReleaseResource(encrypted_buffer.pp_resource()); The C++ interface should take care of reference counting for you correctly. If it doesn't, it's a bug in the C++ wrapper (ppapi/cpp/private/content_decryptor_private.cc). You can either do ReleaseResource there, or add a PassRef constructor to pp::BufferDev (see pp::Resource). http://codereview.chromium.org/10909068/diff/13004/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10909068/diff/13004/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2189: DCHECK(decrypted_block == 0); 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. http://codereview.chromium.org/10909068/diff/13004/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2200: decrypted_block); 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.
http://codereview.chromium.org/10909068/diff/13004/ppapi/proxy/ppp_content_de... File ppapi/proxy/ppp_content_decryptor_private_proxy.cc (left): http://codereview.chromium.org/10909068/diff/13004/ppapi/proxy/ppp_content_de... ppapi/proxy/ppp_content_decryptor_private_proxy.cc:146: core->AddRefResource(encrypted_block); Okay, I think I had it wrong before. I was right that you *do* need this call. That should make the renderer's reference count on the Buffer 2. When the caller of Decrypt leaves, it should the reference count should go to 1. (I don't remember off the top of my head which file that's in, but if it's not decrementing the ref count, it should happen there). The part I'm wrong about is that you shouldn't have to send a ReleaseResource message back. The plugin side will assume that the host holds 1 reference count on the resource. If the plugin-side ref-count goes to zero, then it will send the ReleaseResource message on your behalf: http://code.google.com/searchframe#OAMlx_jo-ck/src/ppapi/shared_impl/resource... So if there's still a leak after checking the caller of Decrypt, then it's probably because the plugin side is holding an extra reference. Note that the ReleaseResource message is asynchronous (plugin->host), so the resource won't go away until that message is received and handled in the renderer.
Possibly one last bit of work to do in PluginInstance::DeliverBlock; hopefully LGTM'able otherwise. http://codereview.chromium.org/10909068/diff/2001/ppapi/proxy/ppp_content_dec... File ppapi/proxy/ppp_content_decryptor_private_proxy.cc (left): http://codereview.chromium.org/10909068/diff/2001/ppapi/proxy/ppp_content_dec... ppapi/proxy/ppp_content_decryptor_private_proxy.cc:146: core->AddRefResource(encrypted_block); On 2012/09/05 17:13:06, dmichael wrote: > tl;dr: Keep this. What you're missing is the corresponding dereference. > > I think you still need this. You want to make sure that the resource lives long > enough that it's still alive when the plugin receives it. Since you're sending > an asynchronous message (as you should), it's reasonably likely that the browser > will be done with this call before the plugin can handle the > PpapiMsg_PPPContentDecryptor_Decrypt message. What you want to do is add the > corresponding RemoveRef after the plugin has gotten the message (see comment > later in the file). > > It might actually work for now. I think the IPC magic under the hood might hold > open the shared memory, since you're sending a shared memory handle. But it > might cause trouble that you have a HostResource on the other side that's no > longer valid. That'll probably bite you later if you start reusing the buffer > resources. Done. http://codereview.chromium.org/10909068/diff/2001/ppapi/proxy/ppp_content_dec... ppapi/proxy/ppp_content_decryptor_private_proxy.cc:323: } On 2012/09/05 17:13:06, dmichael wrote: > At this point, you need to tell the renderer to drop the reference it added on > the plugin's behalf. Doing ReleaseResource() on the plugin-side ResourceTracker > won't work, since what you really want is to release a reference on the > renderer/host side. There's an example of this in ppb_image_data_proxy.cc; I'll > copy/paste that part here for your reference: > """ > // The renderer sent us a reference with the message. If the image data was > // still cached in our process, the proxy still holds a reference so we can > // remove the one the renderer just sent is. If the proxy no longer holds a > // reference, we released everything and we should also release the one the > // renderer just sent us. > dispatcher()->Send(new PpapiHostMsg_PPBCore_ReleaseResource( > API_ID_PPB_CORE, old_image_data)); > """ Done. http://codereview.chromium.org/10909068/diff/11001/ppapi/proxy/ppp_content_de... File ppapi/proxy/ppp_content_decryptor_private_proxy.cc (left): http://codereview.chromium.org/10909068/diff/11001/ppapi/proxy/ppp_content_de... ppapi/proxy/ppp_content_decryptor_private_proxy.cc:180: const PP_EncryptedBlockInfo* encrypted_block_info) { On 2012/09/05 17:13:06, dmichael wrote: > You'll have to do the same here (and other places you send a Buffer, if any). Done. http://codereview.chromium.org/10909068/diff/11001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10909068/diff/11001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:237: pp::Module::Get()->core()->ReleaseResource(encrypted_buffer.pp_resource()); On 2012/09/05 17:13:06, dmichael wrote: > The C++ interface should take care of reference counting for you correctly. If > it doesn't, it's a bug in the C++ wrapper > (ppapi/cpp/private/content_decryptor_private.cc). You can either do > ReleaseResource there, or add a PassRef constructor to pp::BufferDev (see > pp::Resource). Added a PassRef ctor to pp::BufferDev. http://codereview.chromium.org/10909068/diff/13004/ppapi/proxy/ppp_content_de... File ppapi/proxy/ppp_content_decryptor_private_proxy.cc (left): http://codereview.chromium.org/10909068/diff/13004/ppapi/proxy/ppp_content_de... ppapi/proxy/ppp_content_decryptor_private_proxy.cc:146: core->AddRefResource(encrypted_block); On 2012/09/05 18:08:37, dmichael wrote: > Okay, I think I had it wrong before. I was right that you *do* need this call. > That should make the renderer's reference count on the Buffer 2. When the caller > of Decrypt leaves, it should the reference count should go to 1. (I don't > remember off the top of my head which file that's in, but if it's not > decrementing the ref count, it should happen there). > > The part I'm wrong about is that you shouldn't have to send a ReleaseResource > message back. The plugin side will assume that the host holds 1 reference count > on the resource. If the plugin-side ref-count goes to zero, then it will send > the ReleaseResource message on your behalf: > http://code.google.com/searchframe#OAMlx_jo-ck/src/ppapi/shared_impl/resource... > > So if there's still a leak after checking the caller of Decrypt, then it's > probably because the plugin side is holding an extra reference. > > Note that the ReleaseResource message is asynchronous (plugin->host), so the > resource won't go away until that message is received and handled in the > renderer. Done. http://codereview.chromium.org/10909068/diff/13004/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10909068/diff/13004/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2189: DCHECK(decrypted_block == 0); On 2012/09/05 17:13:06, dmichael wrote: > 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. Done. http://codereview.chromium.org/10909068/diff/13004/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2200: decrypted_block); On 2012/09/05 17:13:06, dmichael wrote: > 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. 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.
http://codereview.chromium.org/10909068/diff/10005/ppapi/cpp/dev/buffer_dev.h File ppapi/cpp/dev/buffer_dev.h (right): http://codereview.chromium.org/10909068/diff/10005/ppapi/cpp/dev/buffer_dev.h... ppapi/cpp/dev/buffer_dev.h:26: /// assigned. An additional reference is not taken. How about "No additional reference is taken." http://codereview.chromium.org/10909068/diff/10005/ppapi/proxy/ppp_content_de... File ppapi/proxy/ppp_content_decryptor_private_proxy.cc (right): http://codereview.chromium.org/10909068/diff/10005/ppapi/proxy/ppp_content_de... ppapi/proxy/ppp_content_decryptor_private_proxy.cc:153: if (AddRefResourceForPlugin(dispatcher, encrypted_block) != PP_TRUE) { You could just do if (!AddRefResource...) { right? http://codereview.chromium.org/10909068/diff/10005/ppapi/proxy/ppp_content_de... ppapi/proxy/ppp_content_decryptor_private_proxy.cc:367: API_ID_PPB_CORE, encrypted_buffer.resource)); Sorry, as I mentioned in a more recent comment, I was wrong about this part. The plugin assumes it gets 1 ref count, and it will release it later when the plugin-side refcount goes to 0. (same in Decrypt()) http://codereview.chromium.org/10909068/diff/10007/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10909068/diff/10007/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:256: return buffer.detach(); Why not just return pp::Resource? http://codereview.chromium.org/10909068/diff/10007/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:265: pp::Buffer_Dev message_buffer(MakeBufferResource(key_message.message, This one leaks a reference too as-is (but making MakeBufferResource return pp::Resource should solve it)
I think this is ready for the CQ. I have to run to the DMV, please check the box if it's a LGTM, thanks! http://codereview.chromium.org/10909068/diff/10005/ppapi/cpp/dev/buffer_dev.h File ppapi/cpp/dev/buffer_dev.h (right): http://codereview.chromium.org/10909068/diff/10005/ppapi/cpp/dev/buffer_dev.h... ppapi/cpp/dev/buffer_dev.h:26: /// assigned. An additional reference is not taken. On 2012/09/05 19:46:19, dmichael wrote: > How about "No additional reference is taken." Done. http://codereview.chromium.org/10909068/diff/10005/ppapi/proxy/ppp_content_de... File ppapi/proxy/ppp_content_decryptor_private_proxy.cc (right): http://codereview.chromium.org/10909068/diff/10005/ppapi/proxy/ppp_content_de... ppapi/proxy/ppp_content_decryptor_private_proxy.cc:153: if (AddRefResourceForPlugin(dispatcher, encrypted_block) != PP_TRUE) { On 2012/09/05 19:46:19, dmichael wrote: > You could just do if (!AddRefResource...) { > right? Done. http://codereview.chromium.org/10909068/diff/10005/ppapi/proxy/ppp_content_de... ppapi/proxy/ppp_content_decryptor_private_proxy.cc:367: API_ID_PPB_CORE, encrypted_buffer.resource)); On 2012/09/05 19:46:19, dmichael wrote: > Sorry, as I mentioned in a more recent comment, I was wrong about this part. The > plugin assumes it gets 1 ref count, and it will release it later when the > plugin-side refcount goes to 0. (same in Decrypt()) Removed. http://codereview.chromium.org/10909068/diff/10007/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10909068/diff/10007/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:256: return buffer.detach(); On 2012/09/05 19:46:19, dmichael wrote: > Why not just return pp::Resource? Done. http://codereview.chromium.org/10909068/diff/10007/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:265: pp::Buffer_Dev message_buffer(MakeBufferResource(key_message.message, On 2012/09/05 19:46:19, dmichael wrote: > This one leaks a reference too as-is (but making MakeBufferResource return > pp::Resource should solve it) Done.
Sorry, I'm just getting "error: old chunk mismatch" when I try to review. Can you re-upload when you get a chance?
Same patchset reuploaded. I saw the chunk mismatch error, too. Not sure what the deal was with that.
One last thing, o/w lgtm https://chromiumcodereview.appspot.com/10909068/diff/10016/webkit/media/crypt... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): https://chromiumcodereview.appspot.com/10909068/diff/10016/webkit/media/crypt... webkit/media/crypto/ppapi/cdm_wrapper.cc:266: key_message.message_size).pp_resource()); Don't call pp_resource; just use the copy constructor. The C++ resource wrappers are designed so that you can copy or pass them around safely. Going to the C version unnecessarily just opens you up to mistakes. https://chromiumcodereview.appspot.com/10909068/diff/10016/webkit/media/crypt... webkit/media/crypto/ppapi/cdm_wrapper.cc:298: output_buffer.data_size).pp_resource()); ditto
Used pp::Buffer_Dev instead of pp::Resource-- would've had to add another Buffer_Dev constructor otherwise. http://codereview.chromium.org/10909068/diff/10016/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10909068/diff/10016/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:266: key_message.message_size).pp_resource()); On 2012/09/06 02:46:52, dmichael wrote: > Don't call pp_resource; just use the copy constructor. The C++ resource wrappers > are designed so that you can copy or pass them around safely. Going to the C > version unnecessarily just opens you up to mistakes. Done. http://codereview.chromium.org/10909068/diff/10016/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:298: output_buffer.data_size).pp_resource()); On 2012/09/06 02:46:52, dmichael wrote: > ditto Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tomfinegan@chromium.org/10909068/14002
Change committed as 155145
Providing context on something I requested. It's fine to address later if that's what we decide. http://codereview.chromium.org/10909068/diff/13004/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10909068/diff/13004/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2189: DCHECK(decrypted_block == 0); On 2012/09/05 19:21:43, tomf wrote: > On 2012/09/05 17:13:06, dmichael wrote: > > 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. > > Done. 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. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
