Handle plugin instance crash in ContentDecryptorDelegate.

Handle plugin instance crash in ContentDecryptorDelegate.

BUG=chrome-os-partner:17801
TEST=Kill Widevine CDM process and we get decode error.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243762

[xhwang, 2013-12-28 01:00:27]

PTAL

https://codereview.chromium.org/116443009/diff/1/content/renderer/pepper/cont...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/116443009/diff/1/content/renderer/pepper/cont...
content/renderer/pepper/content_decryptor_delegate.cc:330: }
We have too many duplicate code about reqeust_id and callbacks. In another CL,
I'll clean them up a bit.

https://codereview.chromium.org/116443009/diff/1/content/renderer/pepper/cont...
File content/renderer/pepper/content_decryptor_delegate.h (right):

https://codereview.chromium.org/116443009/diff/1/content/renderer/pepper/cont...
content/renderer/pepper/content_decryptor_delegate.h:190: bool
is_instance_crashed_;
I am debating on this. Probably when the plugin is crashed, we should just
destroy this ContentDecryptorDelegate object. WDYT?

[ddorwin, 2014-01-07 19:28:47]

https://codereview.chromium.org/116443009/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/proxy_decryptor.cc (right):

https://codereview.chromium.org/116443009/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_decryptor.cc:122: return false;
Was the removed line just left over from before we had InitializeCDM?

https://codereview.chromium.org/116443009/diff/1/content/renderer/pepper/cont...
File content/renderer/pepper/content_decryptor_delegate.h (right):

https://codereview.chromium.org/116443009/diff/1/content/renderer/pepper/cont...
content/renderer/pepper/content_decryptor_delegate.h:190: bool
is_instance_crashed_;
On 2013/12/28 01:00:28, xhwang wrote:
> I am debating on this. Probably when the plugin is crashed, we should just
> destroy this ContentDecryptorDelegate object. WDYT?

You can't simply destroy this object without informing PpapiDecryptor and higher
layers to stop calling Decrypt(), etc. PpapiDecryptor assumes there is always a
delegate, so you'd have to add such checks there anyway. Higher-level code might
already have checks for decryptor_ != NULL, but it's also more likely that there
are assumptions about the decryptor/decoder not changing.

To fully clean up, we would tell PpapiDecryptor, which would clean up the WHP as
well. (Such a change would also need the delegate != NULL checks mentioned
above.) However, that's a lot of complexity for an unexpected case.

is_instance_crashed_ isn't great, but it's simpler and isolated. Maybe it should
be is_valid_ or something similar that is more generic instead.

Pushing this knowledge into PpapiDecryptor would allow us to eliminate the
(previously always true) bool return values from most of the methods, which is
nice since it removes ambiguity. I don't think I'd go further up the stack than
that, though.

[xhwang, 2014-01-07 20:48:33]

comments only

https://codereview.chromium.org/116443009/diff/1/content/renderer/pepper/cont...
File content/renderer/pepper/content_decryptor_delegate.h (right):

https://codereview.chromium.org/116443009/diff/1/content/renderer/pepper/cont...
content/renderer/pepper/content_decryptor_delegate.h:190: bool
is_instance_crashed_;
On 2014/01/07 19:28:48, ddorwin wrote:
> On 2013/12/28 01:00:28, xhwang wrote:
> > I am debating on this. Probably when the plugin is crashed, we should just
> > destroy this ContentDecryptorDelegate object. WDYT?
> 
> You can't simply destroy this object without informing PpapiDecryptor and
higher
> layers to stop calling Decrypt(), etc. PpapiDecryptor assumes there is always
a
> delegate, so you'd have to add such checks there anyway. Higher-level code
might
> already have checks for decryptor_ != NULL, but it's also more likely that
there
> are assumptions about the decryptor/decoder not changing.
> 
> To fully clean up, we would tell PpapiDecryptor, which would clean up the WHP
as
> well. (Such a change would also need the delegate != NULL checks mentioned
> above.) However, that's a lot of complexity for an unexpected case.
> 
> is_instance_crashed_ isn't great, but it's simpler and isolated. Maybe it
should
> be is_valid_ or something similar that is more generic instead.
> 
> Pushing this knowledge into PpapiDecryptor would allow us to eliminate the
> (previously always true) bool return values from most of the methods, which is
> nice since it removes ambiguity. I don't think I'd go further up the stack
than
> that, though.

