Reland "gpu: Add message filter destructor stack traces to crash keys."

This is a reland of 1d62963 I had assumed that unregistered crash keys would be a nop but they need to be registered or else a DCHECK is triggered. I also made sure gpu/config/gpu_crash_keys is used as the source of truth for gpu related crash keys. Original change's description: > gpu: Add message filter destructor stack traces to crash keys. > > base::debug::Alias values don't show up in minidumps. Using crash keys > is recommended for use-after-free bugs in the following doc: > http://dev.chromium.org/developers/debugging-with-crash-keys > > R=jbauman,rsesek,sandersd > BUG=729483 > > Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel > Change-Id: I8fd129daf66dc9235307a28fe5cd7a6c23b85a94 > Reviewed-on: https://chromium-review.googlesource.com/538056 > Reviewed-by: Robert Sesek <rsesek@chromium.org> > Reviewed-by: John Bauman <jbauman@chromium.org> > Reviewed-by: Dan Sanders <sandersd@chromium.org> > Commit-Queue: Sunny Sachanandani <sunnyps@chromium.org> > Cr-Commit-Position: refs/heads/master@{#480879} TBR=jbauman,sandersd Bug: 729483, 735175 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win10_chromium_x64_rel_ng;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: I05c46bb583135dff05d5bc75dff694a1caa54b32 Reviewed-on: https://chromium-review.googlesource.com/541937 Reviewed-by: Robert Sesek <rsesek@chromium.org> Reviewed-by: Bo Liu <boliu@chromium.org> Reviewed-by: Robert Shield <robertshield@chromium.org> Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org> Reviewed-by: Alok Priyadarshi <alokp@chromium.org> Reviewed-by: Ken Rockot <rockot@chromium.org> Commit-Queue: Sunny Sachanandani <sunnyps@chromium.org> Cr-Commit-Position: refs/heads/master@{#481787}
marcosholgado · Jun 23, 2017 · a2c68af · a2c68af
1 parent 8a33581
commit a2c68af
Show file tree

Hide file tree

