GNOME.org
 

libseed-list importer GC bug (was Re: js extensions for native modules)

This change exposed another bug: sometimes the object returned from imports.sandbox was screwed up.
The reason this happens is that the GC won't find the module in our cached file_imports hash table, thus allowing the GC to collect it and we'll get a wild pointer in the file_imports cache.
And the reason this never happened before is that it didn't reuse the cached module object but always made a new one for each reference of imports.sandbox.
Solution: protect the module object after adding it to the file_imports cache. Updated patch attached. 

I don't know if this should be done for js-modules or gir-modules too?
PS. With this patch the "workaround WebKit GC bug" (which actually was a seed bug) in seed-cairo.c shouldn't be needed anymore. 

/Jonatan

Jonatan Liljedahl wrote:
I did a fresh clone from git.gnome.org/seed and now it all seems up to date again..
I think I found the problem: in handle_native_module(), file_path is used as a key to the file_imports hash table, but the key is free'd after insertion. So each time a native module is requested, it's a new copy, which means that any additions by the extension-file wouldn't stick.
Attached patch fixes that and implements the following behaviour: When loading a native module, also try running libseed_NAME.js in the same dir as the module. This allows the script to extend the module. 

Here's the libseed_sandbox.js that I find useful:

imports.sandbox.Context.prototype.eval_file = function(fn) {
        var  script = {};
        GLib.file_get_contents(fn,script);
        return this.eval(script.contents);
};

/Jonatan

Jonatan Liljedahl wrote:

Alan Knowles wrote:

Ok - rebuilt from head:

If you run it on the seed interpreter - it will not work.

If you make a file of it, and run that, it works perfectly..


What's the reason for this?
I did a change in seed-importer that runs seed_importer_handle_file() on libseed_NAME.js in the same dir as libseed_NAME.so, and tried adding the imports.sandbox.Context.prototype.eval_file = ... in libseed_sandbox.js.
A print() in libseed_sandbox.js tells me that it's running this file, but 'c.eval_file()' still gives me "TypeError Result of expression 'c.eval_file' [undefined] is not a function." even though it's a file in this case.
Could this be for the same reason that an imported js file can't extend the built-in prototypes (Array, String, etc..)? Perhaps the extension-file must not be evaluated in a new context?
Also I have a GIT question, I tried to get a diff with my recent changes with 'git diff seed-importer.c' but then the diff also includes my old changes which you already commited. Do I have the wrong git branch or how does it work? 

/Jonatan


Regards
Alan

 --- On 29/Jun/2010, Alan Knowles wrote:
This is a bit strange, My portable (Ubuntu 10.4) managed to do this fine this morning, however, pretty much every other box I've got shows the same errors that you get..
I've just reverted the get_global_object stuff, as it's not needed (.global was already there) 

Testing on a fresh build to see if I can get it to work.

Regards
Alan

 --- On 29/Jun/2010, Jonatan Liljedahl wrote:
I reverted my seed-sandbox.c to current git, but I had only this small change which I thought was a typo fix: 

--- a/modules/sandbox/seed-sandbox.c
+++ b/modules/sandbox/seed-sandbox.c
@@ -126,7 +126,7 @@ seed_static_function context_funcs[] = {
    {"eval", seed_context_eval, 0},
    {"add_globals", seed_sandbox_context_add_globals, 0},
    {"destroy", seed_sandbox_context_destroy, 0},
-  {"get_global_object", seed_sandbox_context_get_global_object },
+  {"get_global_object", seed_sandbox_context_get_global_object, 0},
    {NULL, NULL, 0}
  };

Anyhow, now it loads the sandbox module...

But I still can't add to the prototype:
> imports.sandbox.Context.prototype.foo = function() { print("test") }; 
function () { print("test"); }
 > c = new imports.sandbox.Context
[object Context]
 > c.foo()
TypeError Result of expression 'c.foo' [undefined] is not a function.
 > c.eval("1+2")
3
Could this be a difference in JSC version? I'm with webkit 1.1.15.2-1 in ubuntu. 

