[gjs: 5/10] atoms: Allow "symbol atoms" for keys not exposed to JS

[Philip Chimento <pchimento src gnome org>]

commit 5c1a24ad2fb1ca5249eb96c6ce75e771160cb472
Author: Philip Chimento <philip chimento gmail com>
Date:   Wed Sep 5 14:08:24 2018 -0400

    atoms: Allow "symbol atoms" for keys not exposed to JS
    
    There are a few places where we want to have property keys that are
    well-known internally, but not available to JS code. We create "symbol
    atoms" for these, where you can get a jsid from the GjsAtoms object, but
    it refers to a JS::Symbol instead of a string.
    
    Previously we had two instances of this, and both were implemented in a
    different way. For "__gjsPrivateNS" we just used a string and hoped that
    nobody used that string in client code. For "__GObject__hook_up_vfunc" we
    did create a JS::Symbol and had some complicated static member in
    ObjectInstance to expose it to imports._gi. Both of these implementations
    are simplified with this patch.

 gi/object.cpp   | 20 ++------------------
 gi/object.h     |  4 ----
 gi/private.cpp  |  4 +++-
 gjs/atoms.cpp   | 16 ++++++++++++++++
 gjs/atoms.h     |  8 +++++++-
 gjs/context.cpp |  2 ++
 6 files changed, 30 insertions(+), 24 deletions(-)