Thanks for the comments. In summary, notifying PpapiDecryptor would allow us to
do more cleanups, but that requires more complexity. Since this is only a corner
case, I am inclined to keep |is_valid_| to make this CL simple. WDYT?

BTW, could you take a look at https://codereview.chromium.org/105743010/ first,
which would make this CL simpler? Thanks!

[ddorwin, 2014-01-07 23:59:01]

https://codereview.chromium.org/116443009/diff/1/content/renderer/pepper/cont...
File content/renderer/pepper/content_decryptor_delegate.h (right):

https://codereview.chromium.org/116443009/diff/1/content/renderer/pepper/cont...
content/renderer/pepper/content_decryptor_delegate.h:190: bool
is_instance_crashed_;
On 2014/01/07 20:48:34, xhwang wrote:
> On 2014/01/07 19:28:48, ddorwin wrote:
> > On 2013/12/28 01:00:28, xhwang wrote:
> > > I am debating on this. Probably when the plugin is crashed, we should just
> > > destroy this ContentDecryptorDelegate object. WDYT?
> > 
> > You can't simply destroy this object without informing PpapiDecryptor and
> higher
> > layers to stop calling Decrypt(), etc. PpapiDecryptor assumes there is
always
> a
> > delegate, so you'd have to add such checks there anyway. Higher-level code
> might
> > already have checks for decryptor_ != NULL, but it's also more likely that
> there
> > are assumptions about the decryptor/decoder not changing.
> > 
> > To fully clean up, we would tell PpapiDecryptor, which would clean up the
WHP
> as
> > well. (Such a change would also need the delegate != NULL checks mentioned
> > above.) However, that's a lot of complexity for an unexpected case.
> > 
> > is_instance_crashed_ isn't great, but it's simpler and isolated. Maybe it
> should
> > be is_valid_ or something similar that is more generic instead.
> > 
> > Pushing this knowledge into PpapiDecryptor would allow us to eliminate the
> > (previously always true) bool return values from most of the methods, which
is
> > nice since it removes ambiguity. I don't think I'd go further up the stack
> than
> > that, though.
> 
> Thanks for the comments. In summary, notifying PpapiDecryptor would allow us
to
> do more cleanups, but that requires more complexity. Since this is only a
corner
> case, I am inclined to keep |is_valid_| to make this CL simple. WDYT?
> 
> BTW, could you take a look at https://codereview.chromium.org/105743010/
first,
> which would make this CL simpler? Thanks!

Agreed and done.

[xhwang, 2014-01-08 17:49:26]

PTAL again

[ddorwin, 2014-01-08 18:05:46]

lgtm

[xhwang, 2014-01-08 18:25:18]

dmichael@: Please do an OWNERS review!

[dmichael (off chromium), 2014-01-08 20:15:39]

On 2014/01/07 19:28:47, ddorwin wrote:
>
https://codereview.chromium.org/116443009/diff/1/content/renderer/media/crypt...
> File content/renderer/media/crypto/proxy_decryptor.cc (right):
> 
>
https://codereview.chromium.org/116443009/diff/1/content/renderer/media/crypt...
> content/renderer/media/crypto/proxy_decryptor.cc:122: return false;
> Was the removed line just left over from before we had InitializeCDM?
> 
>
https://codereview.chromium.org/116443009/diff/1/content/renderer/pepper/cont...
> File content/renderer/pepper/content_decryptor_delegate.h (right):
> 
>
https://codereview.chromium.org/116443009/diff/1/content/renderer/pepper/cont...
> content/renderer/pepper/content_decryptor_delegate.h:190: bool
> is_instance_crashed_;
> On 2013/12/28 01:00:28, xhwang wrote:
> > I am debating on this. Probably when the plugin is crashed, we should just
> > destroy this ContentDecryptorDelegate object. WDYT?
> 
> You can't simply destroy this object without informing PpapiDecryptor and
higher
> layers to stop calling Decrypt(), etc. PpapiDecryptor assumes there is always
a
> delegate, so you'd have to add such checks there anyway. Higher-level code
might
> already have checks for decryptor_ != NULL, but it's also more likely that
there
> are assumptions about the decryptor/decoder not changing.
> 
> To fully clean up, we would tell PpapiDecryptor, which would clean up the WHP
as
> well. (Such a change would also need the delegate != NULL checks mentioned
> above.) However, that's a lot of complexity for an unexpected case.
Shouldn't we want PpapiDecryptor to know that the plugin crashed? It seems like
useful information, rather than it just finding out that suddenly Decrypt calls
are failing. It could also then drop its reference on the
PepperPluginInstance...  we should generally avoid making the
PepperPluginInstanceImpl live unnaturally long, I think.

