New issue
Advanced search Search tips

Issue 727948 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

KeySystemConfigSelectorTest should not create KeySystemsImpl

Project Member Reported by xhw...@chromium.org, May 30 2017

Issue description

KeySystemConfigSelectorTest works on a test KeySystems implemenation: FakeKeySystems. So in theory, the test should only need FakeKeySystems and should not require any other KeySystems to be created (e.g. KeySystemsImpl). However, KeySystemConfigSelector actually depends on KeySystemsImpl to work. See the following callstack:

#3 0x7f5095408163 media::KeySystemsImpl::KeySystemsImpl()
#4 0x7f5095407d22 media::KeySystemsImpl::GetInstance()
#5 0x7f509540c0c1 media::CanUseAesDecryptor()
#6 0x7f50964c03cb media::KeySystemConfigSelector::IsSupportedContentType()
#7 0x7f50964c0874 media::KeySystemConfigSelector::GetSupportedCapabilities()
#8 0x7f50964c197f media::KeySystemConfigSelector::GetSupportedConfiguration()
#9 0x7f50964c2bf8 media::KeySystemConfigSelector::SelectConfigInternal()
#10 0x7f50964c29be media::KeySystemConfigSelector::SelectConfig()
#11 0x000000490d16 media::KeySystemConfigSelectorTest::SelectConfig()

This makes things really hard to track since we have two KeySystems created at the same time.

We should fix our implementation and test such that we only have one KeySystems created in real world, and in tests.
 
Labels: -M-61 M-66

Comment 2 by xhw...@chromium.org, Feb 20 2018

Labels: -Pri-2 -M-66 M-67 Hotlist-Fixit Pri-3

Comment 3 by xhw...@chromium.org, Mar 23 2018

Labels: -M-67 M-68

Comment 4 by xhw...@chromium.org, Apr 13 2018

Labels: -M-68
Labels: -Pri-3 Target-69 Pri-2
While I am at it right now I'll see how this can be fixed.
Project Member

Comment 6 by bugdroid1@chromium.org, Jun 8 2018

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

commit 4dec0cd03da192177683174bee5456f252d95d9f
Author: Xiaohan Wang <xhwang@chromium.org>
Date: Fri Jun 08 19:41:39 2018

media: Add KeySystems::CanUseAesDecryptor()

This allows KeySystemConfigSelectorTest to fully depend on the
FakeKeySystems. Previously KeySystemConfigSelector calls
CanUseAesDecryptor() which calls into KeySystemsImpl which makes the
test really confusing.

Bug:  727948 
Change-Id: Id5536a142285034f1ec589758a171c70d3a77153
Reviewed-on: https://chromium-review.googlesource.com/1092412
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Commit-Queue: Xiaohan Wang <xhwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565716}
[modify] https://crrev.com/4dec0cd03da192177683174bee5456f252d95d9f/media/base/key_systems.cc
[modify] https://crrev.com/4dec0cd03da192177683174bee5456f252d95d9f/media/base/key_systems.h
[modify] https://crrev.com/4dec0cd03da192177683174bee5456f252d95d9f/media/blink/key_system_config_selector.cc
[modify] https://crrev.com/4dec0cd03da192177683174bee5456f252d95d9f/media/blink/key_system_config_selector_unittest.cc

Status: Fixed (was: Assigned)

Sign in to add a comment