Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1987883002:
Change back to destroy the CDM synchronously (Closed)
|
Created:
4 years, 7 months ago by jrummell Modified:
4 years, 6 months ago CC:
chromium-reviews, blink-reviews, feature-media-reviews_chromium.org, mlamouri+watch-blink_chromium.org, eric.carlson_apple.com Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange 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
Committed: https://crrev.com/3135b5eec1c39fb213cbf906f81553efa8cbaf67
Cr-Commit-Position: refs/heads/master@{#398986}
Patch Set 1 #Patch Set 2 : reject asynchronously #
Total comments: 6
Patch Set 3 : nits #
Messages
Total messages: 29 (9 generated)
jrummell@chromium.org changed reviewers: + halliwell@chromium.org, haraken@chromium.org, xhwang@chromium.org, yhirano@chromium.org
PTAL. MediaKeySession is already using a prefinalizer, so this just adds one to MediaKeys (and converts CDM destruction back to synchronous).
lgtm but I defer to blink owners for a closer look. halliwell: Could you please give it a try to see whether it solves the old bug we had?
On 2016/05/17 23:18:08, xhwang wrote: > lgtm but I defer to blink owners for a closer look. > > halliwell: Could you please give it a try to see whether it solves the old bug > we had? Yep, will test tomorrow morning (WFH today and don't have access to the right hardware).
LGTM
On 2016/05/17 23:52:46, haraken wrote: > LGTM Haven't forgotten about this: sadly struggling to reproduce the original bug ... will keep trying for a bit :)
yhirano@google.com changed reviewers: + yhirano@google.com
lgtm
yhirano@chromium.org changed reviewers: - yhirano@google.com
lgtm
On 2016/05/19 02:12:53, yhirano wrote: > lgtm Sorry for delay, I couldn't figure out my previous repro at all. However, today I managed to write a test case from scratch that reproduces it 100% in a couple of seconds on our hardware. And unfortunately, not lgtm - I still get the ScriptForbiddenScope assertion :( jrummell: I presume the ideal thing would be to get my test case into a form that can reproduce the bug on x86. I suspect there's nothing special about our hardware besides the fact that it's slow. I wonder if we could have a CDM just for testing that put in a bit of artificial delay in its handling of events?
On 2016/05/23 23:07:03, halliwell wrote: > On 2016/05/19 02:12:53, yhirano wrote: > > lgtm > > Sorry for delay, I couldn't figure out my previous repro at all. > > However, today I managed to write a test case from scratch that reproduces it > 100% in a couple of seconds on our hardware. And unfortunately, not lgtm - I > still get the ScriptForbiddenScope assertion :( > > jrummell: I presume the ideal thing would be to get my test case into a form > that can reproduce the bug on x86. I suspect there's nothing special about our > hardware besides the fact that it's slow. I wonder if we could have a CDM just > for testing that put in a bit of artificial delay in its handling of events? Will it be sufficient to write a test where the CDM will hold a promise and never fulfill it? This should be doable in content_browsertests using external clear key by using a special key system name. halliwell: Could you post the stack trace of the assertion to help investigate?
I did try this on Chromium. The problem is that WebPlugin is destroyed asynchronously (on the Blink thread), so it runs after GC is completed. However, I'll try using ClearKey and saving the promise to run when AesDecyptor is destructed.
Description was changed from ========== 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 uses blink prefinalizer and reverts to destroying the CDM synchronously. That way any outstanding promises that need to be rejected are handled before the blink objects are actually destroyed. BUG=597355 TEST=existing EME tests still pass ========== to ========== 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 ==========
Updated to reject promises asynchronously rather than using predispose (which is part of gc, so the same error occurs).
xhwang@chromium.org changed reviewers: + ddorwin@chromium.org
+ddorwin to see whether it's okay to only post for rejections for now. The current CL is easier to merge FWIW. Otherwise LGTM % comments. https://codereview.chromium.org/1987883002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp (right): https://codereview.chromium.org/1987883002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:93: // already posted that need to happen only after the promise is resolved.) Can we add a TODO to improve the resolving part as well? It seems a good thing to do to avoid reentering blink in both the resolving and rejecting cases. https://codereview.chromium.org/1987883002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:98: void ContentDecryptionModuleResultPromise::rejectLater(ExceptionCode code, const String& errorMessage) rejectLater() actually rejects the promise immediately :) Maybe just something like rejectInternal()?
https://codereview.chromium.org/1987883002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp (right): https://codereview.chromium.org/1987883002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:91: // destroying objects that result in unfulfilled promises being rejected. When a GC destroys the object, reject() won't be called. So this CL is just making the promise unrejected forever (if it's not rejected before the object gets destructed), isn't it?
https://codereview.chromium.org/1987883002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp (right): https://codereview.chromium.org/1987883002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:91: // destroying objects that result in unfulfilled promises being rejected. On 2016/06/07 23:45:52, haraken wrote: > > When a GC destroys the object, reject() won't be called. So this CL is just > making the promise unrejected forever (if it's not rejected before the object > gets destructed), isn't it? We do reject unfulfilled promises during destruction (e.g. when GC destroys the object), which was the cause of the original issue. Previously this would cause the promise being rejected synchronously in the same callstack where the GC destroys the object, causing the crash. You can see an example of such callstack at https://bugs.chromium.org/p/chromium/issues/detail?id=597355#c36 This CL basically force post a task for promise rejection such that we break that callstack. The promise should still be rejected, but at a later time (after GC).
On 2016/06/08 06:52:56, xhwang wrote: > https://codereview.chromium.org/1987883002/diff/20001/third_party/WebKit/Sour... > File > third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp > (right): > > https://codereview.chromium.org/1987883002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:91: > // destroying objects that result in unfulfilled promises being rejected. > On 2016/06/07 23:45:52, haraken wrote: > > > > When a GC destroys the object, reject() won't be called. So this CL is just > > making the promise unrejected forever (if it's not rejected before the object > > gets destructed), isn't it? > > We do reject unfulfilled promises during destruction (e.g. when GC destroys the > object), which was the cause of the original issue. Previously this would cause > the promise being rejected synchronously in the same callstack where the GC > destroys the object, causing the crash. You can see an example of such callstack > at https://bugs.chromium.org/p/chromium/issues/detail?id=597355#c36 > > This CL basically force post a task for promise rejection such that we break > that callstack. The promise should still be rejected, but at a later time (after > GC). Makes sense. LGTM as a short-term, temporary fix.
On 2016/06/08 06:57:54, haraken wrote: > On 2016/06/08 06:52:56, xhwang wrote: > > > https://codereview.chromium.org/1987883002/diff/20001/third_party/WebKit/Sour... > > File > > > third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp > > (right): > > > > > https://codereview.chromium.org/1987883002/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:91: > > // destroying objects that result in unfulfilled promises being rejected. > > On 2016/06/07 23:45:52, haraken wrote: > > > > > > When a GC destroys the object, reject() won't be called. So this CL is just > > > making the promise unrejected forever (if it's not rejected before the > object > > > gets destructed), isn't it? > > > > We do reject unfulfilled promises during destruction (e.g. when GC destroys > the > > object), which was the cause of the original issue. Previously this would > cause > > the promise being rejected synchronously in the same callstack where the GC > > destroys the object, causing the crash. You can see an example of such > callstack > > at https://bugs.chromium.org/p/chromium/issues/detail?id=597355#c36 > > > > This CL basically force post a task for promise rejection such that we break > > that callstack. The promise should still be rejected, but at a later time > (after > > GC). > > Makes sense. LGTM as a short-term, temporary fix. lgtm (verified it fixes the crash). You mentioned you had hacked one of the tests to reproduce the crash, is it possible to turn that into a real test for this case?
Thanks for the reviews. I've opened http://crbug.com/618476 to track adding a test case to check this. My hack modified ClearKey, so we won't be able to have a decent test until we load the CDM directly (i.e. not as a plugin). https://codereview.chromium.org/1987883002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp (right): https://codereview.chromium.org/1987883002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:93: // already posted that need to happen only after the promise is resolved.) On 2016/06/07 23:33:46, xhwang wrote: > Can we add a TODO to improve the resolving part as well? It seems a good thing > to do to avoid reentering blink in both the resolving and rejecting cases. Done. https://codereview.chromium.org/1987883002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.cpp:98: void ContentDecryptionModuleResultPromise::rejectLater(ExceptionCode code, const String& errorMessage) On 2016/06/07 23:33:46, xhwang wrote: > rejectLater() actually rejects the promise immediately :) > > Maybe just something like rejectInternal()? Done.
The CQ bit was checked by jrummell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org, yhirano@google.com, halliwell@chromium.org, haraken@chromium.org, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/1987883002/#ps40001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1987883002/40001
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Committed: https://crrev.com/3135b5eec1c39fb213cbf906f81553efa8cbaf67 Cr-Commit-Position: refs/heads/master@{#398986} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/3135b5eec1c39fb213cbf906f81553efa8cbaf67 Cr-Commit-Position: refs/heads/master@{#398986} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
