xhwang mentioned this possibly-related bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=482222

+jrummell from EME side and haraken/bashi from v8 side.

haraken/bashi: Do you have any idea what could be causing a crash in V8ObjectConstructor?

+yhirano who I'm told is expert on Blink's promise API.

I managed to get a log from the crash.  Timing-wise, it looks suspiciously close to MediaKeySession destruction:

03-25 14:12:48.038 I/cast_shell(  872): MediaKeySession(0x55e037a8)::~MediaKeySession
03-25 14:12:48.038 I/cast_shell(  872): MediaKeySession(0x55e03908)::~MediaKeySession
03-25 14:12:48.038 I/cast_shell(  872): MediaKeySession(0x55e0ba78)::~MediaKeySession
03-25 14:12:48.048 I/cast_shell(  872): [873:896:WARNING:crash_handler_host_linux.cc(293)] Could not translate tid - assuming crashing thread is thread group leader; syscall_supported=1


Does that suggest anything?
Does anyone have ideas for extra checks or logging that I could insert to try and narrow this down further?

From the crash, the toV8() call is happening when we try to create the V8 object for a DOMException (reject(DOMException::create(code, errorMessage));).

re #3, the promise is disconnected from the MediaKeySession object, as they have different lifetimes. But they do share the same ExecutionContext.

I can't tell much more from the stack trace (I can't figure out what render_thread_impl.cc:1016 has to do with the promise -- in ToT it's in Send(IPC::Message* msg) but maybe it's part of Shutdown() in m49?). If it is happening during Shutdown(), then any pending promises should be cleaned up (i.e. rejected) on destruction (e.g. see CdmPromiseAdapter::Clear()). It's possible that the reject() happens while blink is in the middle of deleting objects, and it's not handled properly.

Is this happening in a worker, or in the main thread?

Issue 524113 may be related.

yhirano: this is on the main thread.

jrummell: render_thread_impl.cc:1016 is in Send also for our source (it's the line that calls ChildThreadImpl::Send).  These stack traces aren't always perfect though.

However, I don't think it's happening in Shutdown.  The repro case is a page that plays back 2 videos with 2 different CDMs.  The crash always happens when the first video ends, as part of destroying the old key session.

When you say "while blink is in the middle of deleting objects", do you mean that could be happening on another thread?  I could go symbolize the other threads to see if anything relevant is happening there.

Attached callstacks for all threads.

Deleted: callstack.txt 16.4 KB

callstack.txt 16.4 KB View Download

It is indeed a script forbidden scope violation:

03-28 14:20:55.236 I/cast_shell(  784): ASSERTION FAILED: !ScriptForbiddenScope::isScriptForbidden()
03-28 14:20:55.246 I/cast_shell(  784): ../../third_party/WebKit/Source/bindings/core/v8/V8RecursionScope.h(99) : blink::V8RecursionScope::MicrotaskSuppression::MicrotaskSuppression(v8::Isolate*)

Thanks for link to related bug yhirano - that reminded me that we have blink assertions disabled because of binary size restrictions on Chromecast.  I was able to enable them locally at least and got the assertion.

I'm not sure what higher-level thing has blocked script.

jrummell / xhwang: I tracked this down a little further, need more help now!

I put some extra logging into script forbidden scoped entry/exit points; the script forbidden scope appears to be entered in ThreadState::eagerSweep:

03-28 16:19:04.219 I/cast_shell( 2580): LJH Enter ../../third_party/WebKit/Source/platform/heap/ThreadState.cpp:1078
03-28 16:19:04.229 I/cast_shell( 2580): MediaKeySession(0x4988beb8)::~MediaKeySession
03-28 16:19:04.239 I/cast_shell( 2580): ASSERTION FAILED: !ScriptForbiddenScope::isScriptForbidden()
03-28 16:19:04.239 I/cast_shell( 2580): ../../third_party/WebKit/Source/bindings/core/v8/V8RecursionScope.h(99) : blink::V8RecursionScope::MicrotaskSuppressio\
n::MicrotaskSuppression(v8::Isolate*)

Let me know your thoughts.

In chromium the promise would only be created due to a request from JavaScript. Does Cast create the MediaKeySession differently? Even so, I would hope that the creation of the promise would have failed due to isScriptForbidden(), rather than wait until the promise is fulfilled soem time later.

eagerSweep sounds like Oilpan GC.

yhirano / haraken: If #10 is correct, why is scripts forbidden when running GC?

I don't think we do anything Cast-specific; we are using the browser-side CDM, so all our CDM code is in the browser process.  And even there we follow the standard process.

> #11

In Oilpan finalization step, you cannot access on-heap objects other than |this|. ScriptPromiseResolver is an on-heap class so at least you cannot call its methods.

jrummell: I don't understand your comment #11 about why creation of the promise would have failed.  As far as I understand it, this is the sequence of operations:

* page calls 'close' on the MediaKeySession, promise is created
* close operation is happening, but in the browser process and very slow; 8 seconds go by
* eagerSweep entered, script is forbidden
* MediaKeySession object is destroyed as part of GC sweep
* This calls CdmPromiseAdapter::Clear, which rejects the 'close' promise with 'Operation aborted'
* reject hits the script forbidden assert

Does that make sense?  What's the right way to fix this?

Just OOC, do you know why "close" is so slow in the browser process? Is it in some common code, or in ChromeCast specific code?

It's in hardware vendor code; there are specific performance issues with the original Chromecast hardware security functions.  We're working with them on some speedups, but it will always be fairly slow.

Thanks! This exposes some issue in EME/V8 which we need to fix :)

