SMJS: handle view_state outliving wrapping object

Currently, it is possible for the JSObject that wraps a struct view_state
to outlive the view_state.  Using the properties of the JSObject wrapper
will then cause a crash.

This patch adds a smjs_detach_view_state_object function, which is called
in destroy_vs, to dissociate the struct view_state from the JSObject
wrapper.  To this end, the patch modifies the struct view_state to point
the JSObject wrapper.  smjs_get_view_state_object will use this pointer
if it is set, and copy_vs will copy this pointer to the new view_state.
The patch also modifies view_state_get_property and
view_state_set_property to return immediately if the view_state has been
destroyed.  Finally, the patch adds a finalizer (view_state_finalize)
that clears the pointer from the struct view_state to the JSObject.

src/scripting/smjs/view_state_object.c

@@ -51,6 +51,7 @@ view_state_get_property(JSContext *ctx, JSObject *obj, jsid id, jsval *vp)
 
 	vs = JS_GetInstancePrivate(ctx, obj,
 				   (JSClass *) &view_state_class, NULL);
+	if (!vs) return JS_FALSE;
 
 	undef_to_jsval(ctx, vp);
 
@@ -92,6 +93,7 @@ view_state_set_property(JSContext *ctx, JSObject *obj, jsid id, JSBool strict, j
 
 	vs = JS_GetInstancePrivate(ctx, obj,
 				   (JSClass *) &view_state_class, NULL);
+	if (!vs) return JS_FALSE;
 
 	if (!JSID_IS_INT(id))
 		return JS_FALSE;
@@ -111,20 +113,47 @@ view_state_set_property(JSContext *ctx, JSObject *obj, jsid id, JSBool strict, j
 	}
 }
 
+/** Pointed to by view_state_class.finalize.  SpiderMonkey automatically
+ * finalizes all objects before it frees the JSRuntime, so view_state.jsobject
+ * won't be left dangling.  */
+static void
+view_state_finalize(JSContext *ctx, JSObject *obj)
+{
+	struct view_state *vs;
+
+	assert(JS_InstanceOf(ctx, obj, (JSClass *) &view_state_class, NULL));
+	if_assert_failed return;
+
+	vs = JS_GetInstancePrivate(ctx, obj,
+	                           (JSClass *) &view_state_class, NULL);
+
+	if (!vs) return; /* already detached */
+
+	JS_SetPrivate(ctx, obj, NULL); /* perhaps not necessary */
+	assert(vs->jsobject == obj);
+	if_assert_failed return;
+	vs->jsobject = NULL;
+}
+
 static const JSClass view_state_class = {
 	"view_state",
 	JSCLASS_HAS_PRIVATE,	/* struct view_state * */
 	JS_PropertyStub, JS_PropertyStub,
 	view_state_get_property, view_state_set_property,
-	JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub, JS_FinalizeStub
+	JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub, view_state_finalize
 };
 
+/** Return an SMJS object through which scripts can access @a vs.  If there
+ * already is such an object, return that; otherwise create a new one.  */
 JSObject *
 smjs_get_view_state_object(struct view_state *vs)
 {
 	JSObject *view_state_object;
 
+	if (vs->jsobject) return vs->jsobject;
+
 	assert(smjs_ctx);
+	if_assert_failed return NULL;
 
 	view_state_object = JS_NewObject(smjs_ctx,
 	                                  (JSClass *) &view_state_class,
@@ -132,13 +161,17 @@ smjs_get_view_state_object(struct view_state *vs)
 
 	if (!view_state_object) return NULL;
 
-	if (JS_FALSE == JS_SetPrivate(smjs_ctx, view_state_object, vs))	/* to @view_state_class */
-		return NULL;
-
 	if (JS_FALSE == JS_DefineProperties(smjs_ctx, view_state_object,
 	                               (JSPropertySpec *) view_state_props))
 		return NULL;
 
+	/* Do this last, so that if any previous step fails, we can
+	 * just forget the object and its finalizer won't attempt to
+	 * access @vs.  */
+	if (JS_FALSE == JS_SetPrivate(smjs_ctx, view_state_object, vs))	/* to @view_state_class */
+		return NULL;
+
+	vs->jsobject = view_state_object;
 	return view_state_object;
 }
 
@@ -163,6 +196,28 @@ smjs_elinks_get_view_state(JSContext *ctx, JSObject *obj, jsid id, jsval *vp)
 	return JS_TRUE;
 }
 
+/** Ensure that no JSObject contains the pointer @a vs.  This is called from
+ * destroy_vs.  If a JSObject was previously attached to the view state, the
+ * object will remain in memory but it will no longer be able to access the
+ * view state. */
+void
+smjs_detach_view_state_object(struct view_state *vs)
+{
+	assert(smjs_ctx);
+	assert(vs);
+	if_assert_failed return;
+
+	if (!vs->jsobject) return;
+
+	assert(JS_GetInstancePrivate(smjs_ctx, vs->jsobject,
+				     (JSClass *) &view_state_class, NULL)
+	       == vs);
+	if_assert_failed {}
+
+	JS_SetPrivate(smjs_ctx, vs->jsobject, NULL);
+	vs->jsobject = NULL;
+}
+
 void
 smjs_init_view_state_interface(void)
 {

src/viewer/text/vs.c

@@ -14,6 +14,9 @@
 #include "document/view.h"
 #include "ecmascript/ecmascript.h"
 #include "protocol/uri.h"
+#ifdef CONFIG_SCRIPTING_SPIDERMONKEY
+# include "scripting/smjs/smjs.h"
+#endif
 #include "session/location.h"
 #include "session/session.h"
 #include "util/memory.h"
@@ -47,6 +50,10 @@ destroy_vs(struct view_state *vs, int blast_ecmascript)
 {
 	struct form_view *fv, *next;
 
+#ifdef CONFIG_SCRIPTING_SPIDERMONKEY
+	smjs_detach_view_state_object(vs);
+#endif
+
 	/* form_state contains a pointer to form_view, so it's safest
 	 * to delete the form_state first.  */
 	for (; vs->form_info_len > 0; vs->form_info_len--)
@@ -84,6 +91,9 @@ copy_vs(struct view_state *dst, struct view_state *src)
 	/* If we ever get to render this vs, give it an interpreter. */
 	dst->ecmascript_fragile = 1;
 #endif
+#ifdef CONFIG_SCRIPTING_SPIDERMONKEY
+	dst->jsobject = NULL;
+#endif
 
 	/* destroy_vs(vs) does mem_free_if(vs->form_info), so each
 	 * view_state must have its own form_info.  Normally we make a

src/viewer/text/vs.h

@@ -11,6 +11,10 @@ struct session;
 struct uri;
 
 struct view_state {
+#ifdef CONFIG_SCRIPTING_SPIDERMONKEY
+	struct JSObject *jsobject; /* Instance of view_state_class */
+#endif
+
 	struct document_view *doc_view;
 	struct uri *uri;