Try capturing incorrect re-entrancy into DWriteFontProxy

One new theory of why DWriteFontProxy comes back with empty FontHandles
is as follows: Suppose the renderer process calls an asynchronous Mojo
method A, with callback b and continues execution. Then it reaches one
of the DWriteFontProxy methods, let's call it F, and calls F(). Then
Mojo goes into a blocking loop waiting for responses. Normally it would
just receive the response to F() and continue execution where the font
call left of. However, if A is in flight, there might be a situation in
which the Mojo loop receives the response for A(), then calls callback
b() and the renderer continues to execute in a different spot, until it
reaches another DWriteFontProxy call F2() and calls it synchronously
while a previous one is in flight, which would probably break.

This CL is intended to detect this situation with setting a thread local
boolean while the Mojo DWriteFontProxy call is in flight, and crashing
with an assertion failure if another one is issued while the first
response was not received and processed.

Bug: 561873
Change-Id: I15ad4e3484fec8b793206b1e92f319e58aee1cfd
Reviewed-on: https://chromium-review.googlesource.com/1140497
Commit-Queue: Dominik Röttsches <drott@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576518}

content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc

@@ -101,7 +101,8 @@ HRESULT DWriteFontCollectionProxy::FindFamilyName(const WCHAR* family_name,
     return S_OK;
   }
 
