From 314a41588cb7322a01c51d5f0bc7067016c91482 Mon Sep 17 00:00:00 2001 From: Kalle Olavi Niemitalo Date: Sat, 5 Jul 2008 04:55:16 +0300 Subject: [PATCH] Bug 951: weaken pointer from JSObject to cache_entry The SpiderMonkey scripting module handles the "pre-format-html" event by constructing a JSObject for the struct cache_entry and then calling elinks.preformat_html(cache_entry, view_state) if such a function exists. The problem with this was that each such JSObject kept the struct cache_entry locked until SpiderMonkey garbage-collected the JSObject, even if the user had not defined an elinks.preformat_html function and the JSObject was thus never needed at all. To work around that, the SpiderMonkey scripting module ran a garbage collection whenever the user told ELinks to flush caches. Remove the SpiderMonkey scripting module's use of object_lock and object_unlock on struct cache_entry, and instead make the pointers weak so that ELinks can free the cache_entry whenever it wants even if a JSObject is pointing to it. Each cache_entry now has a pointer back to the JSObject; done_cache_entry calls smjs_detach_cache_entry_object, which follows the pointer and detaches the cache_entry and the JSObject from each other. This commit does not yet remove the workaround mentioned above. --- NEWS | 2 + src/cache/cache.c | 6 ++ src/cache/cache.h | 3 + src/scripting/smjs/cache_object.c | 120 +++++++++++++++++++++++++++++--------- src/scripting/smjs/core.c | 3 + src/scripting/smjs/smjs.h | 7 +++ 6 files changed, 114 insertions(+), 27 deletions(-) diff --git a/NEWS b/NEWS index c1e29b23..d6688b2e 100644 --- a/NEWS +++ b/NEWS @@ -16,6 +16,8 @@ Bugs that should be removed from NEWS before the 0.12.0 release: * bug 955: Reset buttons no longer run FORM/@onsubmit, and ``harmless'' buttons no longer submit the form. ELinks 0.12pre1 was the first release that had these bugs. +* minor bug 951: SpiderMonkey scripting objects used to prevent ELinks + from removing files from the memory cache ELinks 0.12pre1: ---------------- diff --git a/src/cache/cache.c b/src/cache/cache.c index 8b050800..001c3656 100644 --- a/src/cache/cache.c +++ b/src/cache/cache.c @@ -18,6 +18,9 @@ #include "protocol/protocol.h" #include "protocol/proxy.h" #include "protocol/uri.h" +#ifdef CONFIG_SCRIPTING_SPIDERMONKEY +# include "scripting/smjs/smjs.h" +#endif #include "util/error.h" #include "util/memory.h" #include "util/string.h" @@ -656,6 +659,9 @@ done_cache_entry(struct cache_entry *cached) delete_entry_content(cached); if (cached->box_item) done_listbox_item(&cache_browser, cached->box_item); +#ifdef CONFIG_SCRIPTING_SPIDERMONKEY + if (cached->jsobject) smjs_detach_cache_entry_object(cached); +#endif if (cached->uri) done_uri(cached->uri); if (cached->proxy_uri) done_uri(cached->proxy_uri); diff --git a/src/cache/cache.h b/src/cache/cache.h index ed68d1fe..e19717bf 100644 --- a/src/cache/cache.h +++ b/src/cache/cache.h @@ -50,6 +50,9 @@ struct cache_entry { off_t data_size; /* The actual size of all fragments */ struct listbox_item *box_item; /* Dialog data for cache manager */ +#ifdef CONFIG_SCRIPTING_SPIDERMONKEY + struct JSObject *jsobject; /* Instance of cache_entry_class */ +#endif timeval_T max_age; /* Expiration time */ diff --git a/src/scripting/smjs/cache_object.c b/src/scripting/smjs/cache_object.c index 8d2a44d9..7da71ef7 100644 --- a/src/scripting/smjs/cache_object.c +++ b/src/scripting/smjs/cache_object.c @@ -11,6 +11,7 @@ #include "protocol/uri.h" #include "scripting/smjs/cache_object.h" #include "scripting/smjs/core.h" +#include "scripting/smjs/smjs.h" #include "util/error.h" #include "util/memory.h" @@ -42,6 +43,7 @@ static JSBool cache_entry_get_property(JSContext *ctx, JSObject *obj, jsval id, jsval *vp) { struct cache_entry *cached; + JSBool ret; /* This can be called if @obj if not itself an instance of the * appropriate class but has one in its prototype chain. Fail @@ -51,15 +53,22 @@ cache_entry_get_property(JSContext *ctx, JSObject *obj, jsval id, jsval *vp) cached = JS_GetInstancePrivate(ctx, obj, (JSClass *) &cache_entry_class, NULL); + if (!cached) return JS_FALSE; /* already detached */ - if (!cache_entry_is_valid(cached)) return JS_FALSE; + assert(cache_entry_is_valid(cached)); + if_assert_failed return JS_FALSE; + + /* Get a strong reference to the cache entry to prevent it + * from being deleted if some function called below decides to + * collect garbage. After this, all code paths must + * eventually unlock the object. */ + object_lock(cached); undef_to_jsval(ctx, vp); if (!JSVAL_IS_INT(id)) - return JS_FALSE; - - switch (JSVAL_TO_INT(id)) { + ret = JS_FALSE; + else switch (JSVAL_TO_INT(id)) { case CACHE_ENTRY_CONTENT: { struct fragment *fragment = get_cache_fragment(cached); @@ -69,27 +78,32 @@ cache_entry_get_property(JSContext *ctx, JSObject *obj, jsval id, jsval *vp) fragment->data, fragment->length)); - return JS_TRUE; + ret = JS_TRUE; + break; } case CACHE_ENTRY_TYPE: *vp = STRING_TO_JSVAL(JS_NewStringCopyZ(smjs_ctx, cached->content_type)); - return JS_TRUE; + ret = JS_TRUE; + break; case CACHE_ENTRY_HEAD: *vp = STRING_TO_JSVAL(JS_NewStringCopyZ(smjs_ctx, cached->head)); - return JS_TRUE; + ret = JS_TRUE; + break; case CACHE_ENTRY_LENGTH: *vp = INT_TO_JSVAL(cached->length); - return JS_TRUE; + ret = JS_TRUE; + break; case CACHE_ENTRY_URI: *vp = STRING_TO_JSVAL(JS_NewStringCopyZ(smjs_ctx, struri(cached->uri))); - return JS_TRUE; + ret = JS_TRUE; + break; default: /* Unrecognized integer property ID; someone is using * the object as an array. SMJS builtin classes (e.g. @@ -97,8 +111,12 @@ cache_entry_get_property(JSContext *ctx, JSObject *obj, jsval id, jsval *vp) * and leave *@vp unchanged. Do the same here. * (Actually not quite the same, as we already used * @undef_to_jsval.) */ - return JS_TRUE; + ret = JS_TRUE; + break; } + + object_unlock(cached); + return ret; } /* @cache_entry_class.setProperty */ @@ -106,6 +124,7 @@ static JSBool cache_entry_set_property(JSContext *ctx, JSObject *obj, jsval id, jsval *vp) { struct cache_entry *cached; + JSBool ret; /* This can be called if @obj if not itself an instance of the * appropriate class but has one in its prototype chain. Fail @@ -115,13 +134,20 @@ cache_entry_set_property(JSContext *ctx, JSObject *obj, jsval id, jsval *vp) cached = JS_GetInstancePrivate(ctx, obj, (JSClass *) &cache_entry_class, NULL); + if (!cached) return JS_FALSE; /* already detached */ - if (!cache_entry_is_valid(cached)) return JS_FALSE; + assert(cache_entry_is_valid(cached)); + if_assert_failed return JS_FALSE; - if (!JSVAL_IS_INT(id)) - return JS_FALSE; + /* Get a strong reference to the cache entry to prevent it + * from being deleted if some function called below decides to + * collect garbage. After this, all code paths must + * eventually unlock the object. */ + object_lock(cached); - switch (JSVAL_TO_INT(id)) { + if (!JSVAL_IS_INT(id)) + ret = JS_FALSE; + else switch (JSVAL_TO_INT(id)) { case CACHE_ENTRY_CONTENT: { JSString *jsstr = JS_ValueToString(smjs_ctx, *vp); unsigned char *str = JS_GetStringBytes(jsstr); @@ -130,7 +156,8 @@ cache_entry_set_property(JSContext *ctx, JSObject *obj, jsval id, jsval *vp) add_fragment(cached, 0, str, len); normalize_cache_entry(cached, len); - return JS_TRUE; + ret = JS_TRUE; + break; } case CACHE_ENTRY_TYPE: { JSString *jsstr = JS_ValueToString(smjs_ctx, *vp); @@ -138,7 +165,8 @@ cache_entry_set_property(JSContext *ctx, JSObject *obj, jsval id, jsval *vp) mem_free_set(&cached->content_type, stracpy(str)); - return JS_TRUE; + ret = JS_TRUE; + break; } case CACHE_ENTRY_HEAD: { JSString *jsstr = JS_ValueToString(smjs_ctx, *vp); @@ -146,18 +174,25 @@ cache_entry_set_property(JSContext *ctx, JSObject *obj, jsval id, jsval *vp) mem_free_set(&cached->head, stracpy(str)); - return JS_TRUE; + ret = JS_TRUE; + break; } default: /* Unrecognized integer property ID; someone is using * the object as an array. SMJS builtin classes (e.g. * js_RegExpClass) just return JS_TRUE in this case. * Do the same here. */ - return JS_TRUE; + ret = JS_TRUE; + break; } + + object_unlock(cached); + return ret; } -/* @cache_entry_class.finalize */ +/** Pointed to by cache_entry_class.finalize. SpiderMonkey + * automatically finalizes all objects before it frees the JSRuntime, + * so cache_entry.jsobject won't be left dangling. */ static void cache_entry_finalize(JSContext *ctx, JSObject *obj) { @@ -169,14 +204,17 @@ cache_entry_finalize(JSContext *ctx, JSObject *obj) cached = JS_GetInstancePrivate(ctx, obj, (JSClass *) &cache_entry_class, NULL); - if (!cached) return; + if (!cached) return; /* already detached */ - object_unlock(cached); + JS_SetPrivate(ctx, obj, NULL); /* perhaps not necessary */ + assert(cached->jsobject == obj); + if_assert_failed return; + cached->jsobject = NULL; } static const JSClass cache_entry_class = { "cache_entry", - JSCLASS_HAS_PRIVATE, /* struct cache_entry * */ + JSCLASS_HAS_PRIVATE, /* struct cache_entry *; a weak reference */ JS_PropertyStub, JS_PropertyStub, cache_entry_get_property, cache_entry_set_property, JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub, cache_entry_finalize @@ -187,7 +225,10 @@ smjs_get_cache_entry_object(struct cache_entry *cached) { JSObject *cache_entry_object; + if (cached->jsobject) return cached->jsobject; + assert(smjs_ctx); + if_assert_failed return NULL; cache_entry_object = JS_NewObject(smjs_ctx, (JSClass *) &cache_entry_class, @@ -195,14 +236,39 @@ smjs_get_cache_entry_object(struct cache_entry *cached) if (!cache_entry_object) return NULL; - if (JS_FALSE == JS_SetPrivate(smjs_ctx, cache_entry_object, cached)) /* to @cache_entry_class */ - return NULL; - - object_lock(cached); - if (JS_FALSE == JS_DefineProperties(smjs_ctx, cache_entry_object, (JSPropertySpec *) cache_entry_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 @cached. */ + if (JS_FALSE == JS_SetPrivate(smjs_ctx, cache_entry_object, cached)) /* to @cache_entry_class */ + return NULL; + + cached->jsobject = cache_entry_object; return cache_entry_object; } + +/** Ensure that no JSObject contains the pointer @a cached. This is + * called when the reference count of the cache entry *@a cached is + * already 0 and it is about to be freed. If a JSObject was + * previously attached to the cache entry, the object will remain in + * memory but it will no longer be able to access the cache entry. */ +void +smjs_detach_cache_entry_object(struct cache_entry *cached) +{ + assert(smjs_ctx); + assert(cached); + if_assert_failed return; + + if (!cached->jsobject) return; + + assert(JS_GetInstancePrivate(smjs_ctx, cached->jsobject, + (JSClass *) &cache_entry_class, NULL) + == cached); + if_assert_failed {} + + JS_SetPrivate(smjs_ctx, cached->jsobject, NULL); + cached->jsobject = NULL; +} diff --git a/src/scripting/smjs/core.c b/src/scripting/smjs/core.c index 599c0096..9d51a915 100644 --- a/src/scripting/smjs/core.c +++ b/src/scripting/smjs/core.c @@ -154,6 +154,9 @@ cleanup_smjs(struct module *module) { if (!smjs_ctx) return; + /* These calls also finalize all JSObjects that have been + * allocated in the JSRuntime, so cache_entry_finalize gets + * called and resets each cache_entry.jsobject = NULL. */ JS_DestroyContext(smjs_ctx); JS_DestroyRuntime(smjs_rt); } diff --git a/src/scripting/smjs/smjs.h b/src/scripting/smjs/smjs.h index 8c545d43..c4155a7f 100644 --- a/src/scripting/smjs/smjs.h +++ b/src/scripting/smjs/smjs.h @@ -2,7 +2,14 @@ #define EL__SCRIPTING_SMJS_SMJS_H struct module; +struct cache_entry; extern struct module smjs_scripting_module; +/* TODO: Use trigger_event() for this, so that the caching code need + * not directly call the SMJS scripting module? That would require + * changes in struct cache_object though, to let a dynamic number of + * scripting modules have pointers from there to their objects. */ +void smjs_detach_cache_entry_object(struct cache_entry *cached); + #endif -- 2.11.4.GIT