Storing the global js object in JS::Heap<T> provokes crash in JS_DestroyRuntime if still in scope

Question

After learning from this question, I'm trying to store SpiderMonkey's global object on the heap. This seems to work as long as it goes out of scope before JS_DestroyRuntime is called. In my code however, the global object is a class member and thus cannot go out of scope before the runtime is destroyed in the class's destructor.

Unfortunately, this leads to the Monkey crashing in JS::~Heap with the following stack trace:

1  js::gc::Cell::storeBuffer() const                                                    Heap.h       1339 0x10004f905 
2  JSObject::writeBarrierPost(void *, JSObject *, JSObject *)                           jsobj.h      655  0x1000a6fc8 
3  js::InternalGCMethods<JSObject *>::postBarrier(JSObject * *, JSObject *, JSObject *) Barrier.h    254  0x1000a6df0 
4  JS::HeapObjectPostBarrier(JSObject * *, JSObject *, JSObject *)                      Barrier.cpp  173  0x100bc1636 
5  js::GCMethods<JSObject *>::postBarrier(JSObject * *, JSObject *, JSObject *)         RootingAPI.h 551  0x100003065 
6  JS::Heap<JSObject *>::post(JSObject * const&, JSObject * const&)                     RootingAPI.h 271  0x10000302b 
7  JS::Heap<JSObject *>::~Heap()                                                        RootingAPI.h 237  0x10000369e 
8  JS::Heap<JSObject *>::~Heap()                                                        RootingAPI.h 236  0x100002f75 
9  MyMonkeyClass::~MyMonkeyClass()                                                      main.cpp     64   0x100003725 
10 MyMonkeyClass::~MyMonkeyClass()                                                      main.cpp     58   0x100002aa5 
11 main                                                                                 main.cpp     110  0x100002a12 
12 start                                                                                                  0x1000029d4 

Here is a minimal example triggering the problem. I've left the GC tracing calls out intentionally, they don't change the outcome.

#include <js/Initialization.h>
#include <jsapi.h>
#include <QDebug>

// The class of the global object. Just a dummy.
static JSClass global_class = {
    "global",
    JSCLASS_GLOBAL_FLAGS,
    nullptr,
    nullptr,
    nullptr,
    nullptr,
    nullptr,
    nullptr,
    nullptr,
    nullptr,
    nullptr,
    nullptr,
    nullptr,
    JS_GlobalObjectTraceHook
};

class MyMonkeyClass
{
public:
    MyMonkeyClass() {
        Q_ASSERT( JS_Init() );
        auto runtime = JS_NewRuntime( 5 * 1024 * 1024 /*heap space*/ );
        Q_ASSERT( runtime );
        m_context = JS_NewContext( runtime, 8192 /*default. keep.*/ );
        Q_ASSERT( m_context );

        JSAutoRequest ar( m_context );

        // Not sure which of these alternatives is the correct way with JS::Heap
        m_global = JS::RootedObject( m_context,
                                   JS_NewGlobalObject( m_context, &global_class, nullptr, JS::FireOnNewGlobalHook ) );
        //global = JS_NewGlobalObject( context, &global_class, nullptr, JS::FireOnNewGlobalHook );
        Q_ASSERT( m_global.get() );
    }

    ~MyMonkeyClass() {
        auto runtime = JS_GetRuntime( m_context );
        JS_DestroyContext( m_context );
        JS_DestroyRuntime( runtime );
        JS_ShutDown();
    }

private:
    JSContext *m_context;
    JS::Heap<JSObject*> m_global;
};

int main( int, char** )
{
    MyMonkeyClass mmc;
    return 0;
}

While composing this question I found out that setting m_global = nullptr; in the dtor before destroying things actually avoids the crash. Now my final question is:

Is this the correct fix? If yes, why? Does SM maybe assume that a non-null JS::Heap pointer refers to still-in-use memory and thus it panics?

Answer 1

Your initial problem is caused by a wrong order of object deallocations.

A C++ class is destructed in a construction reverse order that is, a call to a class destructor is followed by the class member destructors. Members are destroyed in their declaration reverse order. That means that in your case:

1. ~MyMonkeyClass() is entered
2. JS_Destroy* and JS_ShutDown is called
3. JS::~Heap is called. However, JS engine is already gone and that causes undefined behaviour - crash in your case.

By setting m_global to nullptr you prevent JS::~Heap() from going down to JS API to clean up memory. That dodges the crash but leaves you with a possibility of a memory leak.

A proper fix is cleaning everything up in a correct order.

One way would be defining a yet another class "JsUpDown" that initialises JS engine, creates JS runtime and JS context in constructor and calls JS_Destroy* and JS_ShutDown in destructor. Then MyMonkeyClass would look like:

class MyMonkeyClass
{
  MyMonkeyClass() : m_engine(), m_global() // just to make it explicit
  {
     JSAutoRequest ar(m_engine.m_context);
     m_global = JS::RootedObject(...);
  }
  ~MyMonkeyClass() {} // no need for the destructor now

  JsUpDown m_engine;  // Should be first. The order is important:
                      // Members are destroyed in reverse order.
  JS::Heap<JSObject*> m_global;
};