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

["Alan Knowles" <alan akbkhome com>]

Commited in, I changed it to  JSValueProtect, to get rid of the compiler warnings.

Regards
Alan

 --- On 29/Jun/2010, Jonatan Liljedahl wrote: 
> 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
> >>>
> >>
> >