From 06c39a8ac45f8bbe8a2f38c0b28414fc8725acf9 Mon Sep 17 00:00:00 2001
From: Kalle Olavi Niemitalo <kon@iki.fi>
Date: Tue, 8 Jul 2008 15:13:45 +0300
Subject: [PATCH] Bug 951: Lock the cache entry while the hook runs.

---
 contrib/smjs/README               | 13 ++++++++++++-
 doc/events.txt                    |  3 +++
 src/scripting/smjs/cache_object.c |  6 ++++++
 src/scripting/smjs/load_uri.c     |  9 +++++++++
 src/session/session.c             | 11 +++++++++++
 5 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/contrib/smjs/README b/contrib/smjs/README
index 16c1a00b..897120de 100644
--- a/contrib/smjs/README
+++ b/contrib/smjs/README
@@ -45,6 +45,8 @@ elinks.load_uri(uri, callback)
 
    displays a fortune.
 
+   The cache object will not expire until after the callback returns.
+
 
 Properties
 ----------
@@ -183,6 +185,8 @@ elinks.preformat_html(cached, vs)
    to elinks.preformat_html_hooks rather than to assign it to
    elinks.preformat_html.
 
+   The cache object will not expire until after this function returns.
+
 
 elinks.goto_url_hook(url)
 
@@ -210,7 +214,14 @@ cache
 
   The cache object mentioned in the descriptions of elinks.load_uri and
   elinks.preformat_html is a wrapper for the internal ELinks cache object.
-  Its properties are:
+  ELinks passes the ECMAScript cache object as an argument to your
+  ECMAScript function, and keeps the corresponding document in the
+  cache until the function returns. After that, ELinks may remove the
+  document from the cache, even if the function has saved the cache
+  object to some global variable. Such an expired cache object does
+  not work but it does not crash ELinks either.
+
+  The properties of the cache object are:
 
   cached.content (string)
 
diff --git a/doc/events.txt b/doc/events.txt
index 7ee8b639..ed0fca2e 100644
--- a/doc/events.txt
+++ b/doc/events.txt
@@ -249,6 +249,9 @@ Description:
 	add_fragment(cached, 0, new_string, new_len);
 	normalize_cache_entry(cached, new_len);
 
+ The caller must ensure that there is a reference to cached, so that
+ calling garbage_collection() from the event handler cannot free it.
+
 -------------------------------------------------------------------------------
 Name:		quit
 Managed By:	The scripting subsystem/backends
diff --git a/src/scripting/smjs/cache_object.c b/src/scripting/smjs/cache_object.c
index 7da71ef7..59708fe8 100644
--- a/src/scripting/smjs/cache_object.c
+++ b/src/scripting/smjs/cache_object.c
@@ -220,6 +220,12 @@ static const JSClass cache_entry_class = {
 	JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub, cache_entry_finalize
 };
 
+/** Return an SMJS object through which scripts can access @a cached.
+ * If there already is such an object, return that; otherwise create a
+ * new one.  The SMJS object holds only a weak reference to @a cached;
+ * so if the caller wants to guarantee that @a cached exists at least
+ * until a script returns, it should use lock_object() and
+ * unlock_object().  */
 JSObject *
 smjs_get_cache_entry_object(struct cache_entry *cached)
 {
diff --git a/src/scripting/smjs/load_uri.c b/src/scripting/smjs/load_uri.c
index a74f80b5..f9c7e09f 100644
--- a/src/scripting/smjs/load_uri.c
+++ b/src/scripting/smjs/load_uri.c
@@ -32,6 +32,13 @@ smjs_loading_callback(struct download *download, void *data)
 
 	if (!download->cached) goto end;
 
+	/* download->cached->object.refcount is typically 0 here
+	 * because no struct document uses the cache entry.  Because
+	 * the connection is no longer using the cache entry either,
+	 * it can be garbage collected.  Don't let that happen while
+	 * the script is using it.  */
+	object_lock(download->cached);
+
 	assert(hop->callback);
 
 	smjs_ses = hop->ses;
@@ -44,6 +51,8 @@ smjs_loading_callback(struct download *download, void *data)
 	JS_CallFunction(smjs_ctx, NULL, hop->callback, 1, args, &rval);
 
 end:
+	if (download->cached)
+		object_unlock(download->cached);
 	mem_free(download->data);
 	mem_free(download);
 
diff --git a/src/session/session.c b/src/session/session.c
index 46ec3279..aa7fce99 100644
--- a/src/session/session.c
+++ b/src/session/session.c
@@ -513,6 +513,17 @@ maybe_pre_format_html(struct cache_entry *cached, struct session *ses)
 	if (!cached || cached->preformatted)
 		return;
 
+	/* The script called from here may indirectly call
+	 * garbage_collection().  If the refcount of the cache entry
+	 * were 0, it could then be freed, and the
+	 * cached->preformatted assignment at the end of this function
+	 * would crash.  Normally, the document has a reference to the
+	 * cache entry, and that suffices.  If the following assertion
+	 * ever fails, object_lock(cached) and object_unlock(cached)
+	 * must be added to this function.  */
+	assert(cached->object.refcount > 0);
+	if_assert_failed return;
+
 	fragment = get_cache_fragment(cached);
 	if (!fragment) return;