Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.

This is a serious security regression. If you are not able to fix this quickly, please revert the change that introduced it.

If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

https://chromium.googlesource.com/chromium/src/+/5a196055274269b28b99be81b56faf3ef931f5b9 looks relevant

danakj: Can you please take a look? Thanks.

Hello this is your friendly security sheriff here. I wonder if you had any update on this bug?

The crash is content::GpuVideoAcceleratorFactoriesImpl::ReleaseContextProvider() deleting a context provider, which then ends up being called when the context is lost.

void GpuVideoAcceleratorFactoriesImpl::ReleaseContextProvider() {
  DCHECK(task_runner_->BelongsToCurrentThread());

  main_thread_task_runner_->PostTask(
      FROM_HERE,
      base::BindOnce(&GpuVideoAcceleratorFactoriesImpl::SetContextProviderLost,
                     base::Unretained(this)));

  context_provider_ = nullptr;  <- deletes a ui::ContextProviderCommandBuffer


The destructor will unset the callback though:

ContextProviderCommandBuffer::~ContextProviderCommandBuffer() {
  DCHECK(main_thread_checker_.CalledOnValidThread() ||
         context_thread_checker_.CalledOnValidThread());

  if (bind_tried_ && bind_result_ == gpu::ContextResult::kSuccess) {
    // Clear the lock to avoid DCHECKs that the lock is being held during
    // shutdown.
    command_buffer_->SetLock(nullptr);
    // Disconnect lost callbacks during destruction.
    impl_->SetLostContextCallback(base::OnceClosure());  <---- here

Where impl_ is a gpu::ImplementationBase* that points to gles2_impl_ which is a gpu::gles2::GLES2Implementation.

So we're shutting down the ContextProviderCommandBuffer, the impl_ still has a callback to it, and we end up trying to run it.

it says the crash is

void ContextProviderCommandBuffer::OnLostContext() {
  CheckValidThreadOrLockAcquired();

  for (auto& observer : observers_)
    observer.OnContextLost();
  if (gr_context_)                <------- here
    gr_context_->OnLostContext();

So using observers_ was fine, but somehow afterward, |this| is no longer ok.

So theory is:

1. OnLostContext happens.
2. It calls to GPU media code.
3. GPU media code destroys the context.
4. Stack unwinds back up to here.

My CL did change VideoFrameResourceProvider::OnContextLost(), but it drops a raw pointer, not an owning ref there. And we still have to get to GpuVideoAcceleratorFactoriesImpl::ReleaseContextProvider() somehow to do the delete, which the CL does not do.

I don't understand why it is blamed atm.

The latest result is from ToT and it has something explanatory, but it's not that 
CL still?

VideoFrameResourceProvider::OnContextLost() is doing
  if (resource_provider_)
    resource_provider_->ShutdownAndReleaseAllResources();

Which calls release callbacks for resources. One of those release callbacks is holding a scoped_refptr<media::VideoFrame>. When the callback is done, the VideoFrame is destroyed.

This bounces up to media::GpuMemoryBufferVideoFramePool::PoolImpl::MailboxHoldersReleased() to return the video frame's texture.

That goes into GpuVideoAcceleratorFactoriesImpl::ContextGL() to use the GL context, which sees that the context is lost, and destroys the ContextProviderCommandBuffer, in the middle of its OnLostCallback call.

A naive fix is to hold a reference to |this| in ContextProviderCommandBuffer.

But idk we probably don't want to delete the ContextProvider during OnLostContext..

Note the ReleaseCallback would be coming from VideoResourceUpdater, it binds the VideoFrame into its callback to keep it alive for compositing.

I think ContextGL() should return null but not delete the context if its lost. Delete it when a new context is created.

Unfortunately this doesn't repro locally (on linux?)

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

commit 3071401e1b0257d029a6560a9fa8c3c1f695f839
Author: danakj <danakj@chromium.org>
Date: Tue Jun 19 00:23:43 2018

Do not delete the media ContextProvider inside the lost context callback

Currently the GpuVideoAcceleratorFactoriesImpl will check for context
loss every time it tries to use the GL context, and destroy it
immediately if it sees a loss. This is problematic if the
ContextProvider is still on the callstack, which it is if we're inside
the context loss callback. Instead, leave the ContextProvider alive
until the GpuVideoAcceleratorFactoriesImpl stops being used.

R=liberato@chromium.org

Bug:  849131 
Change-Id: I8ec66671b4bbf5d426cb5fb184653a5863de9fd9
Reviewed-on: https://chromium-review.googlesource.com/1104777
Commit-Queue: danakj <danakj@chromium.org>
Reviewed-by: Frank Liberato <liberato@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568256}
[modify] https://crrev.com/3071401e1b0257d029a6560a9fa8c3c1f695f839/content/renderer/media/gpu/gpu_video_accelerator_factories_impl.cc
[modify] https://crrev.com/3071401e1b0257d029a6560a9fa8c3c1f695f839/content/renderer/media/gpu/gpu_video_accelerator_factories_impl.h
[modify] https://crrev.com/3071401e1b0257d029a6560a9fa8c3c1f695f839/content/renderer/render_thread_impl.cc

ClusterFuzz has detected this issue as fixed in range 568241:568257.

Detailed report: https://clusterfuzz.com/testcase?key=4776259490676736

Fuzzer: ifratric-browserfuzzer-v3
Job Type: mac_asan_chrome
Platform Id: mac

Crash Type: Heap-use-after-free
Crash Address: 0x615000129528
Crash State:
  gpu::gles2::GLES2Implementation::OnGpuControlLostContext
  gpu::CommandBufferProxyImpl::OnChannelError
  base::debug::TaskAnnotator::RunTask
  
Sanitizer: address (ASAN)

Recommended Security Severity: High

Regressed: https://clusterfuzz.com/revisions?job=mac_asan_chrome&range=559515:559525
Fixed: https://clusterfuzz.com/revisions?job=mac_asan_chrome&range=568241:568257

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=4776259490676736

See https://github.com/google/clusterfuzz-tools for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

ClusterFuzz testcase 4776259490676736 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

Is VideoInSurfaces enabled in M68?

This bug requires manual review: M68 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

awhalley@ - can you please confirm if this still needs to be merged?

Yes please.

> Is VideoInSurfaces enabled in M68?

Was this answered?

Looks like it's going out as a stable experiment in 68 per issue 793301 (unless I've got the wrong end of the stick)

Thanks

This bug requires manual review: M68 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Approved - branch:3440

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

commit f37d23a71dd6fa7024a1e371663580940571e719
Author: danakj <danakj@chromium.org>
Date: Fri Jul 06 19:45:12 2018

Do not delete the media ContextProvider inside the lost context callback

Currently the GpuVideoAcceleratorFactoriesImpl will check for context
loss every time it tries to use the GL context, and destroy it
immediately if it sees a loss. This is problematic if the
ContextProvider is still on the callstack, which it is if we're inside
the context loss callback. Instead, leave the ContextProvider alive
until the GpuVideoAcceleratorFactoriesImpl stops being used.

TBR=danakj@chromium.org, liberato@chromium.org

(cherry picked from commit 3071401e1b0257d029a6560a9fa8c3c1f695f839)

Bug:  849131 
Change-Id: I8ec66671b4bbf5d426cb5fb184653a5863de9fd9
Reviewed-on: https://chromium-review.googlesource.com/1104777
Commit-Queue: danakj <danakj@chromium.org>
Reviewed-by: Frank Liberato <liberato@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#568256}
Reviewed-on: https://chromium-review.googlesource.com/1128214
Reviewed-by: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#605}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/f37d23a71dd6fa7024a1e371663580940571e719/content/renderer/media/gpu/gpu_video_accelerator_factories_impl.cc
[modify] https://crrev.com/f37d23a71dd6fa7024a1e371663580940571e719/content/renderer/media/gpu/gpu_video_accelerator_factories_impl.h
[modify] https://crrev.com/f37d23a71dd6fa7024a1e371663580940571e719/content/renderer/render_thread_impl.cc

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot