New issue
Advanced search Search tips

Issue 721499 link

Starred by 2 users
Status: Assigned
Owner:
sande...@chromium.org
Cc:
tguilbert@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Crash in media::DecoderSelector::InitializeDecoder() when destructing MojoVideoDecoder

Project Member Reported by sande...@chromium.org, May 11 2017 
On destruction, MojoVideoDecoder synchronously aborts outstanding |init_cb| and |decode_cb| callbacks. In some cases, this causes DecoderSelector to attempt to dereference null scoped_refptrs.

There are two steps to fix this properly:

  1. Decide on and document the correct handling of outstanding callbacks on the VideoDecoder interface.

     I recommend that the contact not require outstanding callbacks to be called, and further that we should explicitly disallow synchronous calls from the destructor. We may wish to go further and disallow synchronous calls at all (in which case a task runner should be passed in).

  2. Update implementations and clients of the API.

Comment 1 by sande...@chromium.org, May 11 2017 

There are only a few synchronous cases in current VideoDecoders:
  - GpuVideoDecoder calls |decode_cb| async from Decode(), but sync from elsewhere.
  - FFmpegVideoDecoder calls |output_cb| sync from Decode()
  - VpxVideoDecoder calls |output_cb| sync if it is not using an offload thread.

I propose that we adopt these rules:
  - Decoders must not call callbacks synchronously from Initialize(), Decode(), Reset(), or their destructors.
  - Otherwise they can post or call synchronously however they want.
  - Decoders do not need to return any callbacks at destruction (but may do so async).

Steps required to implement this:
  - DecoderStream/DecoderSelector should be audited to make sure that the fallback paths are not triggered after decoder destruction (the callbacks should use weak pointers that are invalidated at that time).
  - MojoVideoDecoder should stop calling these callbacks in its destructor.
  - Other VideoDecoder clients (VideoDecoderShim) should also be audited.
►

Sign in to add a comment