Reland "[Logging] Disable always-log-ERRORs-to-stderr unless LOG_TO_FILE is set."

This reverts commit c6b8b53b.

Reason for revert: Fixing the debugging issues.

Original change's description:
> Revert "[Logging] Disable always-log-ERRORs-to-stderr unless LOG_TO_FILE is set."
> 
> This reverts commit ffa05b99.
> 
> Reason for revert: This breaks a lot of debugging workflows.
> 
> Original change's description:
> > [Logging] Disable always-log-ERRORs-to-stderr unless LOG_TO_FILE is set.
> > 
> > logging::LogMessage has a special-case to write LOG_ERROR messages
> > to stderr, even if LOG_TO_STDERR is not enabled, to make ERROR and
> > FATAL messages more visible, especially in tests.
> > 
> > Restrict this behaviour to apply only if LOG_TO_FILE is set, so that
> > binaries which only LOG_TO_SYSTEM_DEBUG_LOG will never emit logspam
> > to stderr.
> > 
> > Tested by adding a unit test for this case.
> > 
> > Bug: 968387
> > Change-Id: Icfe094e7d7bf9697a70a589eab48edbe79c36091
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1638782
> > Commit-Queue: Sharon Yang <yangsharon@chromium.org>
> > Reviewed-by: Wez <wez@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#666507}
> 
> TBR=wez@chromium.org,kmarshall@chromium.org,yangsharon@chromium.org
> 
> Change-Id: I857fd9bc95afedf204a5e981e6a362cd8990b4a8
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 968387
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1647361
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Commit-Queue: Daniel Cheng <dcheng@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#666753}

TBR=dcheng@chromium.org,wez@chromium.org,kmarshall@chromium.org,yangsharon@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 968387
Change-Id: Id656bff53e3b53e4df981ca8e819f33900c966f4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1682954Reviewed-by: Will Harris <wfh@chromium.org>
Commit-Queue: Aidan Wolter <awolter@chromium.org>
Auto-Submit: Aidan Wolter <awolter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#677030}

base/logging.cc

@@ -482,13 +482,21 @@ bool ShouldCreateLogMessage(int severity) {
   if (severity < g_min_log_level)
     return false;
 
-  // Return true here unless we know ~LogMessage won't do anything. Note that
-  // ~LogMessage writes to stderr if severity_ >= kAlwaysPrintErrorLevel, even
-  // when g_logging_destination is LOG_NONE.
+  // Return true here unless we know ~LogMessage won't do anything.
   return g_logging_destination != LOG_NONE || log_message_handler ||
          severity >= kAlwaysPrintErrorLevel;
 }
 
+// Returns true when logging destination LOG_TO_STDERR flag is set. Also when
+// configured to only LOG_TO_FILE, continue to log higher-severity messages to
+// stderr as well to better detect and diagnose problems with unit tests,
+// especially on the buildbots.
+bool ShouldLogToStderr(int severity) {
+  return ((g_logging_destination & LOG_TO_STDERR) != 0) ||
+         (severity >= kAlwaysPrintErrorLevel &&
+          g_logging_destination == LOG_TO_FILE);
+}
+
 int GetVlogVerbosity() {
   return std::max(-1, LOG_INFO - GetMinLogLevel());
 }
@@ -850,11 +858,7 @@ LogMessage::~LogMessage() {
 #endif  // OS_FUCHSIA
   }
 
-  if ((g_logging_destination & LOG_TO_STDERR) != 0 ||
-      severity_ >= kAlwaysPrintErrorLevel) {
-    // Write logs with destination LOG_TO_STDERR to stderr. Also output to
-    // stderr for logs above a certain log level to better detect and diagnose
-    // problems with unit tests, especially on the buildbots.
+  if (ShouldLogToStderr(severity_)) {
     ignore_result(fwrite(str_newline.data(), str_newline.size(), 1, stderr));
     fflush(stderr);
   }

base/logging_unittest.cc

@@ -8,6 +8,8 @@
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/compiler_specific.h"
+#include "base/files/file_util.h"
+#include "base/files/scoped_temp_dir.h"
 #include "base/macros.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
@@ -236,6 +238,60 @@ TEST_F(LoggingTest, LogToStdErrFlag) {
   LOG(INFO) << mock_log_source_stderr.Log();
 }
 
+// Check that when logging to file and message severity is
+// kAlwaysPrintErrorLevel or higher that the message is also written to stderr.
+// When severity is lower, make sure the message is only written to file.
+// Relies on POSIX specific APIs. Though this feature is cross-platform, testing
+// on POSIX platforms is sufficient.
+#if defined(OS_POSIX) || defined(OS_FUCHSIA)
+TEST_F(LoggingTest, LogToFileAndStderr) {
+  const char kInfoLogMessage[] = "This is an INFO level message";
+  const char kErrorLogMessage[] = "Here we have a message of level ERROR";
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+
+  // Set up logging to log to a file.
+  base::FilePath file_logs_path = temp_dir.GetPath().Append("file.log");
+  LoggingSettings settings;
+  settings.logging_dest = LOG_TO_FILE;
+  settings.log_file = file_logs_path.value().c_str();
+  InitLogging(settings);
+
+  // Create a file and change stderr to write to that file, to easily check
+  // contents.
+  base::FilePath stderr_logs_path = temp_dir.GetPath().Append("stderr.log");
+  base::File stderr_logs = base::File(
+      stderr_logs_path,
+      base::File::FLAG_CREATE | base::File::FLAG_WRITE | base::File::FLAG_READ);
+  base::ScopedFD stderr_backup = base::ScopedFD(dup(STDERR_FILENO));
+  int dup_result = dup2(stderr_logs.GetPlatformFile(), STDERR_FILENO);
+  ASSERT_GE(dup_result, 0);
+
+  LOG(INFO) << kInfoLogMessage;
+  LOG(ERROR) << kErrorLogMessage;
+
+  // Restore the original stderr logging destination.
+  dup_result = dup2(stderr_backup.get(), STDERR_FILENO);
+  ASSERT_EQ(dup_result, STDERR_FILENO);
+
+  // Check that both log messages are written to "file.log".
+  std::string written_file_logs;
+  ASSERT_TRUE(base::ReadFileToString(file_logs_path, &written_file_logs));
+  std::size_t found_info_log = written_file_logs.find(kInfoLogMessage);
+  EXPECT_FALSE(found_info_log == std::string::npos);
+  std::size_t found_error_log = written_file_logs.find(kErrorLogMessage);
+  EXPECT_FALSE(found_error_log == std::string::npos);
+
+  // Check that the kErrorLogMessage is written to |stderr.log| and
+  // kInfoLogMessage is not.
+  ASSERT_TRUE(base::ReadFileToString(stderr_logs_path, &written_file_logs));
+  found_info_log = written_file_logs.find(kInfoLogMessage);
+  EXPECT_TRUE(found_info_log == std::string::npos);
+  found_error_log = written_file_logs.find(kErrorLogMessage);
+  EXPECT_FALSE(found_error_log == std::string::npos);
+}
+#endif
+
 // Official builds have CHECKs directly call BreakDebugger.
 #if !defined(OFFICIAL_BUILD)