So on those devices, you'll need to wait >8 second before you can cast a second video? (Assuming you'll wait for the previous session to be completely closed.)

Correct.  You can notice that video switching is much slower on the older hardware.  Although, apps can hide this a bit by overlapping it with other work including network fetches, so it's not quite as bad as it sounds.

Do you have any thoughts on how to fix this?  Our release candidate build is Thursday, it would be great if we could squeeze this in ... :)

I am not really familiar with this code. jrummell@ has the best knowledge on this from our side. And probably we need more help from V8 people.

yhirano: Do you have any suggestions moving forward given the tight schedule from the Cast side?

On the blink side the MediaKeySession object and the promise object have no connection (once the promise is returned to JavaScript and MediaKeySession has asynchronously called the appropriate method on the Chromium side). Both are kept alive as long as JavaScript has a reference to the respective object.

On the Chromium side, we can't pass promises across pepper, so they are saved in CdmPromiseAdapter (and pass an int32 reference across pepper). Whenever this object is destroyed (normally due to failures in the CDM), any pending promises are rejected.

So it appears that the CDM is taking a long time to process the Close() request, and in the meantime all the EME objects on the blink side are cleaned up. This results in the Chromium objects getting cleaned up, which triggers the promise.reject() call.

yhirano: It's been a while since I touched the Oilpan code. Is there a way in ContentDecryptionModuleResultPromise (basically the blink object representing the promise) to detect this case and handle it? I thought the first lines of ScriptPromiseResolver::resolveOrReject() was the code to detect that the promise is no longer needed and return without doing anything, but it appears not to be the case (the crash looks like it's happening a little later in the method when calling toV8(value, ...)).

Correction: #13 may by incorrect. If you have the resolver with Persistent, it may be OK to call methods.

> * page calls 'close' on the MediaKeySession, promise is created
> * close operation is happening, but in the browser process and very slow; 8 seconds go by

I'm confused because MediaKeySession::close does nothing other than resolving a promise and setting a boolean flag. It seems it doesn't trigger any operation.

Is it correct that the close operation is cancelled when no one has reference to the MediaKeySession? It's possible that the MediaKeySession should be alive until the close operation succeeds or fails, so I would like to know which is the correct behavior from the EME perspective.

+ddorwin for EME spec question.  I didn't see any mention of rejecting this promise in the text, but I could be wrong.

