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

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

fsamuel@, could you take a look to see if this is related to your change https://codereview.chromium.org/2932893002?

Please feel free to re-assign. Thanks!

Sigh, annoying bug...cc::Display can outlive FrameSinkManagerHost and crash....and I doubt my CL will revert cleanly. I think the right solution here is to move PerCompositorData to FrameSinkManagerImpl...with a TODO that it's eventually going away once we move to mojo APIs for everything.

What's changed that causes this to happen, can you explain the lifetime and pointer changes?

Previously FrameSinkManagerHost and MojoFrameSinkManager were owned by GpuProcessTransportFactory and so we were ensured that cc::Display would go away prior to MojoFrameSinkManager. Now that FrmaeSinkManagerHost is owned by BrowserMainLoop, it is destroyed prior to GpuProcessTransportFactory it seems and prior to the associated cc::Display going away...

Maybe this can be fixed with some observer pattern...GpuProcessTransportFactory observes FrameSinkManagerHost destruction and destroys its displays...

Are you saying https://cs.chromium.org/chromium/src/content/browser/browser_main_loop.cc?rcl=378a9a29ffab72232c5e1d06c8bb70919ba6c35f&l=1210 is the wrong order?

Heh, good catch :D I totally forgot that ImageTransportFactory IS destroyed explicitly! Yes, that's an easy fix after all :D

Why did you choose the order it is in when you wrote it originally? Are there other dependencies that want the other order?

I don't think there was a very good reason. I pulled it out of ImageTransportFactory so I thought I should put it near ImageTransportFactory's construction/destruction.

I arbitrarily decided to put HostFrameSinkManager's construction AFTER ImageTransportFactory, and so destruction should happen in the opposite order and so I did that...I will verify locally. Maybe there was a problem that I don't recall.

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

commit bc8abba50e2d31501ad5e8e4ce0842f33823773b
Author: Fady Samuel <fsamuel@chromium.org>
Date: Thu Jul 06 17:40:02 2017

Update ImageTransportFactory's construction order

ImageTransportFactory depends on HostFrameSinkManager which depends on
FrameSinkManagerImpl. Thus, we construct:

1. FrameSinkManagerImpl
2. HostFrameSinkManager
3. ImageTransportFactory

Destruction happens in the opposite order:

1. ImageTransportFactory
2. HostFrameSinkManager
3. FrameSinkManagerImpl

Bug:  738652 
Change-Id: If6b3d2abe5f24832c26e8cc159adb4fa24e709b7
Reviewed-on: https://chromium-review.googlesource.com/561639
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#484657}
[modify] https://crrev.com/bc8abba50e2d31501ad5e8e4ce0842f33823773b/content/browser/browser_main_loop.cc

ClusterFuzz has detected this issue as fixed in range 484640:484701.

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

Fuzzer: inferno_twister
Job Type: mac_asan_content_shell
Platform Id: mac

Crash Type: Heap-use-after-free READ 8
Crash Address: 0x619000064c50
Crash State:
  cc::Display::~Display
  content::GpuProcessTransportFactory::~GpuProcessTransportFactory
  content::ImageTransportFactory::Terminate
  
Sanitizer: address (ASAN)

Recommended Security Severity: High

Regressed: https://clusterfuzz.com/revisions?job=mac_asan_content_shell&range=478717:478791
Fixed: https://clusterfuzz.com/revisions?job=mac_asan_content_shell&range=484640:484701

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


See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.

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

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