New issue
Advanced search Search tips

Issue 843784 link

Starred by 1 user
Status: Fixed
Owner:
xhw...@chromium.org
Closed: May 2018
Cc:
liber...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

NOTREACHED() reached in AndroidCdmFactory

Project Member Reported by xhw...@chromium.org, May 16 2018 
During local testing I hit the NOTREACHED() below once, but couldn't repro it again:

if (!MediaDrmBridge::IsKeySystemSupported(key_system)) {
    error_message = "Key system not supported unexpectedly: " + key_system;
    NOTREACHED() << error_message;
    bound_cdm_created_cb.Run(nullptr, error_message);
    return;
}

The NOTREACHED() which essentially DCHECK that MediaDrmBridge::IsKeySystemSupported(key_system) must return true. This DCHECK is based on the fact that we called the same function when deciding whether the key system is supported [1]. However, that is called in the browser process and the DCHECK is in the GPU process. Also, the DCHECK could happen much later. Further, the implemenation of MediaDrmBridge::IsKeySystemSupported() actually calls into Java MediaDrm, where it's hard to assume what it'll return.

With that, I feel we should remove the NOTREACHED() and just fail.
Project Member

Comment 1 by bugdroid1@chromium.org, May 17 2018 

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

commit 8d9220d54b7047d4223eb00856cdeeb36082c5fc
Author: Xiaohan Wang <xhwang@chromium.org>
Date: Thu May 17 00:17:37 2018

media: Remove NOTREACHED() in AndroidCdmFactory

Current we have a NOTREACHED() which essentially DCHECK that
MediaDrmBridge::IsKeySystemSupported(key_system) must return true. This
DCHECK is based on the fact that we called the same function when
deciding whether the key system is supported [1]. However, that is
called in the browser process and the DCHECK is in the GPU process.
Also, the DCHECK could happen much later. Further, the implemenation of
MediaDrmBridge::IsKeySystemSupported() actually calls into Java
MediaDrm, where it's hard to assume what it'll return.

This CL removes the NOTREACHED() so that if that happens we'll just
fail. UMA is also added to track how often this happens in the field.

[1] See CdmMessageFilterAndroid::OnQueryKeySystemSupport()

Bug:  843784 
Change-Id: I6ce743033811e4d2bfbde75ccb715697e6205063
Reviewed-on: https://chromium-review.googlesource.com/1062803
Commit-Queue: Xiaohan Wang <xhwang@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Frank Liberato <liberato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559358}
[modify] https://crrev.com/8d9220d54b7047d4223eb00856cdeeb36082c5fc/media/base/android/android_cdm_factory.cc
[modify] https://crrev.com/8d9220d54b7047d4223eb00856cdeeb36082c5fc/media/base/android/media_drm_bridge.cc
[modify] https://crrev.com/8d9220d54b7047d4223eb00856cdeeb36082c5fc/tools/metrics/histograms/histograms.xml

Comment 2 by xhw...@chromium.org, May 17 2018

Status: Fixed (was: Started)
►

Sign in to add a comment