Not true about MediaKeySession::close: it leads to media::MediaKeys::CloseSession on Chromium side.  On Cast platforms, this runs in browser process.  We subclass MediaKeys and this operation is responsible for releasing underlying hardware resources related to keys.  Once that is complete, we resolve the promise.

>#22

I see, I didn't notice there are two close functions. You are talking about MediaKeySession::close(ScriptState*).

Below are some thoughts on my expectations at a high level. Does that help?

The lifetime of the Blink object and the internal Chromium objects and CDM is somewhat independent. We try to ensure we keep the CDM and other internal objects alive appropriately. Specifically, we need the CDM instance to be alive as long as either the MediaKeys or MediaKeySession Blink objects are alive. This includes a MediaKeys being attached to an HTMLMediaElement.

The CDM is responsible for deciding when a session is closed and running the Session Close algorithm when that happens. (Among other things, this will resolve the |closed| promise, which is independent of promises returned by close(). I would assume that if there are no references to the MediaKeySession, nothing will happen in Blink. However, I don't know how that works if there is an unresolved handler for the |closed| promise.

As for close(), a promise is returned synchronously, and an application could be waiting on it. Thus, it needs to be resolved when the asynchronous steps complete. "Process the close request" is vague, but if the session was not closed when close() was called (there is a check for this), then it should probably wait for the session to be closed. Once that happens, the promise should be resolved.

Note that there is no step in the close() algorithm that can cause the promise to be rejected, so it must be resolved.

More generally, [how] does Blink ensure it doesn't destroy an object for which there is an unresolved promise that was returned by a method on that object? How can an underlying implementation (i.e. in Chromium) ensure the promise is unresolved/rejected if the Blink object it communicates through has been destroyed?

This is particularly problematic in cases where the Chromium object can outlive the Blink objects. In these cases, we may prevent the Blink object from being destroyed, but I don't know whether we do the same for those promises.

>#23
As ddorwin mentioned, there is a "closed" property on MediaKeySession (which returns a promise). This promise is resolved on a "closed" event from the Chromium side. When the MediaKeySession object is destroyed, the corresponding Chromium object is also destroyed, and the event will simply be dropped on the Chromium side.

However, there is also the "close" function, which also returns a promise. This appears to be the source of the crash due to the promise getting rejected due to the related EME objects getting destroyed.

The promise returned by close() is resolved when the close request has been processed, and the closed attribute promise is resolved when the session is actually closed.

>#21
> Is it correct that the close operation is cancelled when no one has reference to the MediaKeySession?
No. The operation is processed asynchronously, so MediaKeySession is no longer involved. It can not be cancelled. 

yhirano: In #21 you mentioned Persistent. Does this imply that changing m_resolver (a ScriptPromiseResolver in ContentDecryptionModuleResultPromise) from Member<> to Persistent<> will avoid the crash?

I'm also curious how the Blink code can know that the promise has been destroyed and is no longer needed (ddorwin's question in #25). I thought the code in ScriptPromiseResolver::resolveOrReject() was set up to do that.

Then you should keep the ScriptPromiseResolver in the ContentDecryptionModuleResultPromise alive, and call the resolve function accordingly. The browser-side async close operation should have a reference to ContentDecryptionModuleResultPromise indirectly, and the close operation should not be aborted because of MediaKeySession object finalization (it could be possible that the close operation has some sort of timeout, though).

> Does this imply that changing m_resolver (a ScriptPromiseResolver in ContentDecryptionModuleResultPromise) from Member<> to Persistent<> will avoid the crash?
No.

>#27
The code does most of what you mention (browser-side async close operation keeps a reference to ContentDecryptionModuleResultPromise) but does reject the promise during MediaKeySession object finalization (if the CDM is still executing the close operation).

I tried to reproduce the crash by modifying the test encrypted-media-async-creation-with-gc.html (use ExternalClearKey, don't hold onto mediaKeySession variable so that it can be gc'd) along with changing AesDecryptor so that Close() didn't resolve the promise. I see the pattern expected:
    // gc() runs
    ~MediaKeySession
    ~ContentDecryptorDelegate // this is the Chromium object that saved the promise
    ContentDecryptionModuleResultPromise::reject()
So the promise gets rejected during the destruction of MediaKeySession, and I don't see the error. It is possible that there aren't a lot of objects for gc() to collect, so gc() is done before reject() gets called. Adding a lot of objects (using the code in gc() in LayoutTests/resources/js-test.js) didn't make a difference.

halliwell: I'll send you a patch to try destroying the promises asynchronously, and you can see if it makes a difference.

Thanks John.  I have a fairly reliable way to reproduce the crash on our hardware, would be happy to test out a patch.

Possible patch to test: https://codereview.chromium.org/1849033002/

>#28

> but does reject the promise during MediaKeySession object finalization (if the CDM is still executing the close operation).

Sorry for asking again, is this a desirable behavior (if it doesn't lead to crash)?

>#31
There are a couple of scenarios where this happens. The original was to handle the case when the CDM crashed (since otherwise JavaScript would wait forever for any promises outstanding, if there were any).

In this case it appears that there is no active user of the EME objects. If there were then a MediaKey object would be connected to a HTMLMediaElement object, and this would be sufficient to keep the ContentDecryptorDelegate from being destroyed (and rejecting the outstanding promises).

My assumption is that JavaScript no longer has any references to any of the EME objects (MediaKeys, MediaKeySession, or any returned promise) either, so rejecting the promise should have no impact (other than now the blink objects used by the promise can be gc'd).

The CL in #30 simply posts a task to reject the promise rather than rejecting it immediately. So the behavior is effectively unchanged.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/23f24a3c67f11e066f65820298c73d438f88da29

commit 23f24a3c67f11e066f65820298c73d438f88da29
Author: jrummell <jrummell@chromium.org>
Date: Fri Apr 08 18:34:11 2016

Destroy the CDM asynchronously

CDM destruction can potentially take a long time, so do this
asynchronously.

When the CDM is destroyed, any unfulfilled promises are rejected.
If this happens during blink garbage collection (nothing refers to the
EME blink objects anymore), blink doesn't like creating new objects
(rejection results in a DOMException being created). So post a task
to delete the CDM asynchronously, which will reject the unfulfilled
promises later (after gc is done).

BUG= 597355 
TEST=existing EME tests still pass

Review URL: https://codereview.chromium.org/1870883003

Cr-Commit-Position: refs/heads/master@{#386130}

[modify] https://crrev.com/23f24a3c67f11e066f65820298c73d438f88da29/media/blink/cdm_session_adapter.cc

Reopening this issue as the fix is causing (new) crashes on chromecast.

[13:13:ERROR:renderer_cdm_manager.cc(20)] RendererCdmManager is owned by RenderFrameImpl and is destroyed only after all ProxyMediaKeys are destroyed and unregistered.

As the previous fix destroyed the CDM asynchronously, the CDM has references to objects owned by RenderFrameImpl, which cause the crash when accessed after ~RenderFrameImpl.

Looks like we'll have to go back to the solution to use the prefinalizer on the blink side.

Looks like doing the destruction in the prefinalizer doesn't fix the issue. I changed ClearKey (in file aes_decryptor.cc) to not resolve the close() promise but save it so that it's destroyed in ~AesDecryptor. I also modified the test media/encrypted-media/encrypted-media-async-creation-with-gc.html so that it doesn't save any of the EME objects globally, allowing gc() to free all the EME objects when it runs. This causes the ASSERTION failure chromecast is seeing.

yhirano@ / haraken@: is this broken or expected behavior? 

ASSERTION FAILED: !ScriptForbiddenScope::isScriptForbidden()
../../third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp(47) : void blink::beforeCallEnteredCallback(v8::Isolate *)

In the callstack below the last #40 is the prefinalizer being called (on MediaKeys), which triggers the destruction of WebContentDecryptionModuleImpl (the corresponding Chromium side object). As it's the last reference to CdmSessionAdapter, it gets destroyed (#32) and destroys AesDecryptor with it (#26). When the promise is destroyed it calls media::CdmPromiseTemplate<>::RejectPromiseOnDestruction() (#19), which ultimately calls ScriptPromiseResolver::reject<>() and that fails due to isScriptForbidden() being true.

#3 0x7ff470bd7c4e blink::beforeCallEnteredCallback()
#4 0x7ff473c21cdd v8::internal::Isolate::FireBeforeCallEnteredCallback()
#5 0x7ff4737c61d3 v8::Function::NewInstance()
#6 0x7ff470be3283 blink::V8ScriptRunner::instantiateObject()
#7 0x7ff470bd0007 blink::V8ObjectConstructor::newInstance()
#8 0x7ff470bd0e2e blink::V8PerContextData::createWrapperFromCacheSlowCase()
#9 0x7ff470bb814f blink::V8PerContextData::createWrapperFromCache()
#10 0x7ff470bb76ff blink::V8DOMWrapper::createWrapper()
#11 0x7ff470b93a0d blink::ScriptWrappable::wrap()
#12 0x7ff4776e9d22 blink::toV8()
#13 0x7ff4776ec958 blink::ScriptPromiseResolver::resolveOrReject<>()
#14 0x7ff4776ec5a2 blink::ScriptPromiseResolver::reject<>()
#15 0x7ff4777b66ae blink::ContentDecryptionModuleResultPromise::reject()
#16 0x7ff4777b68d0 blink::ContentDecryptionModuleResultPromise::completeWithError()
#17 0x7ff480723ae2 blink::WebContentDecryptionModuleResult::completeWithError()
#18 0x7ff478749334 _ZN5media16CdmResultPromiseIJEE6rejectENS_9MediaKeys9ExceptionEjRKSs
#19 0x7ff47873216d _ZN5media18CdmPromiseTemplateIJSsEE26RejectPromiseOnDestructionEv
#20 0x7ff478749241 _ZN5media16CdmResultPromiseIJEED2Ev
#21 0x7ff478749299 _ZN5media16CdmResultPromiseIJEED0Ev
#22 0x7ff478a9209f std::default_delete<>::operator()()
#23 0x7ff478c0f9fc _ZNSt10unique_ptrIN5media18CdmPromiseTemplateIJEEESt14default_deleteIS2_EE5resetEPS2_
#24 0x7ff478c0bb69 _ZNSt10unique_ptrIN5media18CdmPromiseTemplateIJEEESt14default_deleteIS2_EED2Ev
#25 0x7ff478c072be media::AesDecryptor::~AesDecryptor()
#26 0x7ff478c073e9 media::AesDecryptor::~AesDecryptor()
#27 0x7ff478b8d49b media::MediaKeys::DeleteOnCorrectThread()
#28 0x7ff478b8d4c6 media::MediaKeysTraits::Destruct()
#29 0x7ff4786f5bfc base::RefCountedThreadSafe<>::Release()
#30 0x7ff4786f5bb9 scoped_refptr<>::Release()
#31 0x7ff4786f3b3a scoped_refptr<>::~scoped_refptr()
#32 0x7ff4786f1827 media::CdmSessionAdapter::~CdmSessionAdapter()
#33 0x7ff4786f79e7 base::RefCounted<>::Release()
#34 0x7ff4786f7995 scoped_refptr<>::Release()
#35 0x7ff4786f412a scoped_refptr<>::~scoped_refptr()
#36 0x7ff478748451 media::WebContentDecryptionModuleImpl::~WebContentDecryptionModuleImpl()
#37 0x7ff478748489 media::WebContentDecryptionModuleImpl::~WebContentDecryptionModuleImpl()
#38 0x7ff477679ecb WTF::OwnedPtrDeleter<>::deletePtr()
#39 0x7ff47773bdc7 WTF::OwnPtr<>::clear()
#40 0x7ff4777cbae9 blink::MediaKeys::dispose()
#41 0x7ff4777cd1cc blink::MediaKeys::invokePreFinalizer()

jrummell: any progress on this?

Issue 617000 has been merged into this issue.

In issue 617000, we have a related crash report where we are calling into RendererCdmManager during MediaKeys Destroy() which caused a crash. I suspect that at this point, the RenderFrame and the RendererCdmManager has already been destroyed.

FWIW, the callstack doesn't totally make sense. We shouldn't see MediaDrmBridge in the render process at all.

0x96d5b24a	(libchrome.so -renderer_cdm_manager.cc:129 )	content::RendererCdmManager::DestroyCdm
0x96d5ad71	(libchrome.so -proxy_media_keys.cc:189 )	content::ProxyMediaKeys::~ProxyMediaKeys
0x96d5add3	(libchrome.so -proxy_media_keys.cc:192 )	content::ProxyMediaKeys::~ProxyMediaKeys
0x95832559	(libchrome.so -ref_counted.h:184 )	scoped_refptr<media::MediaDrmBridge>::~scoped_refptr
0x96d62ffb	(libchrome.so -tuple:180 )	base::internal::BindState<base::internal::RunnableAdapter<void (*)(scoped_refptr<media::MediaKeys>)>, void(scoped_refptr<media::MediaKeys>), scoped_refptr<media::MediaKeys>&>::Destroy
0x954ce205	(libchrome.so -task_queue_manager.cc:304 )	scheduler::TaskQueueManager::ProcessTaskFromWorkQueue

Users experienced this crash on the following builds:

Android Beta 51.0.2704.77 -  0.51 CPM, 12 reports, 6 clients (signature content::RendererCdmManager::DestroyCdm)

If this update was incorrect, please add "Fracas-Wrong" label to prevent future updates.

- Go/Fracas

Users experienced this crash on the following builds:

Android Beta 51.0.2704.77 -  0.32 CPM, 10 reports, 7 clients (signature content::RendererCdmManager::DestroyCdm)

If this update was incorrect, please add "Fracas-Wrong" label to prevent future updates.

- Go/Fracas

Users experienced this crash on the following builds:

Android Beta 51.0.2704.77 -  0.32 CPM, 10 reports, 7 clients (signature content::RendererCdmManager::DestroyCdm)

If this update was incorrect, please add "Fracas-Wrong" label to prevent future updates.

- Go/Fracas

Users experienced this crash on the following builds:

Android Stable 51.0.2704.81 -  1.02 CPM, 574 reports, 217 clients (signature content::RendererCdmManager::DestroyCdm)

If this update was incorrect, please add "Fracas-Wrong" label to prevent future updates.

- Go/Fracas

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3135b5eec1c39fb213cbf906f81553efa8cbaf67

commit 3135b5eec1c39fb213cbf906f81553efa8cbaf67
Author: jrummell <jrummell@chromium.org>
Date: Thu Jun 09 19:57:00 2016

Change back to destroy the CDM synchronously

Outstanding promises may get rejected due to blink garbage collection
destroying all the remaining EME objects when they are no longer
referenced. The original fix destroyed the CDM asynchronously.
However, that causes problems as the CDM may hold references to
objects owned by RenderFrameImpl, and they may get accessed after
~RenderFrameImpl.

This fix posts a task to reject the promise asynchronously. That way
any outstanding promises that need to be rejected are handled after
gc is done. It affects all EME promise rejections (from Chromium).

Resolving promises is still done synchronously as there may be events
already posted that need to happen only after the promise is resolved.

BUG= 597355 
TEST=existing EME tests still pass

Review-Url: https://codereview.chromium.org/1987883002
Cr-Commit-Position: refs/heads/master@{#398986}

[modify] https://crrev.com/3135b5eec1c39fb213cbf906f81553efa8cbaf67/media/blink/cdm_session_adapter.cc
[modify] https://crrev.com/3135b5eec1c39fb213cbf906f81553efa8cbaf67/third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp
[modify] https://crrev.com/3135b5eec1c39fb213cbf906f81553efa8cbaf67/third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3135b5eec1c39fb213cbf906f81553efa8cbaf67

commit 3135b5eec1c39fb213cbf906f81553efa8cbaf67
Author: jrummell <jrummell@chromium.org>
Date: Thu Jun 09 19:57:00 2016

Change back to destroy the CDM synchronously

Outstanding promises may get rejected due to blink garbage collection
destroying all the remaining EME objects when they are no longer
referenced. The original fix destroyed the CDM asynchronously.
However, that causes problems as the CDM may hold references to
objects owned by RenderFrameImpl, and they may get accessed after
~RenderFrameImpl.

This fix posts a task to reject the promise asynchronously. That way
any outstanding promises that need to be rejected are handled after
gc is done. It affects all EME promise rejections (from Chromium).

Resolving promises is still done synchronously as there may be events
already posted that need to happen only after the promise is resolved.

BUG= 597355 
TEST=existing EME tests still pass

Review-Url: https://codereview.chromium.org/1987883002
Cr-Commit-Position: refs/heads/master@{#398986}

[modify] https://crrev.com/3135b5eec1c39fb213cbf906f81553efa8cbaf67/media/blink/cdm_session_adapter.cc
[modify] https://crrev.com/3135b5eec1c39fb213cbf906f81553efa8cbaf67/third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp
[modify] https://crrev.com/3135b5eec1c39fb213cbf906f81553efa8cbaf67/third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.h

jrummell: Could you please check crash stats to make sure the crash stops in canary, and then merge the fix to M52?

We are also seeing a lot of crashes in M51 (see issue 621104). We should talk to PM to see what we can do there. It seems the easiest solution is to revert the original change in #33.

I checked the crash stats for the signature specified in #40, and the bulk of them (95.6%) happen on 51.0.2704.81. No crashes reported yet on M52 or M53.

Requesting merge into M52.

Your change meets the bar and is auto-approved for M52 (branch: 2743)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4f09e0e74ee9520929b78c7234679d78ca635c64

commit 4f09e0e74ee9520929b78c7234679d78ca635c64
Author: John Rummell <jrummell@chromium.org>
Date: Fri Jun 17 23:50:40 2016

Change back to destroy the CDM synchronously

Outstanding promises may get rejected due to blink garbage collection
destroying all the remaining EME objects when they are no longer
referenced. The original fix destroyed the CDM asynchronously.
However, that causes problems as the CDM may hold references to
objects owned by RenderFrameImpl, and they may get accessed after
~RenderFrameImpl.

This fix posts a task to reject the promise asynchronously. That way
any outstanding promises that need to be rejected are handled after
gc is done. It affects all EME promise rejections (from Chromium).

Resolving promises is still done synchronously as there may be events
already posted that need to happen only after the promise is resolved.

BUG= 597355 
TEST=existing EME tests still pass

Review-Url: https://codereview.chromium.org/1987883002
Cr-Commit-Position: refs/heads/master@{#398986}
(cherry picked from commit 3135b5eec1c39fb213cbf906f81553efa8cbaf67)

Review URL: https://codereview.chromium.org/2078833004 .

Cr-Commit-Position: refs/branch-heads/2743@{#387}
Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939}

[modify] https://crrev.com/4f09e0e74ee9520929b78c7234679d78ca635c64/media/blink/cdm_session_adapter.cc
[modify] https://crrev.com/4f09e0e74ee9520929b78c7234679d78ca635c64/third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp
[modify] https://crrev.com/4f09e0e74ee9520929b78c7234679d78ca635c64/third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.h

Marking this as fixed now that it is merged into M52. According to issue 621104, it doesn't look like there will be another release of M51.

Issue 627737 has been merged into this issue.

Issue 621104 has been merged into this issue.