> 
> is_instance_crashed_ isn't great, but it's simpler and isolated. Maybe it
should
> be is_valid_ or something similar that is more generic instead.
> 
> Pushing this knowledge into PpapiDecryptor would allow us to eliminate the
> (previously always true) bool return values from most of the methods, which is
> nice since it removes ambiguity. I don't think I'd go further up the stack
than
> that, though.

[dmichael (off chromium), 2014-01-08 20:15:59]

https://codereview.chromium.org/116443009/diff/100001/content/renderer/pepper...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/116443009/diff/100001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.cc:1065: void
ContentDecryptorDelegate::CancelAllPendingCallbacks() {
The meaning here is a little different from what I expected from the word
"Cancel"; I expected the callbacks were not run or something (e.g., see
CancellableCallback). Really, they're being run and cleared. Maybe another name
would help?

InformAllCallbacksOfError()?

Not sure if that's better or not.

[xhwang, 2014-01-08 21:18:51]

comments addressed

[xhwang, 2014-01-08 23:12:50]

comments addressed

[xhwang, 2014-01-08 23:19:13]

dmichael: I think I addressed your comments. PTAL!

ddorwin: Now we notify the plugin error to PpapiDecryptor and added tests. PTAL
again.

https://codereview.chromium.org/116443009/diff/100001/content/renderer/pepper...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/116443009/diff/100001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.cc:1065: void
ContentDecryptorDelegate::CancelAllPendingCallbacks() {
On 2014/01/08 20:15:59, dmichael wrote:
> The meaning here is a little different from what I expected from the word
> "Cancel"; I expected the callbacks were not run or something (e.g., see
> CancellableCallback). Really, they're being run and cleared. Maybe another
name
> would help?
> 
> InformAllCallbacksOfError()?
> 
> Not sure if that's better or not.

Done.

https://codereview.chromium.org/116443009/diff/290001/content/renderer/media/...
File content/renderer/media/crypto/ppapi_decryptor.cc (right):

https://codereview.chromium.org/116443009/diff/290001/content/renderer/media/...
content/renderer/media/crypto/ppapi_decryptor.cc:124: session_id, response,
response_length)) {
This is clang-format style:)

https://codereview.chromium.org/116443009/diff/290001/content/renderer/media/...
content/renderer/media/crypto/ppapi_decryptor.cc:194: if (!plugin_cdm_delegate_
||
If I add a lock I can check this before the post (on media thread). But again,
this is a corner failure case and I think the performance impact can be ignored.

[dmichael (off chromium), 2014-01-08 23:30:47]

Thanks for doing that! pepper lgtm

[ddorwin, 2014-01-08 23:40:31]

lgtm with comments

https://codereview.chromium.org/116443009/diff/290001/chrome/browser/media/en...
File chrome/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/116443009/diff/290001/chrome/browser/media/en...
chrome/browser/media/encrypted_media_browsertest.cc:412: kError);
General comment for our browsertests: It would be nice to set MediaError-3 or
something in the title and be able to log and compare against that.

https://codereview.chromium.org/116443009/diff/290001/content/renderer/media/...
File content/renderer/media/crypto/ppapi_decryptor.cc (right):

https://codereview.chromium.org/116443009/diff/290001/content/renderer/media/...
content/renderer/media/crypto/ppapi_decryptor.cc:372:
base::ResetAndReturn(&destroy_plugin_cb_).Run();
This probably exposes/tests new paths related to WHP. :)

https://codereview.chromium.org/116443009/diff/290001/content/renderer/media/...
File content/renderer/media/crypto/ppapi_decryptor.h (right):

https://codereview.chromium.org/116443009/diff/290001/content/renderer/media/...
content/renderer/media/crypto/ppapi_decryptor.h:102: // After this call,
|plugin_cdm_delegate_| should not be used.
Between these two, maybe explicitly note that plugin errors are terminal.

https://codereview.chromium.org/116443009/diff/290001/content/renderer/pepper...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/116443009/diff/290001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.cc:280:
plugin_decryption_interface_->Initialize(
I think one/the reason Initialize() was separate before was to avoid doing
something complex like this IPC within the PpapiDecryptor constructor. I'm not
sure how much it really matters, though.

https://codereview.chromium.org/116443009/diff/290001/content/renderer/pepper...
File content/renderer/pepper/content_decryptor_delegate.h (right):

https://codereview.chromium.org/116443009/diff/290001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.h:43: void Initialize(const
std::string& key_system,
Should we document in this public interface that plugin errors are terminal and
this object should not be used if plugin_error_cb is called? Alternatively/in
addition, maybe something stronger than "error". fatal_plugin_error_cb?

https://codereview.chromium.org/116443009/diff/290001/media/cdm/ppapi/externa...
File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right):

