media: Reject pending CDM promise during destruction.

media: Reject pending CDM promise during destruction.

There are legit cases where a CDM promise could be destructed before
it's fulfilled. For example, a promise could be bound into a callback
and the callback is dropped.

BUG=585356
Committed: https://crrev.com/bce64ffe322ab1730a4aa81e47ab85ceb30e2971
Cr-Commit-Position: refs/heads/master@{#378588}

[xhwang, 2016-02-25 01:09:20]

xhwang@chromium.org changed reviewers:
+ ddorwin@chromium.org, jrummell@chromium.org

[xhwang, 2016-02-25 01:09:20]

Please see the bug for the context.

I still have two InternalCdmPromise to fix. But before I do that I'd like to get
early feedback on the general approach.

PTAL

[jrummell, 2016-02-25 02:30:03]

https://codereview.chromium.org/1729063003/diff/1/media/base/cdm_callback_pro...
File media/base/cdm_callback_promise.cc (right):

https://codereview.chromium.org/1729063003/diff/1/media/base/cdm_callback_pro...
media/base/cdm_callback_promise.cc:34: MarkPromiseSettled();
This call sets a flag to indicate that the promise is settled. There is the
DCHECK in CdmPromiseTemplate() to make sure it is settled upon destruction. Can
you not simply make the change there, so it's only in one place?

[xhwang, 2016-02-25 07:49:21]

https://codereview.chromium.org/1729063003/diff/1/media/base/cdm_callback_pro...
File media/base/cdm_callback_promise.cc (right):

https://codereview.chromium.org/1729063003/diff/1/media/base/cdm_callback_pro...
media/base/cdm_callback_promise.cc:34: MarkPromiseSettled();
On 2016/02/25 02:30:03, jrummell wrote:
> This call sets a flag to indicate that the promise is settled. There is the
> DCHECK in CdmPromiseTemplate() to make sure it is settled upon destruction.
Can
> you not simply make the change there, so it's only in one place?

In CdmPromiseTemplate's destructor, I can't call reject() because it's virtual.
Basically the subclass will be destroyed before the base class, so when we are
at the base class's destructor, it's too late to ask the subclass to do
anything. As a result, e.g. the reject_cb_ here will never be called.

[ddorwin, 2016-02-25 19:14:12]

https://codereview.chromium.org/1729063003/diff/1/media/base/cdm_callback_pro...
File media/base/cdm_callback_promise.cc (right):

https://codereview.chromium.org/1729063003/diff/1/media/base/cdm_callback_pro...
media/base/cdm_callback_promise.cc:24: return;
// Promise has already been settled.
?

Or just add and call IsPromiseSettled(), which would serve as the comment, as
discussed below. This might be less fragile than assuming the CBs are reset in
all implementations (see comment below).

https://codereview.chromium.org/1729063003/diff/1/media/base/cdm_callback_pro...
media/base/cdm_callback_promise.cc:34: MarkPromiseSettled();
Is there still a possibility that specifications/children will not *reset*
(breaking the check above)?

https://codereview.chromium.org/1729063003/diff/1/media/mojo/services/mojo_cd...
File media/mojo/services/mojo_cdm_promise.cc (right):

https://codereview.chromium.org/1729063003/diff/1/media/mojo/services/mojo_cd...
media/mojo/services/mojo_cdm_promise.cc:43: std::string message =
We have the same code in 4 places. Does it make sense to have a
RejectDueToDestruction() in the base class?

[xhwang, 2016-02-25 19:53:11]

comment only

https://codereview.chromium.org/1729063003/diff/1/media/mojo/services/mojo_cd...
File media/mojo/services/mojo_cdm_promise.cc (right):

https://codereview.chromium.org/1729063003/diff/1/media/mojo/services/mojo_cd...
media/mojo/services/mojo_cdm_promise.cc:43: std::string message =
On 2016/02/25 19:14:12, ddorwin wrote:
> We have the same code in 4 places. Does it make sense to have a
> RejectDueToDestruction() in the base class?

reject() is virtual and we have to call the reject() in the subclasses. When we
are in the dtor of the base class it's already too late.

We can share the messages somehow but that doesn't really help too much.

[xhwang, 2016-02-25 19:55:44]

On 2016/02/25 19:53:11, xhwang wrote:
> comment only
> 
>
https://codereview.chromium.org/1729063003/diff/1/media/mojo/services/mojo_cd...
> File media/mojo/services/mojo_cdm_promise.cc (right):
> 
>
https://codereview.chromium.org/1729063003/diff/1/media/mojo/services/mojo_cd...
> media/mojo/services/mojo_cdm_promise.cc:43: std::string message =
> On 2016/02/25 19:14:12, ddorwin wrote:
> > We have the same code in 4 places. Does it make sense to have a
> > RejectDueToDestruction() in the base class?
> 
> reject() is virtual and we have to call the reject() in the subclasses. When
we
> are in the dtor of the base class it's already too late.
> 
> We can share the messages somehow but that doesn't really help too much.

Sorry I missed the point. RejectDueToDestruction() should work and will do.

[xhwang, 2016-02-26 06:10:33]

PTAL again!

[ddorwin, 2016-02-26 17:29:28]

lgtm

https://chromiumcodereview.appspot.com/1729063003/diff/20001/media/base/cdm_p...
File media/base/cdm_promise.h (right):

https://chromiumcodereview.appspot.com/1729063003/diff/20001/media/base/cdm_p...
media/base/cdm_promise.h:112: void RejectPromiseOnDestruction() {
// Must be called by the concrete destructor if !IsPromiseSettled().

You might also comment why this has to be called rather than just implemented in
the destructor.

https://chromiumcodereview.appspot.com/1729063003/diff/20001/third_party/WebK...
File third_party/WebKit/public/platform/WebContentDecryptionModuleResult.h
(right):

https://chromiumcodereview.appspot.com/1729063003/diff/20001/third_party/WebK...
third_party/WebKit/public/platform/WebContentDecryptionModuleResult.h:48: //
Returns whether the operation has been completed (success or fail).
nit: failure

[jrummell, 2016-02-26 18:55:05]

lgtm

[xhwang, 2016-02-26 20:48:20]

rebase only

[xhwang, 2016-02-26 20:52:13]

comments addressed

[xhwang, 2016-02-26 20:52:21]

https://chromiumcodereview.appspot.com/1729063003/diff/20001/media/base/cdm_p...
File media/base/cdm_promise.h (right):

https://chromiumcodereview.appspot.com/1729063003/diff/20001/media/base/cdm_p...
media/base/cdm_promise.h:112: void RejectPromiseOnDestruction() {
On 2016/02/26 17:29:28, ddorwin wrote:
> // Must be called by the concrete destructor if !IsPromiseSettled().
> 
> You might also comment why this has to be called rather than just implemented
in
> the destructor.

Done.

https://chromiumcodereview.appspot.com/1729063003/diff/20001/third_party/WebK...
File third_party/WebKit/public/platform/WebContentDecryptionModuleResult.h
(right):

https://chromiumcodereview.appspot.com/1729063003/diff/20001/third_party/WebK...
third_party/WebKit/public/platform/WebContentDecryptionModuleResult.h:48: //
Returns whether the operation has been completed (success or fail).
On 2016/02/26 17:29:28, ddorwin wrote:
> nit: failure

Done.

[xhwang, 2016-02-26 20:55:37]

xhwang@chromium.org changed reviewers:
+ halliwell@chromium.org

[xhwang, 2016-02-26 20:55:37]

halliwell@chromium.org: Please OWNERS review trivial changes in chromecast/

haraken@chromium.org: Please OWNERS review trivial changes in third_party/Webkit

[halliwell, 2016-02-26 21:10:31]

On 2016/02/26 20:55:37, xhwang wrote:
> mailto:halliwell@chromium.org: Please OWNERS review trivial changes in
chromecast/
> 
> mailto:haraken@chromium.org: Please OWNERS review trivial changes in
third_party/Webkit

chromecast/ lgtm

[xhwang, 2016-02-27 03:58:23]

xhwang@chromium.org changed reviewers:
+ haraken@chromium.org

[xhwang, 2016-02-27 03:58:24]

haraken@chromium.org: Please OWNERS review trivial changes in
third_party/Webkit

[haraken, 2016-02-27 04:11:28]

WebKit LGTM but you'll need to get an LG from a public API owner.

[xhwang, 2016-02-27 04:22:45]

xhwang@chromium.org changed reviewers:
+ pfeldman@chromium.org

[xhwang, 2016-02-27 04:22:46]

pfeldman@chromium.org: Please OWNERS review changes in
third_party/WebKit/public/platform/WebContentDecryptionModuleResult.h

[xhwang, 2016-03-01 16:47:54]

pfeldman: kindly ping!

[pfeldman, 2016-03-01 18:34:31]

https://chromiumcodereview.appspot.com/1729063003/diff/60001/third_party/WebK...
File third_party/WebKit/public/platform/WebContentDecryptionModuleResult.h
(right):

https://chromiumcodereview.appspot.com/1729063003/diff/60001/third_party/WebK...
third_party/WebKit/public/platform/WebContentDecryptionModuleResult.h:49:
BLINK_PLATFORM_EXPORT bool isCompleted();
It seems counter-intuitive that you query for completion here, while you report
completion below. Which side is in control? Would it make sense to file
completion upon client destruction instead?

[xhwang, 2016-03-01 18:51:06]

https://chromiumcodereview.appspot.com/1729063003/diff/60001/third_party/WebK...
File third_party/WebKit/public/platform/WebContentDecryptionModuleResult.h
(right):

https://chromiumcodereview.appspot.com/1729063003/diff/60001/third_party/WebK...
third_party/WebKit/public/platform/WebContentDecryptionModuleResult.h:49:
BLINK_PLATFORM_EXPORT bool isCompleted();
On 2016/03/01 18:34:31, pfeldman wrote:
> Would it make sense to file
> completion upon client destruction instead?

This is exactly what this CL is doing.

isCompleted() is added only for a sanity dcheck (see the change in
media/blink/cdm_result_promise.h). If you feel that check is redundant, I can
totally drop this change here.

[pfeldman, 2016-03-01 18:53:17]

> This is exactly what this CL is doing.
> 
> isCompleted() is added only for a sanity dcheck (see the change in
> media/blink/cdm_result_promise.h). If you feel that check is redundant, I can
> totally drop this change here.

As a part of API, it does add confusion into the control flow understanding. So
I'd rather we did not have it here.

[xhwang, 2016-03-01 20:14:40]

rebase only

[xhwang, 2016-03-01 20:17:54]

comments addressed

[xhwang, 2016-03-01 20:18:31]

On 2016/03/01 18:53:17, pfeldman wrote:
> > This is exactly what this CL is doing.
> > 
> > isCompleted() is added only for a sanity dcheck (see the change in
> > media/blink/cdm_result_promise.h). If you feel that check is redundant, I
can
> > totally drop this change here.
> 
> As a part of API, it does add confusion into the control flow understanding.
So
> I'd rather we did not have it here.

Sure thing! Done.

[xhwang, 2016-03-01 20:18:32]

On 2016/03/01 18:53:17, pfeldman wrote:
> > This is exactly what this CL is doing.
> > 
> > isCompleted() is added only for a sanity dcheck (see the change in
> > media/blink/cdm_result_promise.h). If you feel that check is redundant, I
can
> > totally drop this change here.
> 
> As a part of API, it does add confusion into the control flow understanding.
So
> I'd rather we did not have it here.

Sure thing! Done.

[xhwang, 2016-03-01 20:19:42]

On 2016/03/01 20:18:32, xhwang wrote:
> On 2016/03/01 18:53:17, pfeldman wrote:
> > > This is exactly what this CL is doing.
> > > 
> > > isCompleted() is added only for a sanity dcheck (see the change in
> > > media/blink/cdm_result_promise.h). If you feel that check is redundant, I
> can
> > > totally drop this change here.
> > 
> > As a part of API, it does add confusion into the control flow understanding.
> So
> > I'd rather we did not have it here.
> 
> Sure thing! Done.

pfeldman: As a result I don't need your OWNERS approval anymore. I'll go ahead
and land this after all try bots are happy. Let me know if you still have any
concerns.

[xhwang, 2016-03-01 20:19:51]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-01 20:20:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1729063003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1729063003/100001

[pfeldman, 2016-03-01 20:31:35]

On 2016/03/01 20:20:37, commit-bot: I haz the power wrote:
> Dry run: CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1729063003/100001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1729063003/100001

no concerns, thanks!

[xhwang, 2016-03-01 22:40:10]

The CQ bit was unchecked by xhwang@chromium.org

[xhwang, 2016-03-01 22:40:14]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2016-03-01 22:40:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
jrummell@chromium.org, ddorwin@chromium.org, haraken@chromium.org,
halliwell@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1729063003/#ps100001 (title: "comments
addressed")

[commit-bot: I haz the power, 2016-03-01 22:40:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1729063003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1729063003/100001

[commit-bot: I haz the power, 2016-03-01 22:58:08]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-03-01 22:59:27]

Description was changed from

==========
media: Reject pending CDM promise during destruction.

There are legit cases where a CDM promise could be destructed before
it's fulfilled. For example, a promise could be bound into a callback
and the callback is dropped.

BUG=585356
==========

to

==========
media: Reject pending CDM promise during destruction.

There are legit cases where a CDM promise could be destructed before
it's fulfilled. For example, a promise could be bound into a callback
and the callback is dropped.

BUG=585356

Committed: https://crrev.com/bce64ffe322ab1730a4aa81e47ab85ceb30e2971
Cr-Commit-Position: refs/heads/master@{#378588}
==========

[commit-bot: I haz the power, 2016-03-01 22:59:29]

Patchset 6 (id:??) landed as
https://crrev.com/bce64ffe322ab1730a4aa81e47ab85ceb30e2971
Cr-Commit-Position: refs/heads/master@{#378588}