---
diff --git a/gi/object.cpp b/gi/object.cpp
index 9b081834..fcc58be4 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -72,7 +72,6 @@ static_assert(sizeof(ObjectInstance) <= 88,
 std::stack<JS::PersistentRootedObject> ObjectInstance::object_init_list{};
 static bool weak_pointer_callback = false;
 ObjectInstance *ObjectInstance::wrapped_gobject_list = nullptr;
-JS::PersistentRootedSymbol ObjectInstance::hook_up_vfunc_root;
 
 extern struct JSClass gjs_object_instance_class;
 GJS_DEFINE_PRIV_FROM_JS(ObjectBase, gjs_object_instance_class)
@@ -2149,19 +2148,6 @@ gjs_object_define_static_methods(JSContext       *context,
     return true;
 }
 
-JS::Symbol*
-ObjectInstance::hook_up_vfunc_symbol(JSContext *cx)
-{
-    if (!hook_up_vfunc_root.initialized()) {
-        JS::RootedString descr(cx,
-            JS_NewStringCopyZ(cx, "__GObject__hook_up_vfunc"));
-        if (!descr)
-            g_error("Out of memory defining internal symbol");
-        hook_up_vfunc_root.init(cx, JS::NewSymbol(cx, descr));
-    }
-    return hook_up_vfunc_root;
-}
-
 bool
 gjs_define_object_class(JSContext              *context,
                         JS::HandleObject        in_object,
@@ -2245,9 +2231,8 @@ gjs_define_object_class(JSContext              *context,
 
     /* Hook_up_vfunc can't be included in gjs_object_instance_proto_funcs
      * because it's a custom symbol. */
-    JS::RootedId hook_up_vfunc(context,
-        SYMBOL_TO_JSID(ObjectInstance::hook_up_vfunc_symbol(context)));
-    if (!JS_DefineFunctionById(context, prototype, hook_up_vfunc,
+    const GjsAtoms& atoms = GjsContextPrivate::atoms(context);
+    if (!JS_DefineFunctionById(context, prototype, atoms.hook_up_vfunc(),
                                &ObjectBase::hook_up_vfunc, 3,
                                GJS_MODULE_PROP_FLAGS))
         return false;
@@ -2270,7 +2255,6 @@ gjs_define_object_class(JSContext              *context,
     if (!gtype_obj)
         return false;
 
-    const GjsAtoms& atoms = GjsContextPrivate::atoms(context);
     return JS_DefinePropertyById(context, constructor, atoms.gtype(), gtype_obj,
                                  JSPROP_PERMANENT);
 }
diff --git a/gi/object.h b/gi/object.h
index 536aea87..cd5d9a58 100644
--- a/gi/object.h
+++ b/gi/object.h
@@ -508,13 +508,9 @@ class ObjectInstance : public ObjectBase {
                    JS::MutableHandleObject obj);
 
     /* Methods connected to "public" API */
- private:
-    static JS::PersistentRootedSymbol hook_up_vfunc_root;
-
  public:
     GJS_USE
     bool typecheck_object(JSContext* cx, GType expected_type, bool throw_error);
-    static JS::Symbol* hook_up_vfunc_symbol(JSContext* cx);
 
     /* Notification callbacks */
 
diff --git a/gi/private.cpp b/gi/private.cpp
index 5c938e7d..5a99dd7c 100644
--- a/gi/private.cpp
+++ b/gi/private.cpp
@@ -26,6 +26,7 @@
 
 #include <unordered_map>
 
+#include "gjs/context-private.h"
 #include "gjs/jsapi-util-args.h"
 #include "gjs/jsapi-util.h"
 #include "gjs/jsapi-wrapper.h"
@@ -406,7 +407,8 @@ GJS_JSAPI_RETURN_CONVENTION
 static bool hook_up_vfunc_symbol_getter(JSContext* cx, unsigned argc,
                                         JS::Value* vp) {
     JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
-    args.rval().setSymbol(ObjectInstance::hook_up_vfunc_symbol(cx));
+    const GjsAtoms& atoms = GjsContextPrivate::atoms(cx);
+    args.rval().setSymbol(JSID_TO_SYMBOL(atoms.hook_up_vfunc()));
     return true;
 }
 
diff --git a/gjs/atoms.cpp b/gjs/atoms.cpp
index 147bddef..799840d3 100644
--- a/gjs/atoms.cpp
+++ b/gjs/atoms.cpp
@@ -35,9 +35,25 @@ GjsAtoms::GjsAtoms(JSContext* cx) {
 #undef INITIALIZE_ATOM
 }
 
+/* Requires a current compartment. Unlike the atoms initialized in the
+ * constructor, this can GC, so it needs to be done after the tracing has been
+ * set up. */
+void GjsAtoms::init_symbols(JSContext* cx) {
+    JS::RootedString descr(cx);
+    JS::Symbol* symbol;
+
+#define INITIALIZE_SYMBOL_ATOM(identifier, str) \
+    descr = JS_AtomizeAndPinString(cx, str);    \
+    symbol = JS::NewSymbol(cx, descr);          \
+    m_##identifier = SYMBOL_TO_JSID(symbol);
+    FOR_EACH_SYMBOL_ATOM(INITIALIZE_SYMBOL_ATOM)
+#undef INITIALIZE_SYMBOL_ATOM
+}
+
 void GjsAtoms::trace(JSTracer* trc) {
 #define TRACE_ATOM(identifier, str) \
     JS::TraceEdge<jsid>(trc, &m_##identifier, "Atom " str);
     FOR_EACH_ATOM(TRACE_ATOM)
+    FOR_EACH_SYMBOL_ATOM(TRACE_ATOM)
 #undef TRACE_ATOM
 }
diff --git a/gjs/atoms.h b/gjs/atoms.h
index 0e648e88..69ef49ba 100644
--- a/gjs/atoms.h
+++ b/gjs/atoms.h
@@ -56,7 +56,6 @@
     macro(overrides, "overrides") \
     macro(param_spec, "ParamSpec") \
     macro(parent_module, "__parentModule__") \
-    macro(private_ns_marker, "__gjsPrivateNS") \
     macro(program_invocation_name, "programInvocationName") \
     macro(prototype, "prototype") \
     macro(search_path, "searchPath") \
@@ -68,15 +67,21 @@
     macro(window, "window") \
     macro(x, "x") \
     macro(y, "y")
+
+#define FOR_EACH_SYMBOL_ATOM(macro) \
+    macro(hook_up_vfunc, "__GObject__hook_up_vfunc") \
+    macro(private_ns_marker, "__gjsPrivateNS")
 // clang-format on
 
 class GjsAtoms {
 #define DECLARE_ATOM_MEMBER(identifier, str) JS::Heap<jsid> m_##identifier;
     FOR_EACH_ATOM(DECLARE_ATOM_MEMBER)
+    FOR_EACH_SYMBOL_ATOM(DECLARE_ATOM_MEMBER)
 #undef DECLARE_ATOM_MEMBER
 
  public:
     explicit GjsAtoms(JSContext* cx);
+    void init_symbols(JSContext* cx);
 
     void trace(JSTracer* trc);
 
@@ -87,6 +92,7 @@ class GjsAtoms {
         return JS::HandleId::fromMarkedLocation(&m_##identifier.get()); \
     }
     FOR_EACH_ATOM(DECLARE_ATOM_ACCESSOR)
+    FOR_EACH_SYMBOL_ATOM(DECLARE_ATOM_ACCESSOR)
 #undef DECLARE_ATOM_ACCESSOR
 };
 
diff --git a/gjs/context.cpp b/gjs/context.cpp
index 8096cd3f..999279c4 100644
--- a/gjs/context.cpp
+++ b/gjs/context.cpp
@@ -462,6 +462,8 @@ GjsContextPrivate::GjsContextPrivate(JSContext* cx, GjsContext* public_context)
     m_global = global;
     JS_AddExtraGCRootsTracer(m_cx, &GjsContextPrivate::trace, this);
 
+    m_atoms.init_symbols(m_cx);
+
     JS::RootedObject importer(m_cx,
                               gjs_create_root_importer(m_cx, m_search_path));
     if (!importer) {