-  if (!GetFontProxy().FindFamily(name, &family_index)) {
+  if (!GetFontProxyScopeWrapper().GetFontProxy().FindFamily(name,
+                                                            &family_index)) {
     LogFontProxyError(FIND_FAMILY_SEND_FAILED);
     return E_FAIL;
   }
@@ -145,7 +146,8 @@ UINT32 DWriteFontCollectionProxy::GetFontFamilyCount() {
   TRACE_EVENT0("dwrite", "FontProxy::GetFontFamilyCount");
 
   uint32_t family_count = 0;
-  if (!GetFontProxy().GetFamilyCount(&family_count)) {
+  if (!GetFontProxyScopeWrapper().GetFontProxy().GetFamilyCount(
+          &family_count)) {
     LogFontProxyError(GET_FAMILY_COUNT_SEND_FAILED);
     return 0;
   }
@@ -197,7 +199,8 @@ HRESULT DWriteFontCollectionProxy::CreateEnumeratorFromKey(
 
   std::vector<base::FilePath> file_names;
   std::vector<base::File> file_handles;
-  if (!GetFontProxy().GetFontFiles(*family_index, &file_names, &file_handles)) {
+  if (!GetFontProxyScopeWrapper().GetFontProxy().GetFontFiles(
+          *family_index, &file_names, &file_handles)) {
     LogFontProxyError(GET_FONT_FILES_SEND_FAILED);
     return E_FAIL;
   }
@@ -318,7 +321,8 @@ bool DWriteFontCollectionProxy::LoadFamilyNames(
   TRACE_EVENT0("dwrite", "FontProxy::LoadFamilyNames");
 
   std::vector<mojom::DWriteStringPairPtr> pairs;
-  if (!GetFontProxy().GetFamilyNames(family_index, &pairs)) {
+  if (!GetFontProxyScopeWrapper().GetFontProxy().GetFamilyNames(family_index,
+                                                                &pairs)) {
     return false;
   }
   std::vector<std::pair<base::string16, base::string16>> strings;
@@ -360,7 +364,7 @@ void DWriteFontCollectionProxy::SetProxy(mojom::DWriteFontProxyPtrInfo proxy) {
                             {base::WithBaseSyncPrimitives()}));
 }
 
-mojom::DWriteFontProxy& DWriteFontCollectionProxy::GetFontProxy() {
+FontProxyScopeWrapper DWriteFontCollectionProxy::GetFontProxyScopeWrapper() {
   if (!font_proxy_) {
     mojom::DWriteFontProxyPtrInfo dwrite_font_proxy;
     if (main_task_runner_->RunsTasksInCurrentSequence()) {
@@ -377,7 +381,9 @@ mojom::DWriteFontProxy& DWriteFontCollectionProxy::GetFontProxy() {
     }
     SetProxy(std::move(dwrite_font_proxy));
   }
-  return **font_proxy_;
+  static base::ThreadLocalBoolean font_proxy_method_in_flight;
+
+  return FontProxyScopeWrapper(font_proxy_.get(), &font_proxy_method_in_flight);
 }
 
 DWriteFontFamilyProxy::DWriteFontFamilyProxy() = default;

content/child/dwrite_font_proxy/dwrite_font_proxy_win.h

@@ -22,6 +22,39 @@ namespace content {
 
 class DWriteFontFamilyProxy;
 
+class FontProxyScopeWrapper {
+ public:
+  FontProxyScopeWrapper(mojom::ThreadSafeDWriteFontProxyPtr* font_proxy,
+                        base::ThreadLocalBoolean* is_in_flight)
+      : font_proxy_(font_proxy), is_in_flight_(is_in_flight) {
+    // TODO(crbug.com/561873): Turn this into a DCHECK once instances of this
+    // CHECK have been found in crash reports and the referenced bug has been
+    // root-caused.
+    CHECK(!is_in_flight_->Get());
+    is_in_flight_->Set(true);
+  }
+
+  ~FontProxyScopeWrapper() {
+    // TODO(crbug.com/561873): Turn this into a DCHECK once instances of this
+    // CHECK have been found in crash reports and the referenced bug has been
+    // root-caused.
+    CHECK(is_in_flight_->Get());
+    is_in_flight_->Set(false);
+  }
+
+  content::mojom::DWriteFontProxy& GetFontProxy() const {
+    return **font_proxy_;
+  }
+
+  FontProxyScopeWrapper(FontProxyScopeWrapper&&) = default;
+  FontProxyScopeWrapper& operator=(FontProxyScopeWrapper&&) = default;
+
+ private:
+  mojom::ThreadSafeDWriteFontProxyPtr* font_proxy_;
+  base::ThreadLocalBoolean* is_in_flight_;
+  DISALLOW_COPY_AND_ASSIGN(FontProxyScopeWrapper);
+};
+
 // Implements a DirectWrite font collection that uses IPC to the browser to do
 // font enumeration. If a matching family is found, it will be loaded locally
 // into a custom font collection.
@@ -87,7 +120,7 @@ class DWriteFontCollectionProxy
 
   bool CreateFamily(UINT32 family_index);
 
-  mojom::DWriteFontProxy& GetFontProxy();
+  FontProxyScopeWrapper GetFontProxyScopeWrapper();
 
  private:
   void SetProxy(mojom::DWriteFontProxyPtrInfo);

content/child/dwrite_font_proxy/font_fallback_win.cc

@@ -100,7 +100,7 @@ HRESULT FontFallback::MapCharacters(IDWriteTextAnalysisSource* source,
 
   mojom::MapCharactersResultPtr result;
 
-  if (!GetFontProxy().MapCharacters(
+  if (!GetFontProxyScopeWrapper().GetFontProxy().MapCharacters(
           text_chunk,
           mojom::DWriteFontStyle::New(base_weight, base_style, base_stretch),
           locale, source->GetParagraphReadingDirection(), base_family_name,
@@ -222,8 +222,8 @@ void FontFallback::AddCachedFamily(
     family_list.pop_back();
 }
 
-mojom::DWriteFontProxy& FontFallback::GetFontProxy() {
-  return collection_->GetFontProxy();
+FontProxyScopeWrapper FontFallback::GetFontProxyScopeWrapper() {
+  return collection_->GetFontProxyScopeWrapper();
 }
 
 }  // namespace content

content/child/dwrite_font_proxy/font_fallback_win.h

@@ -65,7 +65,7 @@ class FontFallback
                        const wchar_t* base_family_name);
 
  private:
-  mojom::DWriteFontProxy& GetFontProxy();
+  FontProxyScopeWrapper GetFontProxyScopeWrapper();
 
   Microsoft::WRL::ComPtr<DWriteFontCollectionProxy> collection_;