Reject outstanding promises asynchronously when cleared

Reject outstanding promises asynchronously when cleared

Outstanding promises may get cleared due to blink garbage collection
destroying all the remaining EME objects when they are no longer
referenced. However, blink doesn't like creating new objects during gc
(rejection results in a DOMException being created), so post a task to
reject the promise(s) later.

BUG=597355
TEST=tested on cast

[halliwell, 2016-04-01 04:26:48]

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

[halliwell, 2016-04-01 04:26:49]

Hey John.  Thanks for putting this together.  The first time I ran our test with
this patch, I hit the same script blocked assert, but unfortunately didn't have
symbols to get a callstack.

I then built with symbols and added a couple of bits of logging ... and since
then have been running the test on continuous loop for several hours without
seeing the assert once.  Without your patch, I could repro within 5-10 minutes
(the test takes about 90 seconds).

Couple of possibilities:
(1) I simply messed up the first time round, somehow didn't actually run a build
with your patch in.
(2) There is something else lower-repro that can trigger this assert (doesn't
necessarily have to be EME-related or even a promise resolution ... all I know
is it was a script blocked violation)

I'm pretty sure it's (2), but (1) has to be acknowledged as a possibility :)

Either way, I think it's clear that this patch fixes the crash we were seeing.

Let's catch up tomorrow on next steps: I'd like to land this and cherry-pick
into our release branch if possible (but I don't know what level of readiness
"WIP" indicates in the title here).

[halliwell, 2016-04-01 17:55:55]

On 2016/04/01 04:26:49, halliwell wrote:
> Hey John.  Thanks for putting this together.  The first time I ran our test
with
> this patch, I hit the same script blocked assert, but unfortunately didn't
have
> symbols to get a callstack.
> 
> I then built with symbols and added a couple of bits of logging ... and since
> then have been running the test on continuous loop for several hours without
> seeing the assert once.  Without your patch, I could repro within 5-10 minutes
> (the test takes about 90 seconds).
> 
> Couple of possibilities:
> (1) I simply messed up the first time round, somehow didn't actually run a
build
> with your patch in.
> (2) There is something else lower-repro that can trigger this assert (doesn't
> necessarily have to be EME-related or even a promise resolution ... all I know
> is it was a script blocked violation)
> 
> I'm pretty sure it's (2), but (1) has to be acknowledged as a possibility :)
> 
> Either way, I think it's clear that this patch fixes the crash we were seeing.
> 
> Let's catch up tomorrow on next steps: I'd like to land this and cherry-pick
> into our release branch if possible (but I don't know what level of readiness
> "WIP" indicates in the title here).

Ok, the assert does look like a mistake on my part.

I managed to run tests continuously overnight till now with no further assert
failures.

[jrummell, 2016-04-01 19:53:41]

Description was changed from

==========
WIP: Abort promises asynchronously

Only for testing a fix.

BUG=597355
==========

to

==========
Reject outstanding promises asynchronously when cleared

Outstanding promises may get cleared due to blink garbage collection
destroying all the remaining EME objects when they are no longer
referenced. However, blink doesn't like creating new objects during gc
(rejection results in a DOMException being created), so post a task to
reject the promise(s) later.

BUG=597355
TEST=tested on cast
==========

[jrummell, 2016-04-01 19:54:25]

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

[jrummell, 2016-04-01 19:54:26]

PTAL. This looks like it fixes the cast issue.

[halliwell, 2016-04-01 21:46:09]

On 2016/04/01 19:54:26, jrummell wrote:
> PTAL. This looks like it fixes the cast issue.

lgtm from a 'verified this fixes our bug' perspective :)

[xhwang, 2016-04-01 22:32:21]

Thanks!

I few thoughts:
- if any class stores the promise and GC triggers the class to be destroyed, we
may end up with the same issue because the promise will be destroyed which will
trigger the Result be rejected.
- It seems odd that we solve a Blink GC issue in media/base. CdmPromiseAdapter
is used in a lot of places, e.g. in the browser process.
- In media/base, we don't always assume we have a task runner. If some class
needs a TaskRunner, usually it's passed in.

With these, I wonder whether it's possible to solve this in Blink. For example,
if it's possible, we check whether we are in GC; if so, post in Blink.

[jrummell, 2016-04-08 17:56:01]

Closing this in favor of https://codereview.chromium.org/1870883003/ (which
frees the media::MediaKeys object asynchronously).