/Jonatan

Alan Knowles wrote:

It failed to load the sandbox module by the looks of things.
It's probably worth running with --seed-debug=all to find out what's going on. - if you built seed with debug on. 

Regards
Alan


 --- On 29/Jun/2010, Jonatan Liljedahl wrote:
Doesn't work here, but now something else is strange (first try to get sandbox failed):
> imports.sandbox.Context.prototype.foo = function() { print("test") }; 
function () { print("test"); }
 > c = new imports.sandbox.Context
TypeError Result of expression 'imports.sandbox' [undefined] is not an object. 
 > c = new imports.sandbox.Context
[object Context]
 > c.foo()
TypeError Result of expression 'c.foo' [undefined] is not a function. 
 >


Alan Knowles wrote:

Did I miss something - this works perfectly..


GLib = imports.gi.GLib;
imports.sandbox.Context.prototype.eval_file = function(fn) { var script = {}; print("load " + fn); GLib.file_get_contents(fn,script); return this.eval(script.contents); }; 

c = new imports.sandbox.Context;
c.eval_file("test.js");

#output>>>
load test.js
** (seed:3644): CRITICAL **: Line 10 in /tmp/seedtest.js: GFileError Failed to open file 'test.js': No such file or directory 

Regards
Alan



 --- On 29/Jun/2010, Jonatan Liljedahl wrote:
This should be made to work - It's probably going to take a bit of 
 > working out how to support it...
I'm trying to understand how this works, am I right that native modules create their classes with seed_create_class() (JSClassCreate()), while the Gir importer sets up an object with a 'prototype' property "by hand"? 

 From the JSC API docs:
"Standard JavaScript practice calls for storing function objects in prototypes, so they can be shared. The default JSClass created by JSClassCreate follows this idiom, instantiating objects with a shared, automatically generating prototype containing the class's function objects. The kJSClassAttributeNoAutomaticPrototype attribute specifies that a JSClass should not automatically generate such a prototype."
So, the question is why the prototype is not writable and if it can be made to be that. I tried this hackish function on the constructor but it didn't work: 

gboolean
seed_make_extendable (JSContextRef ctx, JSObjectRef object)
{
JSStringRef jname = JSStringCreateWithUTF8CString ("prototype"); 
   JSValueRef exception = NULL;
   JSValueRef proto = JSObjectGetPrototype(ctx, object);
   if (proto)
     {
JSObjectSetProperty (ctx, (JSObjectRef) object, jname, proto, kJSPropertyAttributeDontEnum|kJSPropertyAttributeDontDelete, 
               &exception);
     }

   JSStringRelease (jname);
   return TRUE;
}
Once JSClassCreate has made the prototype, it seems to be fixed and read-only.
Perhaps one should create classes more "manually" like the Gir importer does instead of using JSClassCreate()?
Looking through the code, it's either we have to explicitly overlay 
the prototype properties in the constructor, or we set the
parent_class of Context to a standard object..
parent_class can only be a JSClassRef, and it says it should be NULL for the normal Object class, which I guess it should be. Seems like JavaScriptCore has classic inherited OOP internally, and that parent_class has nothing to do with prototype. (but that all objects has Object as parent_class)
I'm not sure I understand what you mean by overlay the prototype properties.. 

/Jonatan


Regarsd Alan

--- On 21/Jun/2010, Jonatan Liljedahl wrote:

Also, it should work to add methods to SomeClass.prototype in
native modules. Currently it does not:

imports.sandbox.Context.prototype.eval_file = function(fn) { var
script = {}; GLib.file_get_contents(fn,script); return
this.eval(script.contents); };

c = new imports.sandbox.Context; c.eval_file(foobar);
TypeError Result of expression 'ctx.eval_file' [undefined] is not a 
 function.

It does work for GI modules, as seen in extensions/Gtk.js

/Jonatan

