[gjs: 1/2] system: Use RAII in System.dumpHeap and logging for file pointer

[Philip Chimento <pchimento src gnome org>]

commit 37ee770f9be8d664d11124098056bba8d5750410
Author: Philip Chimento <philip chimento gmail com>
Date:   Tue Mar 16 21:29:51 2021 -0700

    system: Use RAII in System.dumpHeap and logging for file pointer
    
    Avoids accessing the file handle after it is destructed.
    Avoids duplicating the js::DumpHeap() call and will be used later in
    System.dumpMemoryInfo() as well, in GNOME/gjs#292.
    
    Closes: #410

 modules/system.cpp | 21 +++++++--------------
 util/log.cpp       | 19 ++++++++-----------
 util/misc.h        | 40 +++++++++++++++++++++++++++++++++++++++-
 3 files changed, 54 insertions(+), 26 deletions(-)
---
diff --git a/modules/system.cpp b/modules/system.cpp
index f5e0340b..29eb7770 100644
--- a/modules/system.cpp
+++ b/modules/system.cpp
@@ -5,9 +5,6 @@
 
 #include <config.h>  // for GJS_VERSION
 
-#include <errno.h>
-#include <stdio.h>   // for FILE, fclose, stdout
-#include <string.h>  // for strerror
 #include <time.h>    // for tzset
 
 #include <glib-object.h>
@@ -31,6 +28,7 @@
 #include "gjs/jsapi-util.h"
 #include "modules/system.h"
 #include "util/log.h"
+#include "util/misc.h"  // for LogFile
 
 /* Note that this cannot be relied on to test whether two objects are the same!
  * SpiderMonkey can move objects around in memory during garbage collection,
@@ -124,18 +122,13 @@ gjs_dump_heap(JSContext *cx,
     if (!gjs_parse_call_args(cx, "dumpHeap", args, "|F", "filename", &filename))
         return false;
 
-    if (filename) {
-        FILE *fp = fopen(filename, "a");
-        if (!fp) {
-            gjs_throw(cx, "Cannot dump heap to %s: %s", filename.get(),
-                      strerror(errno));
-            return false;
-        }
-        js::DumpHeap(cx, fp, js::CollectNurseryBeforeDump);
-        fclose(fp);
-    } else {
-        js::DumpHeap(cx, stdout, js::CollectNurseryBeforeDump);
+    LogFile file(filename);
+    if (file.has_error()) {
+        gjs_throw(cx, "Cannot dump heap to %s: %s", filename.get(),
+                  file.errmsg());
+        return false;
     }
+    js::DumpHeap(cx, file.fp(), js::CollectNurseryBeforeDump);
 
     gjs_debug(GJS_DEBUG_CONTEXT, "Heap dumped to %s",
               filename ? filename.get() : "stdout");
diff --git a/util/log.cpp b/util/log.cpp
index 93068a3b..3f5b52d4 100644
--- a/util/log.cpp
+++ b/util/log.cpp
@@ -3,6 +3,7 @@
 // SPDX-FileCopyrightText: 2008 litl, LLC
 
 #include <atomic>  // for atomic_bool
+#include <memory>  // for unique_ptr
 #include <string>  // for string
 #include <type_traits>  // for remove_reference<>::type
 
@@ -32,7 +33,7 @@
 static std::atomic_bool s_initialized = ATOMIC_VAR_INIT(false);
 static bool s_debug_log_enabled = false;
 static bool s_print_thread = false;
-static FILE* s_logfp = nullptr;
+static std::unique_ptr<LogFile> s_log_file;
 static GjsAutoPointer<GTimer, GTimer, g_timer_destroy> s_timer;
 static std::vector<bool> s_enabled_topics;
 
@@ -131,16 +132,17 @@ void gjs_log_init() {
         }
 
         /* avoid truncating in case we're using shared logfile */
-        s_logfp = fopen(log_file.c_str(), "a");
-        if (!s_logfp)
+        s_log_file = std::make_unique<LogFile>(log_file.c_str());
+        if (s_log_file->has_error()) {
             fprintf(stderr, "Failed to open log file `%s': %s\n",
                     log_file.c_str(), g_strerror(errno));
+        }
 
         s_debug_log_enabled = true;
     }
 
-    if (!s_logfp)
-        s_logfp = stderr;
+    if (!s_log_file)
+        s_log_file = std::make_unique<LogFile>(nullptr, stderr);
 
     if (s_debug_log_enabled) {
         auto* topics = g_getenv("GJS_DEBUG_TOPICS");
@@ -160,11 +162,6 @@ void gjs_log_cleanup() {
     if (!s_initialized.compare_exchange_strong(expected, false))
         return;
 
-    if (s_logfp && s_logfp != stderr) {
-        fclose(s_logfp);
-        s_logfp = nullptr;
-    }
-
     s_timer = nullptr;
     s_enabled_topics.clear();
 }
@@ -231,7 +228,7 @@ gjs_debug(GjsDebugTopic topic,
         s = s2;
     }
 
-    write_to_stream(s_logfp, topic_to_prefix(topic), s);
+    write_to_stream(s_log_file->fp(), topic_to_prefix(topic), s);
 
     g_free(s);
 }
diff --git a/util/misc.h b/util/misc.h
index b9b5b284..4c7cde3a 100644
--- a/util/misc.h
+++ b/util/misc.h
@@ -5,7 +5,8 @@
 #ifndef UTIL_MISC_H_
 #define UTIL_MISC_H_
 
-#include <stdlib.h>  // for size_t
+#include <errno.h>
+#include <stdio.h>  // for FILE, stdout
 #include <string.h>  // for memcpy
 
 #include <glib.h>  // for g_malloc
@@ -42,4 +43,41 @@ static inline void* _gjs_memdup2(const void* mem, size_t byte_size) {
     return new_mem;
 }
 
+/*
+ * LogFile:
+ * RAII class encapsulating access to a FILE* pointer that must be closed,
+ * unless it is an already-open fallback file such as stdout or stderr.
+ */
+class LogFile {
+    FILE* m_fp;
+    const char* m_errmsg;
+    bool m_should_close : 1;
+
+    LogFile(const LogFile&) = delete;
+    LogFile& operator=(const LogFile&) = delete;
+
+ public:
+    explicit LogFile(const char* filename, FILE* fallback_fp = stdout)
+        : m_errmsg(nullptr), m_should_close(false) {
+        if (filename) {
+            m_fp = fopen(filename, "a");
+            if (!m_fp)
+                m_errmsg = strerror(errno);
+            else
+                m_should_close = true;
+        } else {
+            m_fp = fallback_fp;
+        }
+    }
+
+    ~LogFile() {
+        if (m_should_close)
+            fclose(m_fp);
+    }
+
+    FILE* fp() { return m_fp; }
+    bool has_error() { return !!m_errmsg; }
+    const char* errmsg() { return m_errmsg; }
+};
+
 #endif  // UTIL_MISC_H_