https://codereview.chromium.org/116443009/diff/290001/media/cdm/ppapi/externa...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:400: CHECK(false);
Is crash reporting disabled during browser_tests? We don't want to spam the
server.

[xhwang, 2014-01-09 01:18:18]

rebase only

[xhwang, 2014-01-09 01:58:27]

comments addressed

[xhwang, 2014-01-09 01:58:49]

https://codereview.chromium.org/116443009/diff/290001/chrome/browser/media/en...
File chrome/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/116443009/diff/290001/chrome/browser/media/en...
chrome/browser/media/encrypted_media_browsertest.cc:412: kError);
On 2014/01/08 23:40:32, ddorwin wrote:
> General comment for our browsertests: It would be nice to set MediaError-3 or
> something in the title and be able to log and compare against that.

Added TODO.

https://codereview.chromium.org/116443009/diff/290001/content/renderer/media/...
File content/renderer/media/crypto/ppapi_decryptor.cc (right):

https://codereview.chromium.org/116443009/diff/290001/content/renderer/media/...
content/renderer/media/crypto/ppapi_decryptor.cc:372:
base::ResetAndReturn(&destroy_plugin_cb_).Run();
On 2014/01/08 23:40:32, ddorwin wrote:
> This probably exposes/tests new paths related to WHP. :)

:) Let's see.

https://codereview.chromium.org/116443009/diff/290001/content/renderer/media/...
File content/renderer/media/crypto/ppapi_decryptor.h (right):

https://codereview.chromium.org/116443009/diff/290001/content/renderer/media/...
content/renderer/media/crypto/ppapi_decryptor.h:102: // After this call,
|plugin_cdm_delegate_| should not be used.
On 2014/01/08 23:40:32, ddorwin wrote:
> Between these two, maybe explicitly note that plugin errors are terminal.

Done.

https://codereview.chromium.org/116443009/diff/290001/content/renderer/pepper...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/116443009/diff/290001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.cc:280:
plugin_decryption_interface_->Initialize(
On 2014/01/08 23:40:32, ddorwin wrote:
> I think one/the reason Initialize() was separate before was to avoid doing
> something complex like this IPC within the PpapiDecryptor constructor. I'm not
> sure how much it really matters, though.

If we really want to avoid IPC in ctor, we can add PpapiDecryptor::Init(), but
it seems it's not necessary. So let's land this and see.

https://codereview.chromium.org/116443009/diff/290001/content/renderer/pepper...
File content/renderer/pepper/content_decryptor_delegate.h (right):

https://codereview.chromium.org/116443009/diff/290001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.h:43: void Initialize(const
std::string& key_system,
On 2014/01/08 23:40:32, ddorwin wrote:
> Should we document in this public interface that plugin errors are terminal
and
> this object should not be used if plugin_error_cb is called? Alternatively/in
> addition, maybe something stronger than "error". fatal_plugin_error_cb?

Done.

https://codereview.chromium.org/116443009/diff/290001/media/cdm/ppapi/externa...
File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right):

https://codereview.chromium.org/116443009/diff/290001/media/cdm/ppapi/externa...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:400: CHECK(false);
On 2014/01/08 23:40:32, ddorwin wrote:
> Is crash reporting disabled during browser_tests? We don't want to spam the
> server.

Yes, crash in tests are not uploaded. (I try to understand why, but it's
complicated......)

[ddorwin, 2014-01-09 02:04:31]

lgtm % comment

https://codereview.chromium.org/116443009/diff/510001/chrome/browser/media/me...
File chrome/browser/media/media_browsertest.h (right):

https://codereview.chromium.org/116443009/diff/510001/chrome/browser/media/me...
chrome/browser/media/media_browsertest.h:26: // TODO(xhwang): Report detailed
errors, e.g. "ERROR-3".
s/errors/MediaErrors/
Otherwise, it's not clear that 3 is one of the defined error values. The example
should probably have "Media" in it too.

[commit-bot: I haz the power, 2014-01-09 02:05:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/116443009/510001

[commit-bot: I haz the power, 2014-01-09 04:23:32]

Change committed as 243762