Avoid a potential deadlock when using BridgeFromGoogleLogging

Summary:
Given that folly/logging could be used as LogSink in glog (ex - `folly::BridgeFromGoogleLogging`) there is a potential issue that could lead to a deadlock. An extreme example:

t0: LOG(FATAL) -> LogMessage::Flush() -> acquire Mutex -> BridgeFromGoogleLogging::send -> LogCategory::admitMessage -> std::abort() -> triggered signalHandler that waits for a thread t1

t1: LOG(INFO) -> LogMessage::Flush -> acquire Mutex -> deadlock

Reviewed By: andriigrynenko

Differential Revision: D68178188

fbshipit-source-id: 090bc848934663a8b1f8e96efa139dc442a99d60

third-party/folly/src/folly/logging/BridgeFromGoogleLogging.cpp

@@ -70,7 +70,9 @@ void BridgeFromGoogleLogging::send(
         static_cast<unsigned>(line),
         {},
         std::string{message, message_len}};
-    logger.getCategory()->admitMessage(logMessage);
+    // Make sure we don't abort on fatal messages and let glog library to
+    // handle it. As this call is done under lock, this could lead to a deadlock
+    logger.getCategory()->admitMessage(logMessage, /* skipAbortOnFatal */ true);
   }
 }
 

third-party/folly/src/folly/logging/LogCategory.cpp

@@ -47,7 +47,8 @@ LogCategory::LogCategory(StringPiece name, LogCategory* parent)
   parent_->firstChild_ = this;
 }
 
-void LogCategory::admitMessage(const LogMessage& message) const {
+void LogCategory::admitMessage(
+    const LogMessage& message, bool skipAbortOnFatal) const {
   processMessageWalker(this, message);
 
   // If this is a fatal message, flush the handlers to make sure the log
@@ -68,7 +69,9 @@ void LogCategory::admitMessage(const LogMessage& message) const {
           "\n");
       folly::writeFull(STDERR_FILENO, msg.data(), msg.size());
     }
-    std::abort();
+    if (!skipAbortOnFatal) {
+      std::abort();
+    }
   }
 }
 

third-party/folly/src/folly/logging/LogCategory.h

@@ -213,8 +213,16 @@ class LogCategory {
    *
    * This method generally should be invoked only through the logging macros,
    * rather than calling this directly.
+   *
+   * skipAbortOnFatal parameters provides more granular control on who
+   * is responsible for aborting process when calling the method directly.
+   * If skipAbortOnFatal is true, then this LogCategory will not trigger
+   * std::abort() if the LogMessage is fatal and the process should abort.
+   * This is necessary for the LoggerDB to handle fatal messages specially and
+   * only suitable for direct calls.
    */
-  void admitMessage(const LogMessage& message) const;
+  void admitMessage(
+      const LogMessage& message, bool skipAbortOnFatal = false) const;
 
   /**
    * Note: setLevelLocked() may only be called while holding the

third-party/folly/src/folly/logging/test/BridgeFromGoogleLoggingTest.cpp

@@ -70,3 +70,10 @@ TEST_F(BridgeFromGoogleLoggingTest, bridge) {
   EXPECT_EQ(LogLevel::INFO, messages[0].first.getLevel());
   messages.clear();
 }
+
+TEST_F(BridgeFromGoogleLoggingTest, bridgeFatal) {
+  auto handler = make_shared<TestLogHandler>();
+  logging::BridgeFromGoogleLogging test_bridge{};
+  LoggerDB::get().getCategory("")->addHandler(handler);
+  ASSERT_DEATH([] { LOG(FATAL) << "test crash"; }(), "");
+}