Revert of Fix gpu command buffer use after free by GrContext

Revert of Fix gpu command buffer use after free by GrContext (patchset #14 id:250001 of https://codereview.chromium.org/1414683003/ )

Reason for revert:
Reverting because this breaks several layout tests:
STDERR: [3888:1580:1113/081604:621866:FATAL:context_provider_command_buffer.cc(79)] Check failed: context_thread_checker_.CalledOnValidThread(). 
STDERR: Backtrace:
STDERR: 	base::debug::StackTrace::StackTrace [0x00D8DC31+33]
STDERR: 	logging::LogMessage::~LogMessage [0x00DE69DB+75]
STDERR: 	content::ContextProviderCommandBuffer::WebContext3D [0x11BB5DA7+887]
STDERR: 	content::ContextProviderCommandBuffer::ContextGL [0x11BB481B+267]
STDERR: 	content::RenderThreadImpl::SharedWorkerContextProvider [0x1202B2A0+400]
STDERR: 	content::RenderWidget::CreateOutputSurface [0x12089254+452]
STDERR: 	content::RenderWidgetCompositor::RequestNewOutputSurface [0x11E7767F+127]


https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win7__dbg_/results/layout-test-results/virtual/mediasession/media/mediasession/htmlmediaelement-set-session-crash-log.txt

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%29/builds/3500

Original issue's description:
> Fix gpu command buffer use after free by GrContext
> 
> ContextProviderCommandBuffer owns a WebGraphicsContext3DCommandBufferImpl and a
> GrContextForWebGraphicsContext3D via scoped_ptr. The problem was
> that the GrContext object held by GrContextForWebGraphicsContext3D
> depended on interface pointers that reference an interface that is owned
> by WebGraphicsContext3DCommandBufferImpl, so whenever the
> GrContext outlived the ContextProviderCommandBuffer, we ended up in a
> state where the interface function pointers are deallocated, but still
> referenced. Then, attempts to use the GrContext would result in using
> deallocated function pointers. Because the GrContext is a ref counted
> object, it can easily outlive the ContextProviderCommandBuffer. This led to
> a dangerous situation where we had to be careful about object destruction
> order.
> 
> This CL fixes the problem for good by wrapping the ownership of the
> WebGraphicsContext3DCommandBufferImpl into a subclass of
> GrGLInterface, which is a ref counted object that can be owned jointly by
> the GrContext and the ContextProviderCommandBuffer, thus guaranteeing
> that the command buffer interface will remain valid for the lifetimes of the
> GrContext and of the ContextProviderCommandBuffer.
> 
> BUG=551143
> CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
> 
> Committed: https://crrev.com/d87aa1f1aee6ab0181eadaae827a5768981c1ccc
> Cr-Commit-Position: refs/heads/master@{#359493}

TBR=piman@chromium.org,danakj@chromium.org,kbr@chromium.org,rbyers@chromium.org,boliu@chromium.org,dtrainor@chromium.org,junov@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=551143

[cbiesinger, 2015-11-13 18:57:41]

Created Revert of Fix gpu command buffer use after free by GrContext

[commit-bot: I haz the power, 2015-11-13 19:00:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1444683002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1444683002/1

[commit-bot: I haz the power, 2015-11-13 19:04:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1444683002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1444683002/1

[commit-bot: I haz the power, 2015-11-13 19:07:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-13 19:07:10]

Failed to apply patch for content/browser/gpu/gpu_ipc_browsertests.cc:
While running git apply --index -3 -p1;
  error: patch failed: content/browser/gpu/gpu_ipc_browsertests.cc:197
  Falling back to three-way merge...
  Applied patch to 'content/browser/gpu/gpu_ipc_browsertests.cc' with conflicts.
  U content/browser/gpu/gpu_ipc_browsertests.cc

Patch:       content/browser/gpu/gpu_ipc_browsertests.cc
Index: content/browser/gpu/gpu_ipc_browsertests.cc
diff --git a/content/browser/gpu/gpu_ipc_browsertests.cc
b/content/browser/gpu/gpu_ipc_browsertests.cc
index
932375bc98ccd69d94c121ceb8dd5a3654af5ef2..7154c2eea99418188c12363a5cf57a685cc1a1ed
100644
--- a/content/browser/gpu/gpu_ipc_browsertests.cc
+++ b/content/browser/gpu/gpu_ipc_browsertests.cc
@@ -13,9 +13,6 @@
 #include "content/public/common/content_switches.h"
 #include "content/public/test/content_browser_test.h"
 #include "gpu/blink/webgraphicscontext3d_in_process_command_buffer_impl.h"
-#include "third_party/skia/include/core/SkCanvas.h"
-#include "third_party/skia/include/core/SkSurface.h"
-#include "third_party/skia/include/gpu/GrContext.h"
 #include "ui/gl/gl_switches.h"
 
 namespace {
@@ -197,50 +194,6 @@
 }
 #endif
 
-// Test fails on Windows because GPU Channel set-up does not work.
-#if !defined(OS_WIN)
-#define MAYBE_GrContextKeepsGpuChannelAlive GrContextKeepsGpuChannelAlive
-#else
-#define MAYBE_GrContextKeepsGpuChannelAlive \
-    DISABLED_GrContextKeepsGpuChannelAlive
-#endif
-IN_PROC_BROWSER_TEST_F(BrowserGpuChannelHostFactoryTest,
-                       MAYBE_GrContextKeepsGpuChannelAlive) {
-  // Test for crbug.com/551143
-  // This test verifies that holding a reference to the GrContext created by
-  // a ContextProviderCommandBuffer will keep the gpu channel alive after the
-  // provider has been destroyed. Without this behavior, user code would have
-  // to be careful to destroy objects in the right order to avoid using freed
-  // memory as a function pointer in the GrContext's GrGLInterface instance.
-  DCHECK(!IsChannelEstablished());
-  EstablishAndWait();
-
-  // Step 2: verify that holding onto the provider's GrContext will
-  // retain the host after provider is destroyed.
-  scoped_refptr<ContextProviderCommandBuffer> provider =
-      ContextProviderCommandBuffer::Create(CreateContext(),
-                                           OFFSCREEN_CONTEXT_FOR_TESTING);
-  EXPECT_TRUE(provider->BindToCurrentThread());
-
-  skia::RefPtr<GrContext> gr_context = skia::SharePtr(provider->GrContext());
-  provider = nullptr;
-
-  SkImageInfo info = SkImageInfo::MakeN32Premul(100, 100);
-  skia::RefPtr<SkSurface> surface = skia::AdoptRef(SkSurface::NewRenderTarget(
-      gr_context.get(), SkSurface::kNo_Budgeted, info));
-  gr_context = nullptr;
-
-  // use the canvas after the provider and grcontext have been locally
-  // unref'ed. This should work just fine thanks to SkSurface_Gpu ref'ing
-  // the GrContext, which is ref'ing the GrGLInterfaceForWebGraphicsContext3D,
-  // which owns the commandbuffer instance.
-  SkPaint greenFillPaint;
-  greenFillPaint.setColor(SK_ColorGREEN);
-  greenFillPaint.setStyle(SkPaint::kFill_Style);
-  // Passes by not crashing
-  surface->getCanvas()->drawRect(SkRect::MakeWH(100, 100), greenFillPaint);
-}
-
 // Test fails on Chromeos + Mac, flaky on Windows because UI Compositor
 // establishes a GPU channel.
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)

[commit-bot: I haz the power, 2015-11-13 19:09:01]

The CQ bit was unchecked by commit-bot@chromium.org

[Justin Novosad, 2015-11-13 19:57:34]

No worries. I have a fix coming right up.

[Justin Novosad, 2015-11-13 20:08:18]

Description was changed from

==========
Revert of Fix gpu command buffer use after free by GrContext (patchset #14
id:250001 of https://codereview.chromium.org/1414683003/ )

Reason for revert:
Reverting because this breaks several layout tests:
STDERR:
[3888:1580:1113/081604:621866:FATAL:context_provider_command_buffer.cc(79)]
Check failed: context_thread_checker_.CalledOnValidThread(). 
STDERR: Backtrace:
STDERR: 	base::debug::StackTrace::StackTrace [0x00D8DC31+33]
STDERR: 	logging::LogMessage::~LogMessage [0x00DE69DB+75]
STDERR: 	content::ContextProviderCommandBuffer::WebContext3D [0x11BB5DA7+887]
STDERR: 	content::ContextProviderCommandBuffer::ContextGL [0x11BB481B+267]
STDERR: 	content::RenderThreadImpl::SharedWorkerContextProvider [0x1202B2A0+400]
STDERR: 	content::RenderWidget::CreateOutputSurface [0x12089254+452]
STDERR: 	content::RenderWidgetCompositor::RequestNewOutputSurface
[0x11E7767F+127]


https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win7__dbg...

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%...

Original issue's description:
> Fix gpu command buffer use after free by GrContext
> 
> ContextProviderCommandBuffer owns a WebGraphicsContext3DCommandBufferImpl and
a
> GrContextForWebGraphicsContext3D via scoped_ptr. The problem was
> that the GrContext object held by GrContextForWebGraphicsContext3D
> depended on interface pointers that reference an interface that is owned
> by WebGraphicsContext3DCommandBufferImpl, so whenever the
> GrContext outlived the ContextProviderCommandBuffer, we ended up in a
> state where the interface function pointers are deallocated, but still
> referenced. Then, attempts to use the GrContext would result in using
> deallocated function pointers. Because the GrContext is a ref counted
> object, it can easily outlive the ContextProviderCommandBuffer. This led to
> a dangerous situation where we had to be careful about object destruction
> order.
> 
> This CL fixes the problem for good by wrapping the ownership of the
> WebGraphicsContext3DCommandBufferImpl into a subclass of
> GrGLInterface, which is a ref counted object that can be owned jointly by
> the GrContext and the ContextProviderCommandBuffer, thus guaranteeing
> that the command buffer interface will remain valid for the lifetimes of the
> GrContext and of the ContextProviderCommandBuffer.
> 
> BUG=551143
> CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
> 
> Committed: https://crrev.com/d87aa1f1aee6ab0181eadaae827a5768981c1ccc
> Cr-Commit-Position: refs/heads/master@{#359493}

TBR=piman@chromium.org,danakj@chromium.org,kbr@chromium.org,rbyers@chromium.o...
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=551143
==========

to

==========
Revert of Fix gpu command buffer use after free by GrContext (patchset #14
id:250001 of https://codereview.chromium.org/1414683003/ )

Reason for revert:
Reverting because this breaks several layout tests:
STDERR:
[3888:1580:1113/081604:621866:FATAL:context_provider_command_buffer.cc(79)]
Check failed: context_thread_checker_.CalledOnValidThread(). 
STDERR: Backtrace:
STDERR: 	base::debug::StackTrace::StackTrace [0x00D8DC31+33]
STDERR: 	logging::LogMessage::~LogMessage [0x00DE69DB+75]
STDERR: 	content::ContextProviderCommandBuffer::WebContext3D [0x11BB5DA7+887]
STDERR: 	content::ContextProviderCommandBuffer::ContextGL [0x11BB481B+267]
STDERR: 	content::RenderThreadImpl::SharedWorkerContextProvider [0x1202B2A0+400]
STDERR: 	content::RenderWidget::CreateOutputSurface [0x12089254+452]
STDERR: 	content::RenderWidgetCompositor::RequestNewOutputSurface
[0x11E7767F+127]


https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win7__dbg...

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%...

Original issue's description:
> Fix gpu command buffer use after free by GrContext
> 
> ContextProviderCommandBuffer owns a WebGraphicsContext3DCommandBufferImpl and
a
> GrContextForWebGraphicsContext3D via scoped_ptr. The problem was
> that the GrContext object held by GrContextForWebGraphicsContext3D
> depended on interface pointers that reference an interface that is owned
> by WebGraphicsContext3DCommandBufferImpl, so whenever the
> GrContext outlived the ContextProviderCommandBuffer, we ended up in a
> state where the interface function pointers are deallocated, but still
> referenced. Then, attempts to use the GrContext would result in using
> deallocated function pointers. Because the GrContext is a ref counted
> object, it can easily outlive the ContextProviderCommandBuffer. This led to
> a dangerous situation where we had to be careful about object destruction
> order.
> 
> This CL fixes the problem for good by wrapping the ownership of the
> WebGraphicsContext3DCommandBufferImpl into a subclass of
> GrGLInterface, which is a ref counted object that can be owned jointly by
> the GrContext and the ContextProviderCommandBuffer, thus guaranteeing
> that the command buffer interface will remain valid for the lifetimes of the
> GrContext and of the ContextProviderCommandBuffer.
> 
> BUG=551143
> CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
> 
> Committed: https://crrev.com/d87aa1f1aee6ab0181eadaae827a5768981c1ccc
> Cr-Commit-Position: refs/heads/master@{#359493}

TBR=piman@chromium.org,danakj@chromium.org,kbr@chromium.org,rbyers@chromium.o...
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=551143
==========

[Justin Novosad, 2015-11-13 20:08:41]

Closed revert. fix is here: https://codereview.chromium.org/1450433002

[cbiesinger, 2015-11-13 20:51:59]

Thank you!

-Christian

On Fri, Nov 13, 2015 at 12:08 PM,  <junov@chromium.org> wrote:
> Closed revert. fix is here: https://codereview.chromium.org/1450433002
>
> https://codereview.chromium.org/1444683002/

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[cbiesinger, 2015-11-13 20:52:00]

Thank you!

-Christian

On Fri, Nov 13, 2015 at 12:08 PM,  <junov@chromium.org> wrote:
> Closed revert. fix is here: https://codereview.chromium.org/1450433002
>
> https://codereview.chromium.org/1444683002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.