GNOME Bugzilla – Bug 622896
xulrunner 1.9.3+ port
Last modified: 2010-09-30 14:53:26 UTC
JS_PushArgumentsVA and JS_PopArgument have been removed from the public API, and these are used in gi/closure.c. Relevant Mozilla bug: http://hg.mozilla.org/mozilla-central/rev/c6980db86452
p.s.: I just filed this as a heads-up
Created attachment 166162 [details] [review] support build with xulrunner 2.0 replace JS_PushArgumentsVA and JS_PopArgument. JS_AddRoot -> JS_Add[Value/String/Object/GCThing]Root JS_RemoveRoot -> JS_Remove[Value/String/Object/GCThing]Root JS_NewObject -> JS_NewGlobalObject (for global object) && clasp should NOT be NULL.
I didn't look at the patch comprehensively but a couple things I noticed Because config.h isn't installed and jsapi-util.h is installed, using config.h variables in jsapi-util.h will break. You could put the compat defines in config.h itself perhaps or in some new built-sources header. Removing rooting of the global object seems like an unrelated change to the porting, should be separate commit? It looks like gjs_closure_invoke_simple() just gets removed instead of ported? Deserves some comment in the commit message explaining why, at least. Defining "-DHAVE_JS_ADDROOT" on command line is not right, should just put this in config.h like the usual configure checks would do. If any .c file fails to #include <config.h> as the first header, that file should be fixed. Should probably not key JS_NewGlobalObject() compat off of HAVE_JS_ADDROOT; if we're using the define as a generic "have newer mozilla" define, it should be named accordingly. But maybe cleaner to just do a HAVE_JS_NEWGLOBALOBJECT
(In reply to comment #3) > Because config.h isn't installed and jsapi-util.h is installed, using config.h > variables in jsapi-util.h will break. If gjs compiled with xulrunner-1.9.x, Cflags in gjs-1.0.pc contain "-DHAVE_JS_ADDROOT" > You could put the compat defines in config.h itself perhaps or in some new built-sources header. Gnome-Shell also use JS_AddRoot/JS_RemoveRoot. In my opinion, better has compat defines in one place > Removing rooting of the global object seems like an unrelated change to the > porting, should be separate commit? It is useless code, that use JS_AddRoot/JS_RemoveRoot. should it be separate commit? > It looks like gjs_closure_invoke_simple() just gets removed instead of ported? > Deserves some comment in the commit message explaining why, at least. JS_PushArgumentsVA and JS_PopArgument was removed. Port gjs_closure_invoke_simple mean copy them from xulrunner.
*** Bug 611746 has been marked as a duplicate of this bug. ***
Okay, there are definitely multiple bugs here. I'm working on splitting up various changes here. There are unfortunately compiler warnings from inline functions in the xulrunner headers, but I'm aiming to be warning-free.
Created attachment 170498 [details] [review] remove callers of JS_NewDouble
Created attachment 170499 [details] [review] addvalueroot
Created attachment 170500 [details] [review] JSVAL_NUL != NULL
Created attachment 170501 [details] [review] jsid != jsval
Created attachment 170502 [details] [review] JS_NewGlobalObject
Current state is "compiles without warning on 1.9.3". Yet to do: * Make tests work * Ensure compatibility with 1.9.2 works (i tried, but not tested)
Review of attachment 170498 [details] [review]: Looks good
Review of attachment 170500 [details] [review]: looks good.
Review of attachment 170501 [details] [review]: Looks mostly simple and correct with one suggestion for a change ::: gi/boxed.c @@ +674,3 @@ + return JS_FALSE; + + field_info = get_field_info(context, priv, id_value); to me get_field_info() definitely is taking a jsid. I don't think separating out jsid => jsval and jsval => integer we encoded into a jsid makes sense.
Review of attachment 170499 [details] [review]: I'm going to assume that the compiler did it's job on this one, I've only scanned the line by line changes. If you think they need detailed checking (like if the compiler isn't catching the cases where you get the Add*Root wrong), then let me know.
Review of attachment 170502 [details] [review]: Most looks fine to me. ::: gjs/context.c @@ -619,3 @@ - /* this is probably not necessary, having it as global object in - * context already roots it presumably? Could not find where it - * does in a quick glance through spidermonkey source though. If you've resolved this to your satisfaction, then you need to mention that in the commit message. ::: gjs/jsapi-util-array.c @@ +316,1 @@ JS_BeginRequest(context); Why was this moved oustide of JS_BeginRequest()? (I've never really looked at the JS_BeginRequest stuff) ::: gjs/jsapi-util.c @@ +74,3 @@ + +JSObject * +gjs_new_global_object (JSContext *context) Would like to see some docs of this.
I was going to point you to my docs on BeginRequest and they were still on my hard drive. Bug 629953 if curious.
(In reply to comment #15) > Review of attachment 170501 [details] [review]: > > Looks mostly simple and correct with one suggestion for a change > > ::: gi/boxed.c > @@ +674,3 @@ > + return JS_FALSE; > + > + field_info = get_field_info(context, priv, id_value); > > to me get_field_info() definitely is taking a jsid. I don't think separating > out jsid => jsval and jsval => integer we encoded into a jsid makes sense. Good catch, fixed.
New series of patches: http://git.gnome.org/browse/gjs/log/?h=wip/xulrunner-1.9.3-rebase5 Status: * make check works with xulrunner 1.9.3 * Have not tried compiling on 1.9.2 again, will do so before git bz'ing here
I dug in the old old gjs git logs (before its gnome.org import) and I think the call context came up in the following way: 1. we used to save the JSContext with the jsval for a callback, and JS_CallFunctionValue in that callback's original context. This worked fine except we were using a JSContext from xulrunner and it would go away and we'd crash. 2. so we tried running the callbacks in the load context. the load context had previously been added in order to have a place to load modules that would be shared among all contexts and never destroyed. We had multiple contexts because we had the GjsContext and also one per xulrunner window (I think). Without a load context, modules were not singletons which confuses a lot of them in addition to being wasteful. 3. if you JS_CallFunctionValue() in a context, it sets the scope chain of the context. This replaces the context's global object. So say you invoke a callback, and that callback then tries to load or use a module. But the load context would no longer have the global object with the modules in it. This would either fail outright or load modules into the callback's original global object or something else broken. (detail: JS_CallFunctionValue() does not affect the JS_SetGlobalObject() object, it affects the root of the scope chain. JS_SetGlobalObject() only sets the default root of the scope chain and only matters when the chain is empty) 4. The solution is to use essentially a disposable one-off context, the "call context", which has nothing of interest in its scope chain or global object. As the comments in the code note, creating a new context every time for call context would be fine, having a fixed one is just an efficiency thing. If you didn't have a load context, but did just have multiple contexts for some other reason, you would pretty much have the same issue; invoking a callback from context A inside context B would mess up context B, such that trying to use context B inside the callback would not work as expected. So in short I think the rule is that function objects must be invoked in either a context that already has the callback's closed global object, or a context where we don't care what global object it has. Having only one context ever is one way to achieve this so it should be safe to remove call context as long as load context is also removed. If we had one context per thread, then there would have to be a rule that you could not run a callback from one thread in another thread's context, for example if you did the trick to get to the main thread where you do idle_add(function(){}), then when the main thread ran function() it would set the main thread's context to the global object of the original thread's context, which seems likely to end badly. The fix would probably be along the lines of having a call context. Arguably the bug is using a "JSContext*" when you really wanted to refer to a global object. i.e. maybe it should be a load *global object* not a load *context*. If we just stored a global object with the modules on it, then to load a module you would just need to first set a scope chain consisting of the proper global object (it might work to pass the global to JS_EvaluateScript()?). I'd say if removing load and call context, the "runtime" prop on GjsContext should go, along with the jsapi-util.h calls to get the load and call context, it just needs to be a wholesale removal of the idea that there can be >1 JSContext. I'm not sure what the side effects of loading modules etc. into the app's global object might be; I think the constructors we define might all have "__" in front of them or something? I'm not sure what else might get defined. If there's an issue here or it causes problems, we could try the "load object" instead of a "load context" to try to isolate the gjs internal variables. Anyway that makes me feel better that the problem is understood and it's OK to delete the load and call contexts. Sorry it took a while to dig up the facts and the comments didn't explain it well enough.
The rationale makes sense except for the original root issue: What was the lifetime of the JSContext * in xulrunner? Why was it going away?
I think the JSContext is attached to the toplevel window in xulrunner, or something like that. It isn't a one-per-thread model.
*** Bug 628359 has been marked as a duplicate of this bug. ***
*** Bug 630179 has been marked as a duplicate of this bug. ***
The wip/xulrunner-1.9.3-rebase5 branch does not compile with a one-day old xulrunner-2.0 (changeset: 14b390aa5b85 [1]) from mozilla-central (also failed with an older changeset: 4097291a632c, one week old). Please note that 1.9.3 became 2.0 many weeks ago, and xulrunner-2.0 is the branch that's going into firefox-4. Here's the error message: ----- make all-am make[1]: Entering directory `/var/tmp/portage/dev-libs/gjs-0.7.2_p20100923/work/gjs-0.7.2_p20100923' /bin/sh ./libtool --tag=CC --mode=compile x86_64-pc-linux-gnu-gcc -DHAVE_CONFIG_H -I. -pthread -DXP_UNIX -DJS_THREADSAFE -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/xulrunner-2.0 -I/usr/include/nspr -DGJS_TOP_SRCDIR=\".\" -DGJS_JS_DIR=\"/usr/share/gjs-1.0\" -DGJS_NATIVE_DIR=\"/usr/lib64/gjs-1.0\" -DGJS_COMPILATION -Wfloat-equal -Wsign-compare -Wcast-align -Wpointer-arith -Wnested-externs -Wmissing-prototypes -Wmissing-declarations -Wchar-subscripts -Wall -march=core2 -mtune=generic -msse3 -O2 -pipe -ggdb -MT libgjs_la-jsapi-util.lo -MD -MP -MF .deps/libgjs_la-jsapi-util.Tpo -c -o libgjs_la-jsapi-util.lo `test -f 'gjs/jsapi-util.c' || echo './'`gjs/jsapi-util.c libtool: compile: x86_64-pc-linux-gnu-gcc -DHAVE_CONFIG_H -I. -pthread -DXP_UNIX -DJS_THREADSAFE -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/xulrunner-2.0 -I/usr/include/nspr -DGJS_TOP_SRCDIR=\".\" -DGJS_JS_DIR=\"/usr/share/gjs-1.0\" -DGJS_NATIVE_DIR=\"/usr/lib64/gjs-1.0\" -DGJS_COMPILATION -Wfloat-equal -Wsign-compare -Wcast-align -Wpointer-arith -Wnested-externs -Wmissing-prototypes -Wmissing-declarations -Wchar-subscripts -Wall -march=core2 -mtune=generic -msse3 -O2 -pipe -ggdb -MT libgjs_la-jsapi-util.lo -MD -MP -MF .deps/libgjs_la-jsapi-util.Tpo -c gjs/jsapi-util.c -fPIC -DPIC -o .libs/libgjs_la-jsapi-util.o In file included from /usr/include/xulrunner-2.0/jspubtd.h:47:0, from /usr/include/xulrunner-2.0/jsapi.h:49, from gjs/jsapi-util.h:31, from gjs/jsapi-util.c:31: /usr/include/xulrunner-2.0/jsval.h: In function ‘JS_CANONICALIZE_NAN’: /usr/include/xulrunner-2.0/jsval.h:739:9: warning: comparing floating point with == or != is unsafe gjs/jsapi-util.c: In function ‘gjs_check_constructing’: gjs/jsapi-util.c:377:5: error: too few arguments to function ‘JS_IsConstructing’ /usr/include/xulrunner-2.0/jsapi.h:3124:1: note: declared here gjs/jsapi-util.c: In function ‘gjs_log_object_props’: gjs/jsapi-util.c:633:5: warning: statement with no effect gjs/jsapi-util.c: In function ‘gjs_explain_scope’: gjs/jsapi-util.c:693:5: warning: statement with no effect make[1]: *** [libgjs_la-jsapi-util.lo] Error 1 make[1]: Leaving directory `/var/tmp/portage/dev-libs/gjs-0.7.2_p20100923/work/gjs-0.7.2_p20100923' make: *** [all] Error 2 --- 1, http://hg.mozilla.org/mozilla-central/rev/14b390aa5b85
Created attachment 170973 [details] [review] Drop load and call contexts Apparently for XULRunner embedding reasons, multiple different contexts were added to gjs. We're using these in a way not really intended by Spidermonkey. The default intended design is that there's one (active) JSContext * per thread. In particular, stacking JS_BeginRequest(context_a); JS_BeginRequest(context_b) is "dubious" in current Spidermonkey. For the load context: If we want to e.g. avoid having modules loaded multiple times even if run from a non-default context (and thus a non-default global object), the obvious implementation technique is to just hook them off the GjsRuntime default global object. For the callback context: We need to assume that we have a standard context provided by Gjs, which is the thread default one. Actually, one possibility here is to root the global object for a given callback.
Created attachment 170974 [details] [review] xulrunner 1.9.3: Consistently include compat.h
Created attachment 170975 [details] [review] xulrunner 1.9.3: Use JS_AddValueRoot if available
Created attachment 170976 [details] [review] xulrunner 1.9.3: Fix assumption that jsid == jsval The two have always been conceptually distinct types. Even in 1.9.3, they are still the same in implementation, but to avoid a pile of warnings, we should avoid confusing them. Unfortunately, even the SpiderMonkey docs are wrong in a few places, so gjs' code confusing them is understandable. Anyways, fix up places that made this assumption.
Created attachment 170977 [details] [review] xulrunner 1.9.3: Use JS_NewGlobalObject if available In 1.9.3, we need to explicitly say when we're making the global object. Clean this up by introducing a wrapper function; while we're at it, also call JS_InitStandardClasses here since everything else creating a global did too. Note that rooting the global object is not necessary, so remove that. From a grep of the sources (going back to at least 1.9.2): ./src/jsgc.cpp: if (acx->globalObject && !JS_HAS_OPTION(acx, JSOPTION_UNROOTED_GLOBAL)) ./src/jsgc.cpp: JS_CALL_OBJECT_TRACER(trc, acx->globalObject, "global object");
Created attachment 170978 [details] [review] xulrunner 1.9.3: Drop use of JS_PushArgumentsVA JS_PushArgumentsVA vanished in 1.9.3; since it was only used in debugger.c, which was itself just removed, delete it. Separated from a larger patch by Colin Walters <walters@verbum.org>
Created attachment 170979 [details] [review] dbus: Use separate SetPrototype() call In XULRunner 1.9.3, passing a custom prototype to JS_ConstructObject for Object seems to fail; from a reading of the code, what's happening is the Object constructor is overriding it.
Created attachment 170981 [details] [review] Cast users of JS_EnterLocalRootScope In Xulrunner 1.9.3, this is now a no-op. See https://bugzilla.mozilla.org/show_bug.cgi?id=519949 Now in theory, we should be checking the return value and handling that. I wrote a nontrivial patch to do this, but decided it wasn't worth it, since in newer xulrunners it's just pointless code spaghetti, and in older, it's not like we're really going to go OOM in reality.
Created attachment 170982 [details] [review] importer: Handle JSENUMERATE_INIT_ALL We don't have any non-enumerable properties on the importer object, so just handle this like JSENUMERATE_INIT.
Created attachment 170983 [details] [review] importer: JSID != JSVAL Also, the first entry should be JSID(0) technically, to indicate "unknown length".
Created attachment 170984 [details] [review] importer: Don't crash if we're enumerating the prototype Apparently enumeration changed so that (for v in importer) now *also* loops over the prototype properties. However, the code didn't handle this, because the _INIT case for prototype just stuffed NULL into the custom data. Handle this by simply ignoring NULL in ENUMERATE_NEXT, and finish the iteration.
Review of attachment 170973 [details] [review]: > For the load context: If we want to e.g. avoid having modules loaded > multiple times even if run from a non-default context (and thus a > on-default global object), the obvious implementation technique is to > just hook them off the GjsRuntime default global object. But this patch doesn't implement that, does it? > For the callback context: We need to assume that we have a standard > context provided by Gjs, which is the thread default one. Actually, > one possibility here is to root the global object for a given > callback. Invoking closures across thread boundaries is something that I would expect to be absolutely normal in threaded JS code. Mainloop.idle_add({ ...}) will always result in the code being run in the main thread. (If the different threads are entirely isolated from each other - if we don't allow multithreaded JS code, but only single-threaded JS code running in multiple threads at once - if we give each thread a different default main context so that Mainloop doesn't cross threads. Then tying a closure to a context probably makes more sense) So, the thing where you save the context at creation time and use that at invocation time doesn't necessarily make sense to me. Going to study a bit how Firefox uses contexts. ::: gi/object.c @@ +569,3 @@ + { + context = NULL; + JS_ContextIterator(runtime, &context); Presumably this causes nested contexts if there are more than one context in the process?
Looking at the XPConnect code - in particular mozilla-central/js/src/xpconnect/src/xpcwrappedjsclass.cpp, the behavior with respect to contexts seems to be: * If there is a context (owned by the XPCconnect code, that's all it knows about) already active, that is used. * Otherwise, the context is extracted from the object and that is used (If it failed to extract the context from the object, then it would fall back to some "safe context" for the thread, but I don't think that can happen.) So, it appears that for: https://developer.mozilla.org/en/The_Thread_Manager the context in which the object is created will be used for the callback. (Since the 'Run' method of the IRunnable is invoked right out of the native thread function.) But that contradicts the docs for JS_THREADSAFE about what is safe - I see no usage of JS_ClearContextThread() that would be invoked. Pretty confused.
Created attachment 171051 [details] [review] [SQUASH] ns: add missing goto
(In reply to comment #38) > Review of attachment 170973 [details] [review]: > > > For the load context: If we want to e.g. avoid having modules loaded > > multiple times even if run from a non-default context (and thus a > > on-default global object), the obvious implementation technique is to > > just hook them off the GjsRuntime default global object. > > But this patch doesn't implement that, does it? Correct; I didn't dive into anything for threads. > Invoking closures across thread boundaries is something that I would expect to > be absolutely normal in threaded JS code. Mainloop.idle_add({ ...}) will always > result in the code being run in the main thread. Right. Now...how I think this would work is that we run the closure in the target thread context. This implies that it may no longer have access to any properties of the global object of the other thread, but that seems quite reasonable to me if we assume that "imports" is a singleton across threads. > So, the thing where you save the context at creation time and use that at > invocation time doesn't necessarily make sense to me. True...but irrelevant in a non-multithreaded world. I'm not sure it makes sense to start fixing up the code in theory for multithreads in this patch series. As long as we have a rough idea of how it should work, we can do it later. > Going to study a bit how Firefox uses contexts. > > ::: gi/object.c > @@ +569,3 @@ > + { > + context = NULL; > + JS_ContextIterator(runtime, &context); > > Presumably this causes nested contexts if there are more than one context in > the process? Yes; I can clean up more things like this, but I worry about where it's going to stop. Maybe some new comments around code that we know would change in the new theoretical order?
Talked to the Mozilla folks a bit; their message was basically that things like the Thread Manager or doing Mainloop.idle_add({ <function to run in the main thread >}) are inherently bad form. (Though certainly used a fair bit in Firefox currently.) The recommended way to handle threads is: Define workers in separate JS files using separate global objects Send workers POD messages Receive POD messages back from workers The "web workers" spec (http://www.whatwg.org/specs/web-workers/current-work/) was suggested as a good model to follow. To some extent this strikes me as making a virtue out of necessity or fighting with one hand tied behind your back... using closures to define the "message" and "return message" is a powerful method and doesn't necessarily involve any locking or manipulation of shared objects. But on the other hand, there aren't a ton of places where we need to use threads in a free-form fashion. GIO asynchronous operation often is handling the threads behind the scene. So having an inconvenient-but-safe way of doing threading from JS code might be fine. I'm not sure this fully clarifies things for me, but if nobody is using GJS currently with shared state between threads, then perhaps the approach of binding closures tightly to to contexts works for now and is going in the right direction. I still think that we can't get rid of the load context without doing the work to replace it.
(In reply to comment #41) > (In reply to comment #38) > > Review of attachment 170973 [details] [review] [details]: > > > > > For the load context: If we want to e.g. avoid having modules loaded > > > multiple times even if run from a non-default context (and thus a > > > on-default global object), the obvious implementation technique is to > > > just hook them off the GjsRuntime default global object. > > > > But this patch doesn't implement that, does it? > > Correct; I didn't dive into anything for threads. Load contexts don't have anything to do with threads. They are about separate bits of JS code using the same JS_Runtime and sharing the same JS classes for GObject classes. > > Invoking closures across thread boundaries is something that I would expect to > > be absolutely normal in threaded JS code. Mainloop.idle_add({ ...}) will always > > result in the code being run in the main thread. > > Right. Now...how I think this would work is that we run the closure in the > target thread context. This implies that it may no longer have access to any > properties of the global object of the other thread, but that seems quite > reasonable to me if we assume that "imports" is a singleton across threads. I don't think you'd actually get anything clean - once we've created a closure with a global object in place, that closure is going to be tied to the scope it was created in. You'd get some sort of unholy mix of dynamic scoping and lexical scoping. > > So, the thing where you save the context at creation time and use that at > > invocation time doesn't necessarily make sense to me. > > True...but irrelevant in a non-multithreaded world. I'm not sure it makes > sense to start fixing up the code in theory for multithreads in this patch > series. As long as we have a rough idea of how it should work, we can do it > later. As I understand it, what the code currently tries to handle is not multithreading, but using GJS from multiple different JS_Contexts. We *could* remove that capability if litl is OK with that (it sounded like they were no longer using that capability), but I'm a little uncomfortable with that - the places where load_context is used currently marks places where attention needs to be paid for whatever we do in the future - blanket replacing them with the caller context loses that information. > > Going to study a bit how Firefox uses contexts. > > > > ::: gi/object.c > > @@ +569,3 @@ > > + { > > + context = NULL; > > + JS_ContextIterator(runtime, &context); > > > > Presumably this causes nested contexts if there are more than one context in > > the process? > > Yes; I can clean up more things like this, but I worry about where it's going > to stop. Maybe some new comments around code that we know would change in the > new theoretical order? Grabbing a random, undefined context doesn't make sense to me. If we want to restrict ourselves to the JS_Context that we created ourselves and no other JS_Contexts, then we need to assert on API entry points that that is the case or remove the parameters. And we then should use that JS context when we need a context.
> Right. Now...how I think this would work is that we run the closure in the > target thread context. This implies that it may no longer have access to any > properties of the global object of the other thread, but that seems quite > reasonable to me if we assume that "imports" is a singleton across threads. The problem (or a problem anyway) is you *break* the context in the target thread, not that you don't have the context from the origin thread. OK, original context getting destroyed was a problem for us too, but we fixed that by trying to run callbacks in the load context, and then that failed because the callbacks hosed the load context. The reason it matters that you break the load context is because gjs would expect the load context's global object to exist *while inside the callback* - this is my point about maybe we should change module loading to track a global object (and set it on whatever context) rather than indirectly obtaining the global object to load into *via* a load context. What I don't know (would require some thought) is whether this "hoses the target context" thing would ever come up in say the idle_add() case or whether it's just kind of a quirk of how we used the load context pointer to get to the load global object, from C. For example I think it might be true that you can't detect the "target context hosing" from pure JS. To confirm, litl isn't using multiple contexts right now either, so we could punt from my perspective. But I'm also happy to see Owen digging in and actually trying to figure it out.
(In reply to comment #44) > > Right. Now...how I think this would work is that we run the closure in the > > target thread context. This implies that it may no longer have access to any > > properties of the global object of the other thread, but that seems quite > > reasonable to me if we assume that "imports" is a singleton across threads. > > The problem (or a problem anyway) is you *break* the context in the target > thread, not that you don't have the context from the origin thread. OK, > original context getting destroyed was a problem for us too, but we fixed that > by trying to run callbacks in the load context, and then that failed because > the callbacks hosed the load context. Do you recall what the nature of this hosing was? It's not immediately obvious to me and I'd like to make sure I understand things as well as possible. (Well, obvious in any sense other than I'd expect weird things to happen if you define a closure with one global object than invoke it with a different global object.)
the hosing - unless I still have this wrong - is that to run a closure, spidermonkey sets its scope chain on the target context. For the load context, then when you load a module, you're loading into that scope chain (with the global from the callback's original context). The module is essentially then lost, i.e. doesn't end up in the load context's global. The problem is that we rely on the load context having its original global, when we go to load a module. so we could fix that by forcing the load global onto whatever context we want to use. The bug could be having a "load context" when we should have a "load global" then use a thread-default context or something. One thing I don't know is whether there's actually a way to set the scope chain to our "load global" in the public API, so I don't know if "load global" is actually possible. There is JS_SetGlobalObject but that won't work, it doesn't modify an existing scope chain, just sets the default if the scope chain has to be initially created.
(In reply to comment #43) > (In reply to comment #41) > > (In reply to comment #38) > > > Review of attachment 170973 [details] [review] [details] [details]: > > > > > > > For the load context: If we want to e.g. avoid having modules loaded > > > > multiple times even if run from a non-default context (and thus a > > > > on-default global object), the obvious implementation technique is to > > > > just hook them off the GjsRuntime default global object. > > > > > > But this patch doesn't implement that, does it? > > > > Correct; I didn't dive into anything for threads. > > Load contexts don't have anything to do with threads. They are about separate > bits of JS code using the same JS_Runtime and sharing the same JS classes for > GObject classes. You'll need to better define "separate bits of JS code" here. Separate in what way? > I don't think you'd actually get anything clean - once we've created a closure > with a global object in place, that closure is going to be tied to the scope it > was created in. You'd get some sort of unholy mix of dynamic scoping and > lexical scoping. Probably. I agree with them that message passing is the way to go. > As I understand it, what the code currently tries to handle is not > multithreading, but using GJS from multiple different JS_Contexts. Well...originally they were trying to support a JSContext not under their explicit control, which is sort of different from "different JSContext"s. > We *could* > remove that capability if litl is OK with that (it sounded like they were no > longer using that capability), but I'm a little uncomfortable with that - the > places where load_context is used currently marks places where attention needs > to be paid for whatever we do in the future - blanket replacing them with the > caller context loses that information. If we do go message passing (and actually I am strongly leaning this way), then nothing needs to be done - you can only import from the main thread/context pair. The whole GJS code base would need to be fixed up for arbitrary threading; I'm not sure that it's really valuable to specifically mark the "load" area. > Grabbing a random, undefined context doesn't make sense to me. If we want to > restrict ourselves to the JS_Context that we created ourselves and no other > JS_Contexts, then we need to assert on API entry points that that is the case > or remove the parameters. And we then should use that JS context when we need a > context. Okay so, just add assertions to the patch?
Review of attachment 170975 [details] [review]: This was marked ACN before, and is unchanged. I reattached for ordering reasons.
Review of attachment 170976 [details] [review]: Marked ACN before, unchanged.
(In reply to comment #47) > (In reply to comment #43) > > (In reply to comment #41) > > > (In reply to comment #38) > > > > Review of attachment 170973 [details] [review] [details] [details] [details]: > > > > > > > > > For the load context: If we want to e.g. avoid having modules loaded > > > > > multiple times even if run from a non-default context (and thus a > > > > > on-default global object), the obvious implementation technique is to > > > > > just hook them off the GjsRuntime default global object. > > > > > > > > But this patch doesn't implement that, does it? > > > > > > Correct; I didn't dive into anything for threads. > > > > Load contexts don't have anything to do with threads. They are about separate > > bits of JS code using the same JS_Runtime and sharing the same JS classes for > > GObject classes. > > You'll need to better define "separate bits of JS code" here. Separate in what way? Havoc would be in a better position to provide details about what they were doing - but imagine, say, that we had a widget/applet system where each widget was running with a separate global object - where it was like a separate browser tab. [...] > > As I understand it, what the code currently tries to handle is not > > multithreading, but using GJS from multiple different JS_Contexts. > > Well...originally they were trying to support a JSContext not under their > explicit control, which is sort of different from "different JSContext"s. In some ways - it makes some cleanup harder. It's harder to wrap what spidermonkey is doing in further layers of accounting. But in other ways it seems pretty much the same situation. > > We *could* > > remove that capability if litl is OK with that (it sounded like they were no > > longer using that capability), but I'm a little uncomfortable with that - the > > places where load_context is used currently marks places where attention needs > > to be paid for whatever we do in the future - blanket replacing them with the > > caller context loses that information. > > If we do go message passing (and actually I am strongly leaning this way), then > nothing needs to be done - you can only import from the main thread/context > pair. There are plenty of GObject libraries (GIO being the poster child) that you want to use from another thread. A worker thread that can only do pure JS computation and can't talk to the outside world is a bit boring :-) > The whole GJS code base would need to be fixed up for arbitrary threading; I'm > not sure that it's really valuable to specifically mark the "load" area. This seems to be a declaration We're 50% to being thread safe (or multi-context safe) now 50% isn't useful So let's rip out code and go to being 30% thread safe If there was something entirely wrong about our usage of JS_BeginRequest, etc, or we thought we never could make any sort of valid multithread/multicontext thing work, then I'd agree, and would rather that we went to 0% thread safe. But if we have a reasonable start, and some thought that we want to continue it in the future, we should avoid losing work. Here, avoiding losing work to me is replacing the separate load context with code that handles multiple contexts correctly. > > Grabbing a random, undefined context doesn't make sense to me. If we want to > > restrict ourselves to the JS_Context that we created ourselves and no other > > JS_Contexts, then we need to assert on API entry points that that is the case > > or remove the parameters. And we then should use that JS context when we need a > > context. > > Okay so, just add assertions to the patch? If you don't mind, I'll see if I can come up with proper handling for the load context situation over the weekend.
Review of attachment 170974 [details] [review]: Looks as consistent with itself as consistency with the not-entirely-consistent surrounding code allows
Review of attachment 170977 [details] [review]: Looks good
Review of attachment 170978 [details] [review]: Looks good
Review of attachment 170981 [details] [review]: Patch is fine but hard to figure out the "why" from the commit message - Cast users of JS_EnterLocalRootScope _ Cast users of JS_EnterLocalRootScope to suppress warning In Xulrunner 1.9.3, this is now a no-op. See https://bugzilla.mozilla.org/show_bug.cgi?id=519949 + The compat define '#define JS_EnterLocalRootScope(cx) (true)' + produces a 'statement has no effect" warning without a cast to (void) ...
Review of attachment 170982 [details] [review]: Fine
Review of attachment 170983 [details] [review]: Looks fine, not sure what is 'technically' about needing to be set to INT_TO_JSID(0) it does or it doesn't ;-) - https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/JSClass.enumerate which maybe is stale says JSVAL_ZERO, so JSID_NULL was likely never right.
Review of attachment 170984 [details] [review]: Looks fine to me, though it seems buggy if storing JSVAL_NULL into *statep with _NEXT means "no more things" and storing JSVAL_NULL into *statep still causes NEXT to be called once. But I don't understand the JS code or how it's changed enough to say that we should file a bug about it.
Review of attachment 171051 [details] [review]: Correct fix to the earlier patch to drop load context, but obviously dependent on what happens with the earlier patch
Review of attachment 170979 [details] [review]: Think I tracked down the underlying problem. Filed as: https://bugzilla.mozilla.org/show_bug.cgi?id=599651 fine with this workaround if you document it as a bug workaround and include the above link.
(In reply to comment #50) > > Havoc would be in a better position to provide details about what they were > doing - but imagine, say, that we had a widget/applet system where each widget > was running with a separate global object - where it was like a separate > browser tab. Okay, though I'm not totally sure about how the imports-on-main-thread-context-object plan would affect this use case. > There are plenty of GObject libraries (GIO being the poster child) that you > want to use from another thread. I don't think that's true; GIO is conveniently used async. > A worker thread that can only do pure JS > computation and can't talk to the outside world is a bit boring :-) Remember that the compute-add-idle-with-data is actually very close conceptually to message passing, because you can't actually modify the data after you've queued it for the main thread, so you might as well serialize it. Unless you add locks, and then you're in a totally different concurrency world. There's a lot of stuff you can do with pure computation, like string searching or parsing. > This seems to be a declaration > > We're 50% to being thread safe (or multi-context safe) now > 50% isn't useful > So let's rip out code and go to being 30% thread safe True. > If there was something entirely wrong about our usage of JS_BeginRequest, etc, > or we thought we never could make any sort of valid multithread/multicontext > thing work, then I'd agree, and would rather that we went to 0% thread safe. > But if we have a reasonable start, and some thought that we want to continue it > in the future, we should avoid losing work. Here, avoiding losing work to me is > replacing the separate load context with code that handles multiple contexts > correctly. Multiple on the same thread? The deep problem here actually is that to make that *actually* work right now we'd need to be careful about request suspending, which is a large-ish project. Remember none of this really was safe before either. > If you don't mind, I'll see if I can come up with proper handling for the load > context situation over the weekend. Did you do this? If you did, I bet you ran into the request suspension, at least?
(In reply to comment #60) > (In reply to comment #50) > > > > Havoc would be in a better position to provide details about what they were > > doing - but imagine, say, that we had a widget/applet system where each widget > > was running with a separate global object - where it was like a separate > > browser tab. > > Okay, though I'm not totally sure about how the > imports-on-main-thread-context-object plan would affect this use case. Note that separate browser tabs aren't inherently security-isolated. They are in some cases and not in other cases. > > There are plenty of GObject libraries (GIO being the poster child) that you > > want to use from another thread. > > I don't think that's true; GIO is conveniently used async. > > > A worker thread that can only do pure JS > > computation and can't talk to the outside world is a bit boring :-) > > Remember that the compute-add-idle-with-data is actually very close > conceptually to message passing, because you can't actually modify the data > after you've queued it for the main thread, so you might as well serialize it. > Unless you add locks, and then you're in a totally different concurrency world. The point I'm making is that threads need to do disk IO and networking and so forth. > There's a lot of stuff you can do with pure computation, like string searching > or parsing. If you happen to be CPU bound on string searching for data that is already in memory. [...] > > If there was something entirely wrong about our usage of JS_BeginRequest, etc, > > or we thought we never could make any sort of valid multithread/multicontext > > thing work, then I'd agree, and would rather that we went to 0% thread safe. > > But if we have a reasonable start, and some thought that we want to continue it > > in the future, we should avoid losing work. Here, avoiding losing work to me is > > replacing the separate load context with code that handles multiple contexts > > correctly. > > Multiple on the same thread? The deep problem here actually is that to make > that *actually* work right now we'd need to be careful about request > suspending, which is a large-ish project. I'm not talking co-routines running interleaved or anything. The simple case is two contexts calling from the main loop. The reason you need to suspend isn't anything to do interleaving contexts on a single thread; the main reason to suspend is that if two threads are in a request at the same time (for *any* context of the runtime), GC can't happen. So, if we have the main thread sitting in the main loop, some other thread doing computation or whatever will never GC. I don't think request suspension is that big a deal. Two approaches: - Just suspend in Mainloop.run() - Suspend every time we call out to generic native code - which is only a half dozen or so places. But request suspension isn't that interesting until we're actually using threads either. > > If you don't mind, I'll see if I can come up with proper handling for the load > > context situation over the weekend. > > Did you do this? If you did, I bet you ran into the request suspension, at > least? My concern isn't to make anything in particular work beyond works currently, it is to come up with a coherent strategy. I'll post a strategy in a separate comment.
Some useful groundwork. * All uses of gjs/gi within a single process must use the same JS_Runtime This comes from the rule that only one entity in a process can use toggle references. Two copies of the GJS engine using different JS_Runtime would cause any shared object to leak, since they would create separateproxies with separate toggle references. It would work, of course, if the two runtimes were used with entirely disjoint sets of GObjects, but most GObject-based libraries have some global objects. * All uses of gjs/gi within a single process must use the same GC compartment. The basic idea of compartments, which are being added now to Spidermonkey is to have separate domains of GC - so different tabs/webpages can be GC'ed independently. "every object is in a compartment, and an ordinary object cannot have a direct reference to an object in a different compartment. (We will support special wrapper objects that provide transparent access to objects in other compartments.)" Since we have the requirement that each GObject must have only a single JS proxy, that means that there can be only one compartment with these proxies, and references from any other compartment must be wrapped up. * Sandboxing code using gi will be very hard and isn't a primary concern right now. There are three basic problems for trying to Sandbox gi code: - There are huge swathes of the GNOME platform which are inherently unsafe. Like the unrestricted access to the filesystem through GIO. - It's never been a concern to make the GNOME API safe against malicious callers. E.g. GTK+ doesn't try to catch widget hierarchies that are self-recursive. - Shared proxies for GObject would allow evil-doing by modifying the JS proxy object. In order to sandbox, we'd certainly need another layer that added wrapping around proxies. And we'd need a whitelist system. It's probably simpler just not to expose the GNOME platform to things that need to be sandboxed. * In general, Javascript code doesn't care what context it is running in While the context does have a default global object, in general, that's invisible to running javascript code. The 'global' that is used for Javascript code execution is the global at the root of the scope chain; which is derived from the running code. If you search for usage of cx->globalObject in the spidermonkey sources, it's used only in a tiny number of places. (One way of experimenting with code with multiple global objects involved is in HTML pages do 'w = open("other.html"); w.onload = function() { ... };' ) So, it's actually pretty normal and expected behavior to take a Javascript function that was defined in one context and run it in another context. On the other hand, switching back to the original context means that the caller isn't visible on the stack, etc. * Per-context class definitions combined with shared proxies produces confusion Presumably, we don't want: Gdk.Display.get_default() instanceOf Gdk.Display To fail depending on what context called Gdk.Display.get_default() first. (This *does* happen with standard classes like Array if they are passed between contexts not sharing a global object. You can see this in the two-html-pages test case. But the pinned proxies on global objects make this a more important issue.)
The Call Context ================ In most cases, one of two things will be the case when we're calling from native code back to Javascript: A) GJS was called from Javascript, ran some native code, and that code triggered callbacks. In this case, there is a "current context" we can track by maintaining a [future: per-thread] stack of current contexts. B) There is no Javascript context involved at all at call time (callback from the Mutter mainloop, say.) In this case, using the default context makes sense. (The only exception I could think of to this is if you had a xulrunner/gjs mash-up and the xulrunner code recursed the main loop within a JS context) So, my proposal for replacing the call context is: * Add gjs_runtime_push_context()/gjs_runtime_pop_context() (As mentioned above, the runtime argument to these functions is a bit silly; supporting multiple runtimes in the same process won't work because of the limitations of toggle references) * Add gjs_runtime_get_current_context() and replace saved contexts with this for: - closures - GI callback values - D-Bus callbacks Because of the runtime argument to the functions above, we'd need to save a runtime pointer in places where we are saving a context pointer. * For now, leave the complicated machinery in gi/closure.c for invalidating closures when a context is torn down in place; whether we want this would really depend on what we are actually using multiple contexts for. I have a patch partially started for this - don't think it's going to be hard to finish.
The Load Context ================ As mentioned above, since proxies are shared between all contexts, I think we need to share class definitions between all contexts as well. As far as I know, we can achieve that fine by: * Using the global object for gjs_context->context rather than the current global object in gjs_init_class_dynamic() so that the constructor is stored on that object. * Passing the global object for gjs_context->context in gjs_construct_object_dynamic() as the parent to JS_ConstructObject(). Note that this does result in the parent object of the constructed object being permanently stored as the default global object, but I don't think any harm will come from that. (And isn't different than what the load context code resulted in.)
(In reply to comment #61) > > There's a lot of stuff you can do with pure computation, like string searching > > or parsing. > > If you happen to be CPU bound on string searching for data that is already in > memory. You don't necessarily need to be bound; as long as the task (e.g. searching) is more expensive than the cost of communicating it over to a separate thread, it's useful to take advantage of multiple cores too. > I'm not talking co-routines running interleaved or anything. The simple case is > two contexts calling from the main loop. > > The reason you need to suspend isn't anything to do interleaving contexts on a > single thread; Modulo that this trips assertions in the Spidermonkey in rawhide, though is *probably* allowed in newer Spidermonkey, yes. > the main reason to suspend is that if two threads are in a > request at the same time (for *any* context of the runtime), GC can't happen. > So, if we have the main thread sitting in the main loop, some other thread > doing computation or whatever will never GC. Right. > I don't think request suspension is that big a deal. Two approaches: > > - Just suspend in Mainloop.run() > - Suspend every time we call out to generic native code - which is only a half > dozen or so places. It's not a big deal to *try*; but if we're talking about actually testing it and ensuring it really works, that's a more unknown quantity. > My concern isn't to make anything in particular work beyond works currently, it > is to come up with a coherent strategy. I'll post a strategy in a separate > comment. That definitely makes sense, yes.
(In reply to comment #63) > The Call Context > ================ > > In most cases, one of two things will be the case when we're calling from > native code back to Javascript: > > A) GJS was called from Javascript, ran some native code, and that code > triggered callbacks. In this case, there is a "current context" we can track by > maintaining a [future: per-thread] stack of current contexts. > > B) There is no Javascript context involved at all at call time (callback from > the Mutter mainloop, say.) In this case, using the default context makes sense. Bit of a problem here - there's no concept of a "default context" currently. (For some reason I thought there could be only one GjsContext for a runtime, but that's not the case.) I'm just going to say that the first created GjsContext for a runtime is the default context. Could make it explicit later if we need to.
(In reply to comment #66) > (In reply to comment #63) > > The Call Context > > ================ > > > > In most cases, one of two things will be the case when we're calling from > > native code back to Javascript: > > > > A) GJS was called from Javascript, ran some native code, and that code > > triggered callbacks. In this case, there is a "current context" we can track by > > maintaining a [future: per-thread] stack of current contexts. > > > > B) There is no Javascript context involved at all at call time (callback from > > the Mutter mainloop, say.) In this case, using the default context makes sense. > > Bit of a problem here - there's no concept of a "default context" currently. > (For some reason I thought there could be only one GjsContext for a runtime, > but that's not the case.) Hmm? Do you mean JSContext * or GjsContext*? For the latter, note my first patch removed the "runtime" constructor property, so the JSRuntime* is always owned by the GjsContext*. GjsContext should really be named GjsRuntime.
(In reply to comment #67) > (In reply to comment #66) > > Bit of a problem here - there's no concept of a "default context" currently. > > (For some reason I thought there could be only one GjsContext for a runtime, > > but that's not the case.) > > Hmm? Do you mean JSContext * or GjsContext*? For the latter, note my first > patch removed the "runtime" constructor property, so the JSRuntime* is always > owned by the GjsContext*. I meant mostly GjsContext. > GjsContext should really be named GjsRuntime. Well, that's not the way it is at the moment. (it's a weird hybrid really - you create one GjsContext that owns the runtime, then can create more GjsContexts that connect to that GjsContext.)
Fixing the call context as described was very straightforward, but the load context has been a can of worms. One problem is that the load context isn't just something behind the scenes. Because imports of of Javascript code are done inside the load context, nearly everything will be done with the load context's global object. For gnome-shell, the "main program' is: const Main = imports.ui.main; Main.start(); That's the only code that is going to use the global object for gjs_context->context; all other Javascript code will be using the load context's global object. Another problem turns out to be that you can't just "make up" a global object - while I haven't fully tracked things down, things seem to work badly if the global object at the end of the scope chain isn't actually the global object for some context. This seems to be true even if you are only using it for constructing objects, but is especially true if we are using the global object as the global object when importing Javascript modules. If we want a "nothing shared" threading model, then having code in all contexts use the same global object for their JS modules isn't ideal. Not only does it cause leakage for modules that have global variables, it will cause contention over the global object between threads. (Objects accessed from only one thread are apparently optimized.) But giving each context it's own stack of separately imported JS modules isn't something I want to worry about at the moment. Colin's approach of making the context created with the GjsContext that owns the runtime the special default context and extracting the load global from that may be the best approach. Going to try that now.
Created attachment 171301 [details] [review] importer: Don't crash if we're enumerating the prototype Your patch for this didn't work for me - JSVAL_TO_PRIVATE(JSVAL_NULL) wasn't NULL (i386 vs. x86_64?) - but this does.
Created attachment 171306 [details] [review] Replace call context with a concept of "current context" Using multiple nested contexts on the same thread isn't normal usage of Spidermonkey and triggers bugs in Spidermonkey that might not be caught in upstream testing. (It is theoretically supported, to at least some extent.) When calling a callback, the best context to use is the current context. If that's not available, then using the default context from the GjsContext is usually sensible. Add functions: gjs_runtime_push_context() gjs_runtime_pop_context() gjs_runtime_get_current_context() One potential concern here is the extensive use of GDataset to get data from a context, which involves locking overhead and hash table lookups and can be slow; since we can't use GJS with multiple JS_Runtimes in the same process due to the limitations of toggle references, perhaps we should enforce that and use global variables.
Created attachment 171307 [details] [review] Replace "load context" with a "default global" The idea of the load context was to have a single context where modules and native were defined and shared between different JSContexts in the same runtime. But this caused context switching when loading modules, which isn't very tested in Spidermonkey and caused lots of use of gjs_move_exception() and other complications. Instead, switch to the idea of a default global object; this is the global object that is used in the scope chain for modules, and is also where the constructors for our native classes are stored, allowing native classes to be shared between different contexts.
The attached patches pass 'make check' and work with the shell. I can't say I really love them, and the big caveat is that I didn't create a framework to actually test them with multiple contexts. And I don't have a xulrunner-1.9.2 box to test with. But: - I think the approach is internally self consistent - I'm pretty confident that it's generally less weird than what we were doing before before even in the case of multiple contexts. (We were consistently mixing the default context with load context's global object before; now we only do that if there are multiple contexts involved.) - It keeps things flexible for future expansion compared to just saying one-context/one-runtime/one-global-object.
Pushed all your patches and my patches to the wip/xulrunner-1.9.3-rebase7 branch with fixes squashed. Only thing in the previous comments I made that still needs to be done is the comment about the commit message for JS_EnterLocalRootScope - forgot to do that before pushing the branch.
Review of attachment 171306 [details] [review]: I'm still confused under what situations it makes sense for us to maintain a stack. This code seems to call _push_context around every function invocation, but makes no attempt to handle the case where the contexts are identical (as they always will be in our usage?). Basically I still think this should be gjs_runtime_get_context() which accesses a single threadlocal JSContext *.
(In reply to comment #75) > Review of attachment 171306 [details] [review]: > > I'm still confused under what situations it makes sense for us to maintain a > stack. I'm maintaining a stack *mainly* because it's simpler conceptually than keeping a nesting count for: gjs_runtime_set_running_context() gjs_runtime_unset_running_context() I don't have great examples of when you'd actually have two different contexts - but the basic example would be: Mainloop.run() <in the future suspends the current context> Callback from the mainloop does arbitrary stuff that involves a different JS context. > This code seems to call _push_context around every function invocation, but > makes no attempt to handle the case where the contexts are identical (as they > always will be in our usage?). What handling do you think is needed for that?
Review of attachment 171307 [details] [review]: Only two minor comments; but may need some changes if any comments about the call context patch are acted on. ::: gi/enumeration.c @@ +147,3 @@ + * can't just pass in the global as the parent */ + JS_SetParent(context, enum_obj, + gjs_get_default_global (context)); I'd prefer this patch be split out again. ::: gjs/jsapi-util.h @@ +207,3 @@ JSContext *context); void gjs_runtime_pop_context (JSRuntime *runtime); +JSObject* gjs_get_default_global (JSContext *context); "default global" is sort of uninformative; how about "import global"?
(In reply to comment #77) > Review of attachment 171307 [details] [review]: > > Only two minor comments; but may need some changes if any comments about the > call context patch are acted on. > > ::: gi/enumeration.c > @@ +147,3 @@ > + * can't just pass in the global as the parent */ > + JS_SetParent(context, enum_obj, > + gjs_get_default_global (context)); > > I'd prefer this patch be split out again. This isn't your patch (still separate), it's the same workaround propagating into other places. But I can commit this patch as if JS_ConstructObject worked properly and do a single patch that adds the workaround in all places. > ::: gjs/jsapi-util.h > @@ +207,3 @@ > JSContext *context); > void gjs_runtime_pop_context (JSRuntime *runtime); > +JSObject* gjs_get_default_global (JSContext *context); > > "default global" is sort of uninformative; how about "import global"? 'import global' only really gets to part of what it's used for, but sure. No name is going to really represent the idea without reading the docs and code. (I had it as "load_global' matching the old 'load_context' but that just didn't make any sense to me... it implied that it was used only when *loading* modules, but it actually is the global object when modules are executing as well.)
(In reply to comment #76) > I don't have great examples of when you'd actually have two different contexts > - but the basic example would be: > > Mainloop.run() <in the future suspends the current context> > Callback from the mainloop does arbitrary stuff that involves a different JS > context. You gave the example of an applet system earlier; I forgot about that one. So in that case, we actually have to have consistent pairs to JS_SuspendRequest/gjs_push_context right? Seems like if we were to support this, it would make sense for gjs_push_context to implicity suspend the earlier request? > > This code seems to call _push_context around every function invocation, but > > makes no attempt to handle the case where the contexts are identical (as they > > always will be in our usage?). > > What handling do you think is needed for that? It seems bizarre/wasteful to by default accumulate a GList of identical contexts as we call functions recursively? Not that function invocation is fast, but it can't be hard to do the optimization of counting entries, right?
Created attachment 171369 [details] [review] Replace "load context" with a "import global" Change default_global => import_global Remove Mozilla bug workarounds, will attach a new version of your patch moved to the end of the branch with SetParent
Created attachment 171370 [details] [review] Use separate SetPrototype() and SetParent calls Adds explicit SetParent as well as setPrototype
Bug 630964 has optimization patches on top as asked for - I meant to attach them here but accidentally filed them separately.
(In reply to comment #79) > (In reply to comment #76) > > > I don't have great examples of when you'd actually have two different contexts > > - but the basic example would be: > > > > Mainloop.run() <in the future suspends the current context> > > Callback from the mainloop does arbitrary stuff that involves a different JS > > context. > > You gave the example of an applet system earlier; I forgot about that one. So > in that case, we actually have to have consistent pairs to > JS_SuspendRequest/gjs_push_context right? Seems like if we were to support > this, it would make sense for gjs_push_context to implicity suspend the earlier > request? I think that would only make sense if we made gjs_push_context() do the JS_BeginRequest() and we basically replaced all uses of JS_BeginRequest() in our code with gjs_push_context(). Which may well make sense. Right now gjs_push_context() is used in a much more limited set of places - basically in just the places that seemed necessary to make sure that callbacks from native code to GJS could get the right context. > > > This code seems to call _push_context around every function invocation, but > > > makes no attempt to handle the case where the contexts are identical (as they > > > always will be in our usage?). > > > > What handling do you think is needed for that? > > It seems bizarre/wasteful to by default accumulate a GList of identical > contexts as we call functions recursively? Not that function invocation is > fast, but it can't be hard to do the optimization of counting entries, right? Implemented in bug 630964
All patches pushed