Add checks to font proxy for diagnostics.

content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc

Index: content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc
diff --git a/content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc b/content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc
index 5d186424b6621683defd82868e51c9e700c3b347..e793d6a0ab2dc32c1c48ae4b5471f69a9229617f 100644
--- a/content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc
+++ b/content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc
@@ -71,6 +71,7 @@ base::string16 GetWindowsFontsPath() {
                                        font_path_chars.data(), CSIDL_FONTS,
                                        FALSE /* fCreate */);
   DCHECK(result);
+  CHECK(result);
   return base::i18n::FoldCase(font_path_chars.data());
 }
 
@@ -142,13 +143,20 @@ void DWriteFontProxyMessageFilter::OnGetFamilyNames(
 
   mswr::ComPtr<IDWriteFontFamily> family;
   HRESULT hr = collection_->GetFontFamily(family_index, &family);
-  if (!SUCCEEDED(hr))
+  if (!SUCCEEDED(hr)) {
+    // Crash only if index was not out of bounds.
+    CHECK(family_index >= collection_->GetFontFamilyCount());

[ananta, 2016/03/16 20:22:25]

You should use base Alias here and below to ensure that the local variables you
want to monitor in the crashes are not optimized out.

     return;
+  }
+  CHECK(family);
 
   mswr::ComPtr<IDWriteLocalizedStrings> localized_names;
   hr = family->GetFamilyNames(&localized_names);
-  if (!SUCCEEDED(hr))
+  if (!SUCCEEDED(hr)) {
+    CHECK(false);
     return;
+  }
+  CHECK(localized_names);
 
   size_t string_count = localized_names->GetCount();
 
@@ -157,25 +165,33 @@ void DWriteFontProxyMessageFilter::OnGetFamilyNames(
   for (size_t index = 0; index < string_count; ++index) {
     UINT32 length = 0;
     hr = localized_names->GetLocaleNameLength(index, &length);
-    if (!SUCCEEDED(hr))
+    if (!SUCCEEDED(hr)) {
+      CHECK(false);
       return;
+    }
     ++length;  // Reserve space for the null terminator.
     locale.resize(length);
     hr = localized_names->GetLocaleName(index, locale.data(), length);
-    if (!SUCCEEDED(hr))
+    if (!SUCCEEDED(hr)) {
+      CHECK(false);
       return;
-    DCHECK_EQ(L'\0', locale[length - 1]);
+    }
+    CHECK_EQ(L'\0', locale[length - 1]);
 
     length = 0;
     hr = localized_names->GetStringLength(index, &length);
-    if (!SUCCEEDED(hr))
+    if (!SUCCEEDED(hr)) {
+      CHECK(false);
       return;
+    }
     ++length;  // Reserve space for the null terminator.
     name.resize(length);
     hr = localized_names->GetString(index, name.data(), length);
-    if (!SUCCEEDED(hr))
+    if (!SUCCEEDED(hr)) {
+      CHECK(false);
       return;
-    DCHECK_EQ(L'\0', name[length - 1]);
+    }
+    CHECK_EQ(L'\0', name[length - 1]);
 
     // Would be great to use emplace_back instead.
     family_names->push_back(std::pair<base::string16, base::string16>(
@@ -194,8 +210,11 @@ void DWriteFontProxyMessageFilter::OnGetFontFiles(
 
   mswr::ComPtr<IDWriteFontFamily> family;
   HRESULT hr = collection_->GetFontFamily(family_index, &family);
-  if (!SUCCEEDED(hr))
+  if (!SUCCEEDED(hr)) {
+    // Crash only if index was not out of bounds.
+    CHECK(family_index >= collection_->GetFontFamilyCount());
     return;
+  }
 
   UINT32 font_count = family->GetFontCount();
 
@@ -206,8 +225,11 @@ void DWriteFontProxyMessageFilter::OnGetFontFiles(
   for (UINT32 font_index = 0; font_index < font_count; ++font_index) {
     mswr::ComPtr<IDWriteFont> font;
     hr = family->GetFont(font_index, &font);
-    if (!SUCCEEDED(hr))
+    if (!SUCCEEDED(hr)) {
+      CHECK(false);
       return;
+    }
+    CHECK(font);
 
     AddFilesForFont(&path_set, font.Get());
   }
@@ -224,13 +246,15 @@ void DWriteFontProxyMessageFilter::InitializeDirectWrite() {
   mswr::ComPtr<IDWriteFactory> factory;
   gfx::win::CreateDWriteFactory(&factory);
   if (factory == nullptr) {
+    CHECK(false);
     // We won't be able to load fonts, but we should still return messages so
     // renderers don't hang if they for some reason send us a font message.
     return;
   }
 
   HRESULT hr = factory->GetSystemFontCollection(&collection_);
-  DCHECK(SUCCEEDED(hr));
+  CHECK(SUCCEEDED(hr));
+  CHECK(collection_);
 }
 
 bool DWriteFontProxyMessageFilter::AddFilesForFont(
@@ -239,26 +263,34 @@ bool DWriteFontProxyMessageFilter::AddFilesForFont(
   mswr::ComPtr<IDWriteFontFace> font_face;
   HRESULT hr;
   hr = font->CreateFontFace(&font_face);
-  if (!SUCCEEDED(hr))
+  if (!SUCCEEDED(hr)) {
+    CHECK(false);
     return false;
+  }
 
   UINT32 file_count;
   hr = font_face->GetFiles(&file_count, nullptr);
-  if (!SUCCEEDED(hr))
+  if (!SUCCEEDED(hr)) {
+    CHECK(false);
     return false;
+  }
 
   std::vector<mswr::ComPtr<IDWriteFontFile>> font_files;
   font_files.resize(file_count);
   hr = font_face->GetFiles(
       &file_count, reinterpret_cast<IDWriteFontFile**>(font_files.data()));
-  if (!SUCCEEDED(hr))
+  if (!SUCCEEDED(hr)) {
+    CHECK(false);
     return false;
+  }
 
   for (unsigned int file_index = 0; file_index < file_count; ++file_index) {
     mswr::ComPtr<IDWriteFontFileLoader> loader;
     hr = font_files[file_index]->GetLoader(&loader);
-    if (!SUCCEEDED(hr))
+    if (!SUCCEEDED(hr)) {
+      CHECK(false);
       return false;
+    }
 
     mswr::ComPtr<IDWriteLocalFontFileLoader> local_loader;
     hr = loader.CopyTo(local_loader.GetAddressOf());  // QueryInterface.
@@ -271,12 +303,18 @@ bool DWriteFontProxyMessageFilter::AddFilesForFont(
       // for this font, forcing blink/skia to fall back to whatever font is
       // next). If we get telemetry indicating that this case actually
       // happens, we can implement this by exposing the loader via ipc. That
-      // will likely by loading the font data into shared memory, although we
-      // could proxy the stream reads directly instead.
+      // will likely be by loading the font data into shared memory, although
+      // we could proxy the stream reads directly instead.
       LogLoaderType(OTHER_LOADER);
       DCHECK(false);
+
+      // UMA indicates zero results for loaderType=OTHER_LOADER, so doing this
+      // CHECK should be safe to rule out this case as a possibility.
+      CHECK(false);
+
       return false;
     } else if (!SUCCEEDED(hr)) {
+      CHECK(false);
       return false;
     }
 
@@ -296,20 +334,26 @@ bool DWriteFontProxyMessageFilter::AddLocalFile(
   const void* key;
   UINT32 key_size;
   hr = font_file->GetReferenceKey(&key, &key_size);
-  if (!SUCCEEDED(hr))
+  if (!SUCCEEDED(hr)) {
+    CHECK(false);
     return false;
+  }
 
   UINT32 path_length = 0;
   hr = local_loader->GetFilePathLengthFromKey(key, key_size, &path_length);
-  if (!SUCCEEDED(hr))
+  if (!SUCCEEDED(hr)) {
+    CHECK(false);
     return false;
+  }
   ++path_length;  // Reserve space for the null terminator.
   std::vector<base::char16> file_path_chars;
   file_path_chars.resize(path_length);
   hr = local_loader->GetFilePathFromKey(key, key_size, file_path_chars.data(),
                                         path_length);
-  if (!SUCCEEDED(hr))
+  if (!SUCCEEDED(hr)) {
+    CHECK(false);
     return false;
+  }
 
   base::string16 file_path = base::i18n::FoldCase(file_path_chars.data());
   if (!base::StartsWith(file_path, windows_fonts_path_,
@@ -319,6 +363,12 @@ bool DWriteFontProxyMessageFilter::AddLocalFile(
     // this turns out to be a common case, we can either grant the renderer
     // access to these files (not sure if this is actually possible), or
     // load the file data ourselves and hand it to the renderer.
+
+    // Really, really, really want to know what families hit this. Current
+    // data indicates about 0.09% of families fall into this case. Nothing to
+    // worry about if it's random obscure fonts noone has ever heard of, but
+    // could be a problem if it's common fonts.
+
     LogLoaderType(FILE_OUTSIDE_SANDBOX);
     NOTREACHED();  // Not yet implemented.
     return false;