Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1761603002:
media: Handle "encrypted" event in pipeline_integration_fuzzertest. (Closed)
Descriptionmedia: Handle "encrypted" event in pipeline_integration_fuzzertest.
Set the EncryptedMediaInitDataCB to handle "encrypted" event. In the callback we'll simply stop the test and error out.
BUG=584119
TEST=No more CHECK fire.
Committed: https://crrev.com/b7fa6915f0e6bef0cd0d55e6da203e0c7f921efe
Cr-Commit-Position: refs/heads/master@{#379101}
Patch Set 1 #
Total comments: 5
Patch Set 2 : comments addressed #
Total comments: 1
Patch Set 3 : rebase only #
Total comments: 6
Patch Set 4 : comments addressed #
Total comments: 2
Patch Set 5 : comments addressed #
Messages
Total messages: 28 (9 generated)
Description was changed from ========== media: Handle "encrypted" event when EME is not supported. In this case the test will fail gracefully. BUG=584119 TEST=No crash on CHECK. ========== to ========== media: Handle "encrypted" event when EME is not supported. Currently we handle EME in PipelineIntegrationTest, not in PipelineIntegrationTestBase. So when the media file is encrypted, we should fail gracefully instead of a crash on CHECK. BUG=584119 TEST=No crash on CHECK. ==========
xhwang@chromium.org changed reviewers: + dalecurtis@chromium.org
PTAL
ddorwin@chromium.org changed reviewers: + ddorwin@chromium.org
It's not clear why this is a problem. Are there platforms or upcoming tests that do not set the CB? Will they never set the CB? https://chromiumcodereview.appspot.com/1761603002/diff/1/media/test/pipeline_... File media/test/pipeline_integration_test_base.cc (right): https://chromiumcodereview.appspot.com/1761603002/diff/1/media/test/pipeline_... media/test/pipeline_integration_test_base.cc:78: if (encrypted_media_init_data_cb_.is_null()) { Would ASSERT/EXPECT_FALSE() be better? Then the test would fail but not crash? https://chromiumcodereview.appspot.com/1761603002/diff/1/media/test/pipeline_... media/test/pipeline_integration_test_base.cc:80: DLOG(WARNING) << "Encryted media not supported."; Encry*p*ted Supported by the test? "Test does not implement support for EME"? "Test does not expect content to be encrypted"?
comment only https://chromiumcodereview.appspot.com/1761603002/diff/1/media/test/pipeline_... File media/test/pipeline_integration_test_base.cc (right): https://chromiumcodereview.appspot.com/1761603002/diff/1/media/test/pipeline_... media/test/pipeline_integration_test_base.cc:78: if (encrypted_media_init_data_cb_.is_null()) { On 2016/03/02 21:42:12, ddorwin wrote: > Would ASSERT/EXPECT_FALSE() be better? Then the test would fail but not crash? PipelineIntegrationTestBase is used by PipelineIntegrationTest, FFmpegRegressionTest and the new pipeline_integration_fuzzertest. We only support EME in PipelineIntegrationTest by supplying a FakeEncryptedMedia to handle EME events. Also we set set_encrypted_media_init_data_cb explicit in those tests: https://code.google.com/p/chromium/codesearch#chromium/src/media/test/pipelin... ASSERT_FALSE will only abort the current function. We still need to post QuitWhenIdleClosure() to be able to abort the test. Otherwise the test will abort. Setting pipeline_status_ on line 81 would finally trigger the test to fail.
Sorry it should be "otherwise the test will timeout".
Defer to David since he has opinions here :) lgtm otherwise. https://chromiumcodereview.appspot.com/1761603002/diff/1/media/test/pipeline_... File media/test/pipeline_integration_test_base.cc (right): https://chromiumcodereview.appspot.com/1761603002/diff/1/media/test/pipeline_... media/test/pipeline_integration_test_base.cc:82: message_loop_.PostTask(FROM_HERE, base::MessageLoop::QuitWhenIdleClosure()); Oy, I forgot we use this in this file, we should convert this class to use RunLoops since this isn't guaranteed to quit in some cases :)
Per offline discussion with ddorwin. I'll try to move the code added to the fuzzer instead of in the PITB. https://chromiumcodereview.appspot.com/1761603002/diff/1/media/test/pipeline_... File media/test/pipeline_integration_test_base.cc (right): https://chromiumcodereview.appspot.com/1761603002/diff/1/media/test/pipeline_... media/test/pipeline_integration_test_base.cc:82: message_loop_.PostTask(FROM_HERE, base::MessageLoop::QuitWhenIdleClosure()); On 2016/03/03 00:17:59, DaleCurtis wrote: > Oy, I forgot we use this in this file, we should convert this class to use > RunLoops since this isn't guaranteed to quit in some cases :) sgtm. In a different CL though :)
comments addressed
Description was changed from ========== media: Handle "encrypted" event when EME is not supported. Currently we handle EME in PipelineIntegrationTest, not in PipelineIntegrationTestBase. So when the media file is encrypted, we should fail gracefully instead of a crash on CHECK. BUG=584119 TEST=No crash on CHECK. ========== to ========== media: Handle "encrypted" event in pipeline_integration_fuzzertest. Set the EncryptedMediaInitDataCB to handle "encrypted" event. In the callback we'll simply stop the test and error out. BUG=584119 TEST=No crash on CHECK. ==========
Description was changed from ========== media: Handle "encrypted" event in pipeline_integration_fuzzertest. Set the EncryptedMediaInitDataCB to handle "encrypted" event. In the callback we'll simply stop the test and error out. BUG=584119 TEST=No crash on CHECK. ========== to ========== media: Handle "encrypted" event in pipeline_integration_fuzzertest. Set the EncryptedMediaInitDataCB to handle "encrypted" event. In the callback we'll simply stop the test and error out. BUG=584119 TEST=No more CHECK fire. ==========
PTAL again! https://chromiumcodereview.appspot.com/1761603002/diff/20001/media/test/pipel... File media/test/pipeline_integration_test_base.h (right): https://chromiumcodereview.appspot.com/1761603002/diff/20001/media/test/pipel... media/test/pipeline_integration_test_base.h:95: I moved these to "public" so that they can be called in the fuzzertest.
rebase only
Thanks for changing the approach! LGTM with comments. https://chromiumcodereview.appspot.com/1761603002/diff/40001/media/test/pipel... File media/test/pipeline_integration_fuzzertest.cc (right): https://chromiumcodereview.appspot.com/1761603002/diff/40001/media/test/pipel... media/test/pipeline_integration_fuzzertest.cc:22: test->OnError(media::PIPELINE_ERROR_INITIALIZATION_FAILED); LOG(WARNING) with an explanation? At least explain how this can happen and how it is handled. https://chromiumcodereview.appspot.com/1761603002/diff/40001/media/test/pipel... File media/test/pipeline_integration_test_base.h (right): https://chromiumcodereview.appspot.com/1761603002/diff/40001/media/test/pipel... media/test/pipeline_integration_test_base.h:94: void OnError(PipelineStatus status); OnError sounds like an event handler. When public, the caller is saying FailWithError or something like that. I suggest restoring this to protected and adding a new public method that calls it.
comments addressed
https://chromiumcodereview.appspot.com/1761603002/diff/40001/media/test/pipel... File media/test/pipeline_integration_fuzzertest.cc (right): https://chromiumcodereview.appspot.com/1761603002/diff/40001/media/test/pipel... media/test/pipeline_integration_fuzzertest.cc:22: test->OnError(media::PIPELINE_ERROR_INITIALIZATION_FAILED); On 2016/03/03 17:47:47, ddorwin wrote: > LOG(WARNING) with an explanation? At least explain how this can happen and how > it is handled. fuzzer owners don't like logs in fuzzer tests: https://chromiumcodereview.appspot.com/1757603002/diff/20001/testing/libfuzze... https://chromiumcodereview.appspot.com/1761603002/diff/40001/media/test/pipel... File media/test/pipeline_integration_test_base.h (right): https://chromiumcodereview.appspot.com/1761603002/diff/40001/media/test/pipel... media/test/pipeline_integration_test_base.h:94: void OnError(PipelineStatus status); On 2016/03/03 17:47:47, ddorwin wrote: > OnError sounds like an event handler. When public, the caller is saying > FailWithError or something like that. I suggest restoring this to protected and > adding a new public method that calls it. Done.
https://chromiumcodereview.appspot.com/1761603002/diff/40001/media/test/pipel... File media/test/pipeline_integration_fuzzertest.cc (right): https://chromiumcodereview.appspot.com/1761603002/diff/40001/media/test/pipel... media/test/pipeline_integration_fuzzertest.cc:22: test->OnError(media::PIPELINE_ERROR_INITIALIZATION_FAILED); On 2016/03/03 19:41:32, xhwang wrote: > On 2016/03/03 17:47:47, ddorwin wrote: > > LOG(WARNING) with an explanation? At least explain how this can happen and how > > it is handled. > > fuzzer owners don't like logs in fuzzer tests: > > https://chromiumcodereview.appspot.com/1757603002/diff/20001/testing/libfuzze... As a comment then? This one might be worth logging, though. It's telling us that the media was not actually processed. An attacker could provide this media and an appropriate key then get further into it than we have fuzzed. (I don't know how much we care about that case.) Also, the fact that we fail here means we aren't testing any (potentially) encrypted media. https://chromiumcodereview.appspot.com/1761603002/diff/60001/media/test/pipel... File media/test/pipeline_integration_test_base.h (right): https://chromiumcodereview.appspot.com/1761603002/diff/60001/media/test/pipel... media/test/pipeline_integration_test_base.h:111: // Sets a callback to handle EME "encrypted" event. Must be called to test ... called if the media might be encrypted? "potentially encrypted media" has (had) a specific meaning for EME - media with initData.
comments addressed
Thanks for the comment. Please take a look again. https://chromiumcodereview.appspot.com/1761603002/diff/40001/media/test/pipel... File media/test/pipeline_integration_fuzzertest.cc (right): https://chromiumcodereview.appspot.com/1761603002/diff/40001/media/test/pipel... media/test/pipeline_integration_fuzzertest.cc:22: test->OnError(media::PIPELINE_ERROR_INITIALIZATION_FAILED); On 2016/03/03 19:46:20, ddorwin wrote: > On 2016/03/03 19:41:32, xhwang wrote: > > On 2016/03/03 17:47:47, ddorwin wrote: > > > LOG(WARNING) with an explanation? At least explain how this can happen and > how > > > it is handled. > > > > fuzzer owners don't like logs in fuzzer tests: > > > > > https://chromiumcodereview.appspot.com/1757603002/diff/20001/testing/libfuzze... > > As a comment then? > > This one might be worth logging, though. It's telling us that the media was not > actually processed. An attacker could provide this media and an appropriate key > then get further into it than we have fuzzed. (I don't know how much we care > about that case.) > > Also, the fact that we fail here means we aren't testing any (potentially) > encrypted media. Added a comment and a TODO. https://chromiumcodereview.appspot.com/1761603002/diff/60001/media/test/pipel... File media/test/pipeline_integration_test_base.h (right): https://chromiumcodereview.appspot.com/1761603002/diff/60001/media/test/pipel... media/test/pipeline_integration_test_base.h:111: // Sets a callback to handle EME "encrypted" event. Must be called to test On 2016/03/03 19:46:20, ddorwin wrote: > ... called if the media might be encrypted? > "potentially encrypted media" has (had) a specific meaning for EME - media with > initData. The EncryptedMediaInitDataCB will be fired iff the media has initData. So this comment is actually accurate?
lgtm
The CQ bit was checked by xhwang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1761603002/#ps80001 (title: "comments addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1761603002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1761603002/80001
Message was sent while issue was closed.
Description was changed from ========== media: Handle "encrypted" event in pipeline_integration_fuzzertest. Set the EncryptedMediaInitDataCB to handle "encrypted" event. In the callback we'll simply stop the test and error out. BUG=584119 TEST=No more CHECK fire. ========== to ========== media: Handle "encrypted" event in pipeline_integration_fuzzertest. Set the EncryptedMediaInitDataCB to handle "encrypted" event. In the callback we'll simply stop the test and error out. BUG=584119 TEST=No more CHECK fire. ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== media: Handle "encrypted" event in pipeline_integration_fuzzertest. Set the EncryptedMediaInitDataCB to handle "encrypted" event. In the callback we'll simply stop the test and error out. BUG=584119 TEST=No more CHECK fire. ========== to ========== media: Handle "encrypted" event in pipeline_integration_fuzzertest. Set the EncryptedMediaInitDataCB to handle "encrypted" event. In the callback we'll simply stop the test and error out. BUG=584119 TEST=No more CHECK fire. Committed: https://crrev.com/b7fa6915f0e6bef0cd0d55e6da203e0c7f921efe Cr-Commit-Position: refs/heads/master@{#379101} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b7fa6915f0e6bef0cd0d55e6da203e0c7f921efe Cr-Commit-Position: refs/heads/master@{#379101} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