Jonatan Liljedahl wrote:
The gi_importer tries to import extensions/name.js by evaluating "imports.extensions.name". It would be nice if this also worked 
for native modules, I imagine it to be quite common that you
write a module with core parts in C and some high-level stuff in 
js.
The question is, how should this be handled regarding searchpaths 
etc?

If your module is installed in /usr/local/lib/seed, then you
probably installed the extension js in
/usr/local/share/seed/extensions, and imports.extensions would be 
the right thing.

But what happens if your module is installed somewhere else? It
should probably start with the same dir as the native module,
then go on with imports.searchPath, and if the file is found call 
 seed_importer_handle_file() on it..

BTW, what happens if you have 'extensions' folder in more than
one search path? will extensions.foo search for 'foo' in all of
those or only the first match for 'extensions'? Just doing a
'imports.extensions' will of course create only one dir_importer 
for that folder, hiding possible 'extensions' folders later in
the searchpath. I don't know if this is a good thing...
/Jonatan _______________________________________________ libseed-list mailing list libseed-list gnome org http://mail.gnome.org/mailman/listinfo/libseed-list 
_______________________________________________ libseed-list
mailing list libseed-list gnome org http://mail.gnome.org/mailman/listinfo/libseed-list
_______________________________________________ libseed-list mailing list libseed-list gnome org http://mail.gnome.org/mailman/listinfo/libseed-list 
_______________________________________________
libseed-list mailing list
libseed-list gnome org
http://mail.gnome.org/mailman/listinfo/libseed-list









diff --git a/libseed/seed-importer.c b/libseed/seed-importer.c
index 7d66f18..09221d2 100644
--- a/libseed/seed-importer.c
+++ b/libseed/seed-importer.c
@@ -70,6 +70,12 @@ GHashTable *file_imports;
  *        for that context.
  */
 
+static JSObjectRef
+seed_importer_handle_file (JSContextRef ctx,
+			   const gchar * dir,
+			   const gchar * file, JSValueRef * exception);
+
+
 /*
 * Handle definition of toplevel functions in a namespace.
 * i.e. Gtk.main
@@ -576,18 +582,19 @@ seed_importer_get_search_path (JSContextRef ctx, JSValueRef * exception)
 static JSObjectRef
 seed_importer_handle_native_module (JSContextRef ctx,
 				    const gchar * dir,
-				    const gchar * file,
+				    const gchar * prop,
 				    JSValueRef * exception)
 {
   GModule *module;
   JSObjectRef module_obj;
   SeedModuleInitCallback init;
-  gchar *file_path = g_strconcat (dir, "/", file, NULL);
+  gchar *file_path = g_strconcat (dir, "/libseed_", prop, ".", G_MODULE_SUFFIX, NULL);
 
   SEED_NOTE (IMPORTER, "Trying native module: %s", file_path);
 
   if ((module_obj = g_hash_table_lookup (file_imports, file_path)))
     {
+      SEED_NOTE (IMPORTER, "Using existing global");
       g_free (file_path);
       return module_obj;
     }
@@ -609,6 +616,11 @@ seed_importer_handle_native_module (JSContextRef ctx,
   g_hash_table_insert (file_imports, file_path, module_obj);
   SEED_NOTE (IMPORTER, "Loaded native module");
 
+  //protect module_obj since the GC won't find the module in our file_imports cache
+  seed_value_protect(ctx, module_obj);
+  
+  file_path = g_strconcat ("libseed_", prop, ".js", NULL);
+  seed_importer_handle_file (ctx, dir, file_path, exception);
   g_free (file_path);
 
   return module_obj;
@@ -767,7 +779,7 @@ seed_importer_search_dirs (JSContextRef ctx, GSList *path, gchar *prop, JSValueR
         // check if file is native module
         file_path = g_build_filename (test_path, prop_as_lib, NULL);
         if (g_file_test (file_path, G_FILE_TEST_IS_REGULAR)) {
-            ret = seed_importer_handle_native_module (ctx, test_path, prop_as_lib, exception);
+            ret = seed_importer_handle_native_module (ctx, test_path, prop, exception);
             g_free (file_path);
             break;
         }
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]