Showing 16 changed files with 82 additions and 55 deletions.
diff --git a/android_webview/BUILD.gn b/android_webview/BUILD.gn
@@ -730,6 +730,7 @@ source_set("common") {
     "//gpu/command_buffer/client:gles2_implementation",
     "//gpu/command_buffer/common:gles2_utils",
     "//gpu/command_buffer/service",
+    "//gpu/config:crash_keys",
     "//gpu/ipc:gl_in_process_context",
     "//gpu/skia_bindings",
     "//media",

diff --git a/android_webview/common/crash_reporter/crash_keys.cc b/android_webview/common/crash_reporter/crash_keys.cc
@@ -7,6 +7,7 @@
 #include "base/debug/crash_logging.h"
 #include "components/crash/content/app/breakpad_linux.h"
 #include "components/crash/core/common/crash_keys.h"
+#include "gpu/config/gpu_crash_keys.h"
 
 using namespace crash_keys;
 
@@ -25,12 +26,6 @@ const char kAppPackageVersionCode[] = "app-package-version-code";
 
 const char kAndroidSdkInt[] = "android-sdk-int";
 
-const char kGPUDriverVersion[] = "gpu-driver";
-const char kGPUPixelShaderVersion[] = "gpu-psver";
-const char kGPUVertexShaderVersion[] = "gpu-vsver";
-const char kGPUVendor[] = "gpu-gl-vendor";
-const char kGPURenderer[] = "gpu-gl-renderer";
-
 const char kInputEventFilterSendFailure[] = "input-event-filter-send-failure";
 
 const char kViewCount[] = "view-count";
@@ -48,15 +43,21 @@ size_t RegisterWebViewCrashKeys() {
       {kVariations, kHugeSize},
       {kShutdownType, kSmallSize},
       {kBrowserUnpinTrace, kMediumSize},
-      {kGPUDriverVersion, kSmallSize},
-      {kGPUPixelShaderVersion, kSmallSize},
-      {kGPUVertexShaderVersion, kSmallSize},
-      {kGPUVendor, kSmallSize},
-      {kGPURenderer, kSmallSize},
       {kAppPackageName, kSmallSize},
       {kAppPackageVersionCode, kSmallSize},
       {kAndroidSdkInt, kSmallSize},
 
+      // gpu
+      {gpu::crash_keys::kGPUDriverVersion, kSmallSize},
+      {gpu::crash_keys::kGPUPixelShaderVersion, kSmallSize},
+      {gpu::crash_keys::kGPUVertexShaderVersion, kSmallSize},
+      {gpu::crash_keys::kGPUVendor, kSmallSize},
+      {gpu::crash_keys::kGPURenderer, kSmallSize},
+      // Temporary for https://crbug.com/729483.
+      // TODO(sunnyps): Remove after https://crbug.com/729483 is fixed.
+      {gpu::crash_keys::kGpuChannelFilterTrace, kMediumSize},
+      {gpu::crash_keys::kMediaGpuChannelFilterTrace, kMediumSize},
+
       // content/:
       {"bad_message_reason", kSmallSize},
       {"discardable-memory-allocated", kSmallSize},
@@ -143,15 +144,17 @@ size_t RegisterWebViewCrashKeys() {
 // clang-format off
 const char* const kWebViewCrashKeyWhiteList[] = {
     "AW_WHITELISTED_DEBUG_KEY",
-    kGPUDriverVersion,
-    kGPUPixelShaderVersion,
-    kGPUVertexShaderVersion,
-    kGPUVendor,
-    kGPURenderer,
     kAppPackageName,
     kAppPackageVersionCode,
     kAndroidSdkInt,
 
+    // gpu
+    gpu::crash_keys::kGPUDriverVersion,
+    gpu::crash_keys::kGPUPixelShaderVersion,
+    gpu::crash_keys::kGPUVertexShaderVersion,
+    gpu::crash_keys::kGPUVendor,
+    gpu::crash_keys::kGPURenderer,
+
     // content/:
     "bad_message_reason",
     "discardable-memory-allocated",

diff --git a/android_webview/common/crash_reporter/crash_keys.h b/android_webview/common/crash_reporter/crash_keys.h
@@ -21,13 +21,6 @@ extern const char* const kWebViewCrashKeyWhiteList[];
 
 // Crash Key Name Constants ////////////////////////////////////////////////////
 
-// GPU information.
-extern const char kGPUDriverVersion[];
-extern const char kGPUPixelShaderVersion[];
-extern const char kGPUVertexShaderVersion[];
-extern const char kGPUVendor[];
-extern const char kGPURenderer[];
-
 // Application information.
 extern const char kAppPackageName[];
 extern const char kAppPackageVersionCode[];

diff --git a/chrome/app/chrome_crash_reporter_client_win.cc b/chrome/app/chrome_crash_reporter_client_win.cc
@@ -27,6 +27,7 @@
 #include "components/crash/content/app/crashpad.h"
 #include "components/crash/core/common/crash_keys.h"
 #include "components/version_info/channel.h"
+#include "gpu/config/gpu_crash_keys.h"
 
 namespace {
 
@@ -52,12 +53,6 @@ constexpr char kBrowserUnpinTrace[] = "browser-unpin-trace";
 constexpr char kApValue[] = "ap";
 constexpr char kCohortName[] = "cohort-name";
 
-constexpr char kGPUVendorID[] = "gpu-venid";
-constexpr char kGPUDeviceID[] = "gpu-devid";
-constexpr char kGPUDriverVersion[] = "gpu-driver";
-constexpr char kGPUPixelShaderVersion[] = "gpu-psver";
-constexpr char kGPUVertexShaderVersion[] = "gpu-vsver";
-
 constexpr char kHungRendererOutstandingAckCount[] = "hung-outstanding-acks";
 constexpr char kHungRendererOutstandingEventType[] =
     "hung-outstanding-event-type";
@@ -109,11 +104,17 @@ size_t RegisterCrashKeysHelper() {
       {kBrowserUnpinTrace, kMediumSize},
       {kApValue, kSmallSize},
       {kCohortName, kSmallSize},
-      {kGPUVendorID, kSmallSize},
-      {kGPUDeviceID, kSmallSize},
-      {kGPUDriverVersion, kSmallSize},
-      {kGPUPixelShaderVersion, kSmallSize},
-      {kGPUVertexShaderVersion, kSmallSize},
+
+      // gpu
+      {gpu::crash_keys::kGPUVendorID, kSmallSize},
+      {gpu::crash_keys::kGPUDeviceID, kSmallSize},
+      {gpu::crash_keys::kGPUDriverVersion, kSmallSize},
+      {gpu::crash_keys::kGPUPixelShaderVersion, kSmallSize},
+      {gpu::crash_keys::kGPUVertexShaderVersion, kSmallSize},
+      // Temporary for https://crbug.com/729483.
+      // TODO(sunnyps): Remove after https://crbug.com/729483 is fixed.
+      {gpu::crash_keys::kGpuChannelFilterTrace, kMediumSize},
+      {gpu::crash_keys::kMediaGpuChannelFilterTrace, kMediumSize},
 
       // browser/:
       {kThirdPartyModulesLoaded, kSmallSize},

diff --git a/chrome/common/crash_keys.cc b/chrome/common/crash_keys.cc
@@ -112,6 +112,8 @@ size_t RegisterChromeCrashKeys() {
     {kApValue, kSmallSize},
     {kCohortName, kSmallSize},
 #endif  // defined(OS_WIN)
+
+// gpu
 #if !defined(OS_ANDROID)
     {gpu::crash_keys::kGPUVendorID, kSmallSize},
     {gpu::crash_keys::kGPUDeviceID, kSmallSize},
@@ -125,6 +127,10 @@ size_t RegisterChromeCrashKeys() {
     {gpu::crash_keys::kGPUVendor, kSmallSize},
     {gpu::crash_keys::kGPURenderer, kSmallSize},
 #endif
+    // Temporary for https://crbug.com/729483.
+    // TODO(sunnyps): Remove after https://crbug.com/729483 is fixed.
+    {gpu::crash_keys::kGpuChannelFilterTrace, kMediumSize},
+    {gpu::crash_keys::kMediaGpuChannelFilterTrace, kMediumSize},
 
     // content/:
     {"bad_message_reason", kSmallSize},
@@ -169,7 +175,7 @@ size_t RegisterChromeCrashKeys() {
     {"channel_error_bt", kMediumSize},
     {"remove_route_bt", kMediumSize},
     {"rwhvm_window", kMediumSize},
-    // media/:
+// media/:
 #endif
     {kBug464926CrashKey, kSmallSize},
     {kViewCount, kSmallSize},

diff --git a/chrome/test/BUILD.gn b/chrome/test/BUILD.gn
@@ -217,6 +217,7 @@ static_library("test_support") {
     public_deps += [
       "//chrome/install_static/test:test_support",
       "//components/crash/content/app",
+      "//gpu/config:crash_keys",
       "//third_party/wtl",
     ]
   }

diff --git a/chrome_elf/BUILD.gn b/chrome_elf/BUILD.gn
@@ -152,6 +152,7 @@ static_library("crash") {
     "//components/crash/core/common",  # crash_keys
     "//components/version_info:channel",
     "//content/public/common:result_codes",
+    "//gpu/config:crash_keys",
     "//third_party/crashpad/crashpad/client:client",  # DumpWithoutCrash
   ]
 }

diff --git a/chromecast/crash/BUILD.gn b/chromecast/crash/BUILD.gn
@@ -38,6 +38,7 @@ source_set("crash") {
     "//components/crash/core/common",
     "//components/metrics",
     "//components/prefs",
+    "//gpu/config:crash_keys",
   ]
 
   if (chromecast_branding == "public") {

diff --git a/chromecast/crash/DEPS b/chromecast/crash/DEPS
@@ -1,4 +1,5 @@
 include_rules = [
   "+breakpad",
   "+components/crash/core/common",
+  "+gpu/config/gpu_crash_keys.h",
 ]
diff --git a/chromecast/crash/cast_crash_keys.cc b/chromecast/crash/cast_crash_keys.cc
@@ -5,6 +5,7 @@
 #include "chromecast/crash/cast_crash_keys.h"
 
 #include "components/crash/core/common/crash_keys.h"
+#include "gpu/config/gpu_crash_keys.h"
 
 namespace chromecast {
 namespace crash_keys {
@@ -31,9 +32,15 @@ size_t RegisterCastCrashKeys() {
       {"num-extensions", ::crash_keys::kSmallSize},
       {"shutdown-type", ::crash_keys::kSmallSize},
       {"browser-unpin-trace", ::crash_keys::kMediumSize},
-      {"gpu-driver", ::crash_keys::kSmallSize},
-      {"gpu-psver", ::crash_keys::kSmallSize},
-      {"gpu-vsver", ::crash_keys::kSmallSize},
+
+      // gpu
+      {gpu::crash_keys::kGPUDriverVersion, ::crash_keys::kSmallSize},
+      {gpu::crash_keys::kGPUPixelShaderVersion, ::crash_keys::kSmallSize},
+      {gpu::crash_keys::kGPUVertexShaderVersion, ::crash_keys::kSmallSize},
+      // Temporary for https://crbug.com/729483.
+      // TODO(sunnyps): Remove after https://crbug.com/729483 is fixed.
+      {gpu::crash_keys::kGpuChannelFilterTrace, ::crash_keys::kMediumSize},
+      {gpu::crash_keys::kMediaGpuChannelFilterTrace, ::crash_keys::kMediumSize},
 
       // content/:
       {"bad_message_reason", ::crash_keys::kSmallSize},

diff --git a/gpu/config/gpu_crash_keys.cc b/gpu/config/gpu_crash_keys.cc
@@ -19,5 +19,10 @@ const char kGPUGLVersion[] = "gpu-glver";
 const char kGPUVendor[] = "gpu-gl-vendor";
 const char kGPURenderer[] = "gpu-gl-renderer";
 #endif
+
+// TODO(sunnyps): Remove after https://crbug.com/729483 is fixed.
+const char kGpuChannelFilterTrace[] = "gpu-channel-filter-trace";
+const char kMediaGpuChannelFilterTrace[] = "media-gpu-channel-filter-trace";
+
 }  // namespace crash_keys
 }  // namespace gpu
diff --git a/gpu/config/gpu_crash_keys.h b/gpu/config/gpu_crash_keys.h
@@ -24,6 +24,12 @@ extern const char kGPUGLVersion[];
 extern const char kGPUVendor[];
 extern const char kGPURenderer[];
 #endif
+
+// TEMPORARY: Backtraces for gpu message filters to fix use-after-free.
+// TODO(sunnyps): Remove after https://crbug.com/729483 is fixed.
+extern const char kGpuChannelFilterTrace[];
+extern const char kMediaGpuChannelFilterTrace[];
+
 }  // namespace crash_keys
 }  // namespace gpu
 

diff --git a/gpu/ipc/service/BUILD.gn b/gpu/ipc/service/BUILD.gn
@@ -74,6 +74,7 @@ target(link_target_type, "ipc_service_sources") {
     "//gpu/command_buffer/common:common_sources",
     "//gpu/command_buffer/service:service_sources",
     "//gpu/config:config_sources",
+    "//gpu/config:crash_keys",
     "//gpu/ipc/common:ipc_common_sources",
   ]
   libs = []

diff --git a/gpu/ipc/service/gpu_channel.cc b/gpu/ipc/service/gpu_channel.cc
@@ -18,7 +18,8 @@
 #include "base/atomicops.h"
 #include "base/bind.h"
 #include "base/command_line.h"
-#include "base/debug/alias.h"
+#include "base/debug/crash_logging.h"
+#include "base/debug/stack_trace.h"
 #include "base/location.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/single_thread_task_runner.h"
@@ -37,6 +38,7 @@
 #include "gpu/command_buffer/service/mailbox_manager.h"
 #include "gpu/command_buffer/service/preemption_flag.h"
 #include "gpu/command_buffer/service/scheduler.h"
+#include "gpu/config/gpu_crash_keys.h"
 #include "gpu/ipc/common/gpu_messages.h"
 #include "gpu/ipc/service/gpu_channel_manager.h"
 #include "gpu/ipc/service/gpu_channel_manager_delegate.h"
@@ -588,7 +590,10 @@ GpuChannelMessageFilter::GpuChannelMessageFilter(
 }
 
 GpuChannelMessageFilter::~GpuChannelMessageFilter() {
-  DCHECK(!gpu_channel_);
+  // TODO(sunnyps): Remove once crbug.com/729483 has been resolved.
+  base::debug::SetCrashKeyToStackTrace(crash_keys::kGpuChannelFilterTrace,
+                                       base::debug::StackTrace());
+  CHECK(!gpu_channel_);
 }
 
 void GpuChannelMessageFilter::Destroy() {
@@ -614,8 +619,6 @@ void GpuChannelMessageFilter::RemoveRoute(int32_t route_id) {
 void GpuChannelMessageFilter::OnFilterAdded(IPC::Channel* channel) {
   // TODO(sunnyps): Remove once crbug.com/729483 has been resolved.
   CHECK(io_thread_checker_.CalledOnValidThread());
-  GpuChannelMessageFilter* alias_this = this;
-  base::debug::Alias(&alias_this);
 
   DCHECK(!ipc_channel_);
   ipc_channel_ = channel;
@@ -626,8 +629,6 @@ void GpuChannelMessageFilter::OnFilterAdded(IPC::Channel* channel) {
 void GpuChannelMessageFilter::OnFilterRemoved() {
   // TODO(sunnyps): Remove once crbug.com/729483 has been resolved.
   CHECK(io_thread_checker_.CalledOnValidThread());
-  GpuChannelMessageFilter* alias_this = this;
-  base::debug::Alias(&alias_this);
 
   for (scoped_refptr<IPC::MessageFilter>& filter : channel_filters_)
     filter->OnFilterRemoved();
@@ -638,8 +639,6 @@ void GpuChannelMessageFilter::OnFilterRemoved() {
 void GpuChannelMessageFilter::OnChannelConnected(int32_t peer_pid) {
   // TODO(sunnyps): Remove once crbug.com/729483 has been resolved.
   CHECK(io_thread_checker_.CalledOnValidThread());
-  GpuChannelMessageFilter* alias_this = this;
-  base::debug::Alias(&alias_this);
 
   DCHECK(peer_pid_ == base::kNullProcessId);
   peer_pid_ = peer_pid;
@@ -650,8 +649,6 @@ void GpuChannelMessageFilter::OnChannelConnected(int32_t peer_pid) {
 void GpuChannelMessageFilter::OnChannelError() {
   // TODO(sunnyps): Remove once crbug.com/729483 has been resolved.
   CHECK(io_thread_checker_.CalledOnValidThread());
-  GpuChannelMessageFilter* alias_this = this;
-  base::debug::Alias(&alias_this);
 
   for (scoped_refptr<IPC::MessageFilter>& filter : channel_filters_)
     filter->OnChannelError();
@@ -660,8 +657,6 @@ void GpuChannelMessageFilter::OnChannelError() {
 void GpuChannelMessageFilter::OnChannelClosing() {
   // TODO(sunnyps): Remove once crbug.com/729483 has been resolved.
   CHECK(io_thread_checker_.CalledOnValidThread());
-  GpuChannelMessageFilter* alias_this = this;
-  base::debug::Alias(&alias_this);
 
   for (scoped_refptr<IPC::MessageFilter>& filter : channel_filters_)
     filter->OnChannelClosing();
@@ -671,8 +666,6 @@ void GpuChannelMessageFilter::AddChannelFilter(
     scoped_refptr<IPC::MessageFilter> filter) {
   // TODO(sunnyps): Remove once crbug.com/729483 has been resolved.
   CHECK(io_thread_checker_.CalledOnValidThread());
-  GpuChannelMessageFilter* alias_this = this;
-  base::debug::Alias(&alias_this);
 
   channel_filters_.push_back(filter);
   if (ipc_channel_)
@@ -685,8 +678,6 @@ void GpuChannelMessageFilter::RemoveChannelFilter(
     scoped_refptr<IPC::MessageFilter> filter) {
   // TODO(sunnyps): Remove once crbug.com/729483 has been resolved.
   CHECK(io_thread_checker_.CalledOnValidThread());
-  GpuChannelMessageFilter* alias_this = this;
-  base::debug::Alias(&alias_this);
 
   if (ipc_channel_)
     filter->OnFilterRemoved();

diff --git a/media/gpu/ipc/service/BUILD.gn b/media/gpu/ipc/service/BUILD.gn
@@ -40,6 +40,7 @@ target(link_target_type, "service") {
     "//media/gpu",
   ]
   deps = [
+    "//gpu/config:crash_keys",
     "//gpu/ipc/service",
     "//media:media_features",
     "//media/gpu/ipc/common",

diff --git a/media/gpu/ipc/service/media_gpu_channel.cc b/media/gpu/ipc/service/media_gpu_channel.cc
@@ -4,8 +4,11 @@
 
 #include "media/gpu/ipc/service/media_gpu_channel.h"
 
+#include "base/debug/crash_logging.h"
+#include "base/debug/stack_trace.h"
 #include "base/single_thread_task_runner.h"
 #include "base/unguessable_token.h"
+#include "gpu/config/gpu_crash_keys.h"
 #include "gpu/ipc/service/gpu_channel.h"
 #include "ipc/message_filter.h"
 #include "media/gpu/ipc/common/media_messages.h"
@@ -72,7 +75,12 @@ class MediaGpuChannelFilter : public IPC::MessageFilter {
   }
 
  private:
-  ~MediaGpuChannelFilter() override {}
+  ~MediaGpuChannelFilter() override {
+    // TODO(sunnyps): Remove once crbug.com/729483 has been resolved.
+    base::debug::SetCrashKeyToStackTrace(
+        gpu::crash_keys::kMediaGpuChannelFilterTrace,
+        base::debug::StackTrace());
+  }
 
   IPC::Channel* channel_;
   base::UnguessableToken channel_token_;