GNOME Bugzilla – Bug 776966
Spidermonkey 38 prep
Last modified: 2017-02-13 20:43:18 UTC
This bug should gather patches which are necessary for porting to SpiderMonkey 38 but can still be applied while using the known-working SpiderMonkey 31 API.
Created attachment 343064 [details] [review] js: Use JS_NewObjectWithGivenProto In mozjs38, JS_NewObject() doesn't take a prototype JSObject anymore; instead, JS_NewObjectWithGivenProto() is used for that purpose. The behaviour of JS_NewObject() is changed so that it looks for a default prototype, which is the same as passing JS::NullPtr() as the proto parameter in mozjs31.
Created attachment 343065 [details] [review] js: Root IDs passed to JS_DefinePropertyById In mozjs38, rooting is required on these jsids, presumably because the function can GC where before it could not.
Created attachment 343106 [details] [review] js: Use JS_Enumerate instead of iterator JS_NewPropertyIterator() and its related functions are gone in mozjs38; apparently they were broken anyway for indexed properties (i.e. those with integer IDs rather than string IDs.) Instead, we use JS_Enumerate() which often makes the code cleaner anyway. Note that the JS::AutoIdArray does not root the jsids inside it, it just manages the array wrapper.
Created attachment 343108 [details] [review] js: Discontinue JS_GetTypeName() JS_GetTypeName() is going away in SpiderMonkey 38. Luckily, we have a very similar function in GJS already, gjs_get_type_name(), so we'll use that instead.
Created attachment 343109 [details] [review] closure: Use JS::Heap wrapper for closure object This needs to be done in case the garbage collector moves the JSObject somewhere else. This was left over from the SpiderMonkey 31 port.
Created attachment 343133 [details] [review] WIP - refactor keep-alive Keep-alive would need to change in SpiderMonkey 38 (and really should have already for SpiderMonkey 31, but I didn't realize!) because it uses JSObject pointers to generate its hash values. Since the GC can move JSObjects around, that is not a good thing to use for a hash value. This makes keep-alive a lot simpler by using JS::PersistentRootedObject (which does not get moved around by the GC). Instead of being a JSObject itself, GjsKeepAlive is a pure C++ object, a set which stores GjsKeepAliveObjects. The GjsKeepAliveObject is just JS::PersistentRootedObject augmented with a destroy notify function and a hash value. Instead of sticking all the objects in a big keep-alive object that's kept on the global object, this allows each translation unit to have its own GjsKeepAlive. This allows us to get rid of the awkward iteration functions where you have to skip over objects added by other code by keying off their destroy notify functions. GjsKeepAlive is a std::unordered_set, so uses the normal standard library API. It offers two additional methods, add_object() and remove_object(), so you don't have to deal with adding an extra root to an object just to add or remove it from the keep-alive. GjsKeepAlive registers itself with the GjsContext and unroots any remaining objects when the GjsContext is destroyed. TODO: Write tests after validating this approach.
This last patch is a complete rewrite of keep-alive.cpp... I'd still need to clean it up a bit and write tests for it, but I'm interested in hearing opinions on the approach before I go to the extra trouble. Giovanni, since it touches the toggle-ref code, would you be able to find a bit of time to look at it?
Review of attachment 343064 [details] [review]: Looks good
Review of attachment 343065 [details] [review]: Looks good to me.
Review of attachment 343106 [details] [review]: Looks good.
Review of attachment 343108 [details] [review]: Sure
Review of attachment 343109 [details] [review]: OK
Attachment 343064 [details] pushed as a2fdf35 - js: Use JS_NewObjectWithGivenProto Attachment 343065 [details] pushed as 1b0d5b0 - js: Root IDs passed to JS_DefinePropertyById Attachment 343106 [details] pushed as 03f4e89 - js: Use JS_Enumerate instead of iterator Attachment 343108 [details] pushed as e7b1a4d - js: Discontinue JS_GetTypeName() Attachment 343109 [details] pushed as f9b7962 - closure: Use JS::Heap wrapper for closure object
Created attachment 343279 [details] [review] gerror: Root define_error_properties() This function now requires a rooted object, since the JS_DefinePropertyById() calls require one as well.
Created attachment 343280 [details] [review] js: Set IMPLEMENTS_BARRIERS flag on traced classes I'm not sure what this flag does exactly, the documentation says it indicates that a class "correctly implements GC read and write barriers," but in SpiderMonkey 38 it's required on all classes that have a trace hook.
Created attachment 343281 [details] [review] js: Be more pedantic about defining DEBUG I found a bugzilla bug [1] about the situation with JS_DEBUG and DEBUG, and it says that DEBUG is used in some of the Mozilla headers such as Vector. If we don't define DEBUG every time we include a SpiderMonkey or Mozilla header, we could get runtime undefined behaviour depending on which order SpiderMonkey includes its own headers in! This consolidates all SpiderMonkey and Mozilla includes in jsapi-wrapper.h and adds a link to the bug in the comment so that it's more clear when this workaround can be removed. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1261161
Created attachment 343282 [details] [review] tests: Fix double-declared variable SpiderMonkey 38 is more strict about this and will emit a warning which causes the test to fail.
Created attachment 343283 [details] [review] minijasmine: Unref context on error We have to unref the GjsContext before exiting or JS_ShutDown() will segfault. For convenience, stick the unref in bail_out(). (This doesn't have anything to do with SpiderMonkey 38, but I noticed it because I broke some tests while porting.)
Created attachment 343346 [details] [review] js: Remove JSCLASS_NEW_RESOLVE where not used Classes that don't have a resolve callback, or have one but don't need it, can lose the JSCLASS_NEW_RESOLVE flag as well. This flag is going away in SpiderMonkey 38.
Created attachment 343347 [details] [review] js: Adapt to upcoming JS::Heap API These lines as they previously were cause compile errors in SpiderMonkey 38. JS::Heap's operators are defined slightly differently than they previously were.
Giovanni, I forgot to CC you before, but if you might have a moment to look at the "WIP - refactor keep-alive" patch I'd appreciate it. BTW, I've gotten somewhat farther along in compiling with SpiderMonkey 38 now and the toggle-ref code will have to change even more after the switch...
Another thing that will have to change in SpiderMonkey 38 is GjsCallbackTrampoline... Currently it roots its JSObject function _unless_ the function is a vfunc, in which case it maintains a weak pointer. Adding and removing roots to a JS::Heap wrapper is removed in SpiderMonkey 38, you are supposed to use JS::PersistentRooted instead. But we can't put a vfunc in a JS::PersistentRooted because this GjsCallbackTrampoline here [1] is effectively leaked. [1] https://git.gnome.org/browse/gjs/tree/gi/object.cpp#n2460
I found a regression in the "js: Use JS_NewObjectWithGivenProto" patch. If you pass a prototype to JS_NewObjectWithGivenProto, you have to make sure it is not NULL, otherwise the object will have _no_ prototype. Concretely this meant that Object methods such as hasOwnProperty() stopped working on GObject wrappers. Pushed a fix together with a regression test.
Comment on attachment 343280 [details] [review] js: Set IMPLEMENTS_BARRIERS flag on traced classes This patch is no longer necessary, I committed a similar one from bug 724797.
Created attachment 343619 [details] [review] js: Set IMPLEMENTS_BARRIERS flag on fundamental This was the only class that didn't have this flag. In SpiderMonkey 38 it's required on all classes that have a trace hook.
Created attachment 343620 [details] [review] js: Remove JSCLASS_NEW_RESOLVE where not used Classes that don't have a resolve callback, or have one but don't need it, can lose the JSCLASS_NEW_RESOLVE flag as well. This flag is going away in SpiderMonkey 38.
Created attachment 343621 [details] [review] WIP - refactor keep-alive Keep-alive would need to change in SpiderMonkey 38 (and really should have already for SpiderMonkey 31, but I didn't realize!) because it uses JSObject pointers to generate its hash values. Since the GC can move JSObjects around, that is not a good thing to use for a hash value. This makes keep-alive a lot simpler by using JS::PersistentRootedObject (which does not get moved around by the GC). Instead of being a JSObject itself, GjsKeepAlive is a pure C++ object, a set which stores GjsKeepAliveObjects. The GjsKeepAliveObject is just JS::PersistentRootedObject augmented with a destroy notify function and a hash value. Instead of sticking all the objects in a big keep-alive object that's kept on the global object, this allows each translation unit to have its own GjsKeepAlive. This allows us to get rid of the awkward iteration functions where you have to skip over objects added by other code by keying off their destroy notify functions. GjsKeepAlive is a std::unordered_set, so uses the normal standard library API. It offers two additional methods, add_object() and remove_object(), so you don't have to deal with adding an extra root to an object just to add or remove it from the keep-alive. GjsKeepAlive registers itself with the GjsContext and unroots any remaining objects when the GjsContext is destroyed. TODO: Write tests after validating this approach.
Review of attachment 343621 [details] [review]: Ok, so I looked into this, and all in all it looks sane. Two comments: 1) The original design was agnostic of context destruction, because at context destruction the keep alive would be unreachable and everything would be GCed. The new design uses explicit roots and explicit destruction. While this new design involves less code overall, it also requires some pretty hard to follow codependence between context and keep-alive. Maybe it would be easier to solve the existing problem (an unchanging pointer key for the hashtable) than the rewrite. The GClosure and GObject ptrs seem obvious candidates for this. 2) I don't like that you inherit from std::unordered_set directly, and you don't use it anywhere. Composition would make more sense, but if you want to avoid extra characters, maybe private inheritance? 3) I don't like the static allocation of GjsKeepAlive. What if I create a context, then tear it down, then create a new one?
Review of attachment 343347 [details] [review]: Ok
Review of attachment 343279 [details] [review]: Ok
Review of attachment 343281 [details] [review]: Makes sense
Review of attachment 343282 [details] [review]: Ok
Review of attachment 343283 [details] [review]: g_autoptr? But ok
Review of attachment 343619 [details] [review]: Not an issue with this patch, but I noticed the same problem that the closure code had: strictly speaking, you need to invoke the c++ constructor on any struct that has members with non-trivial constructors, such as struct _Fundamental, which has a member of type js::Heap. I'm not sure how much it matters in practice.
Review of attachment 343620 [details] [review]: Ok
Attachment 343282 [details] pushed as 6529051 - tests: Fix double-declared variable Attachment 343347 [details] pushed as 785bd8d - js: Adapt to upcoming JS::Heap API Attachment 343620 [details] pushed as 42ffb15 - js: Remove JSCLASS_NEW_RESOLVE where not used Other attachments pushed too, but git-bz doesn't like it when you have a link to the mozilla bugzilla instance in your commit message :-P
Thanks for the reviews, Giovanni! (In reply to Giovanni Campagna from comment #28) > Review of attachment 343621 [details] [review] [review]: > > Ok, so I looked into this, and all in all it looks sane. > > Two comments: > > 1) The original design was agnostic of context destruction, because at > context destruction the keep alive would be unreachable and everything would > be GCed. > The new design uses explicit roots and explicit destruction. > > While this new design involves less code overall, it also requires some > pretty hard to follow codependence between context and keep-alive. > > Maybe it would be easier to solve the existing problem (an unchanging > pointer key for the hashtable) than the rewrite. The GClosure and GObject > ptrs seem obvious candidates for this. After writing this refactor I found out about JS_AddWeakPointerCallback() and JS_UpdateWeakPointerAfterGC() which are new in SpiderMonkey 38. The comment says | Weak pointers are by their nature not marked as part of garbage collection, | but they may need to be updated in two cases after a GC: | | 1) Their referent was found not to be live and is about to be finalized | 2) Their referent has been moved by a compacting GC | | To handle this, any part of the system that maintain weak pointers to | JavaScript GC things must register a callback with | JS_(Add,Remove)WeakPointerCallback(). This callback must then call | JS_UpdateWeakPointerAfterGC() on all weak pointers it knows about. | | The argument to JS_UpdateWeakPointerAfterGC() is an in-out param. If the | referent is about to be finalized the pointer will be set to null. If the | referent has been moved then the pointer will be updated to point to the new | location. | | Callers of this method are responsible for updating any state that is | dependent on the object's address. For example, if the object's address is | used as a key in a hashtable, then the object must be removed and | re-inserted with the correct hash. However, that seems to imply that if the JS::Heap _isn't_ a weak pointer, i.e. is traced, then you don't have to do the above... so maybe we don't need this refactor after all. I'll try my mozjs38 branch without it. > 2) I don't like that you inherit from std::unordered_set directly, and you > don't use it anywhere. > Composition would make more sense, but if you want to avoid extra > characters, maybe private inheritance? I do use its clear() method when destructing the context. > 3) I don't like the static allocation of GjsKeepAlive. What if I create a > context, then tear it down, then create a new one? Good point. (In reply to Giovanni Campagna from comment #34) > Review of attachment 343619 [details] [review] [review]: > > Not an issue with this patch, but I noticed the same problem that the > closure code had: strictly speaking, you need to invoke the c++ constructor > on any struct that has members with non-trivial constructors, such as struct > _Fundamental, which has a member of type js::Heap. I'm not sure how much it > matters in practice. Ah, that answers the question I had about your IMPLEMENTS_BARRIERS patch. Will fix. I still have one fairly large problem to overcome, maybe you have some suggestion? The JSObject in GjsCallbackTrampoline currently is rooted if it's not a vfunc, otherwise it's a weak pointer (though it is reachable from the prototype of the GObject of which it is a vfunc.) This will be difficult to do in SpiderMonkey 38 since you can't root JS::Heap wrappers anymore. I've been poring over your patch https://bugzilla.gnome.org/attachment.cgi?id=218425&action=diff but I don't understand it well enough yet to know if it will help with that.
Created attachment 343695 [details] [review] js: Implement barriers correctly in structs Fundamental was the only class that didn't have the IMPLEMENTS_BARRIERS flag set. In SpiderMonkey 38 it's required on all classes that have a trace hook. In addition, in order to implement barriers correctly we need to make sure that JS::Heap's constructor and destructor are called. In most cases we do this by making the containing struct a C++ struct, and calling its constructor and destructor. In the case of GjsContext (a GObject struct) and GjsCoveragePrivate (a GObject private struct) we can't do that, so instead we use placement new on the JS::Heap member.
I'm not a huge fan of the placement new in the above patch, since it means two-step construction every time you need to create one of the structs, which is error-prone in my opinion. But I guess it's the only way to keep using GSlice for allocation...
This seemes to be ongoing work, but after finishing up could you please check on feddora 25 or rawhide. 1.47.4 landed in rawhide and dies in a fortify smash protection. I tried building git in 25 with much the same result.
(In reply to Philip Chimento from comment #37) > However, that seems to imply that if the JS::Heap _isn't_ a weak pointer, > i.e. is traced, then you don't have to do the above... so maybe we don't > need this refactor after all. I'll try my mozjs38 branch without it. What I said here was wrong, pointers can be moved when traced, but their location is updated during JS_CallHeapWhateverTracer(). So that would be a good time to update the hashtable key.
(In reply to Yanko Kaneti from comment #40) > This seemes to be ongoing work, but after finishing up could you please > check on feddora 25 or rawhide. 1.47.4 landed in rawhide and dies in a > fortify smash protection. I tried building git in 25 with much the same > result. Sure, I'll check it out. Where did you get the mozjs31 package, also from rawhide? I've found that occasionally if you aren't compiling mozjs31 with the right settings then stuff like that can happen because their struct layouts in their header files can change depending on configure-time settings.
Created attachment 343763 [details] [review] function: Two wrappers in GjsCallbackTrampoline The previous situation was that the JS::Value holding the a trampoline's function was stored in a JS::Heap wrapper. If the function was owned by the trampoline (the normal case), then it was rooted. If the function was a vfunc, in which case it was owned by the GObject class prototype and the trampoline was essentially leaked, then it remained a weak pointer. In SpiderMonkey 38, JS::AddValueRoot() and friends are going away, in favour of JS::PersistentRooted. However, JS::PersistentRooted does not have the option of maintaining a weak pointer to the value. Therefore, we use a JS::Heap to store a vfunc value and JS::PersistentRooted* otherwise. We maintain the invariant that one of the two must be null and provide a function to get the function value from the appropriate wrapper. This will still need to change further, since weak pointers must be updated when they are moved by the GC.
(In reply to Philip Chimento from comment #42) > (In reply to Yanko Kaneti from comment #40) > > This seemes to be ongoing work, but after finishing up could you please > > check on feddora 25 or rawhide. 1.47.4 landed in rawhide and dies in a > > fortify smash protection. I tried building git in 25 with much the same > > result. > > Sure, I'll check it out. Where did you get the mozjs31 package, also from > rawhide? I've found that occasionally if you aren't compiling mozjs31 with > the right settings then stuff like that can happen because their struct > layouts in their header files can change depending on configure-time > settings. Yep, mozjs31 from rawhide and respectively from f25 as packages when I tested git. My feeble bisecting efforts pointed to the switch to mozjs31 between 1.47.0 and 1.47.3. All the working gjs packages in fedora are/were built against mozjs24.
Created attachment 343764 [details] [review] keep-alive: Update hash keys when objects move The garbage collector can change the locations of objects. Since we use the object pointer as part of the hash function for the keep-alive hash table, an object moving would invalidate the hash table key. Therefore, we remove and reinsert each key if tracing changed the pointer's location.
Created attachment 343847 [details] [review] jsapi-util: Use JS_PutEscapedString() In gjs_string_readable(), instead of doing our own escaping of strings, we can use JS_PutEscapedString(). Since JS_GetStringCharsAndLength() is going away in SpiderMonkey 38, we have to change this code anyway.
Created attachment 343927 [details] [review] context: Empty heap wrapper after removing tracer This did not break before, but in SpiderMonkey 38 not doing this is more likely to cause crashes because the pointer stored in the JS::Heap wrapper is more likely to become invalid once it is no longer traced. This would crash under SpiderMonkey 38 when executing JS::Heap's destructor, so we should instead empty out the wrapper after removing the tracer.
Created attachment 343928 [details] [review] boxed: Use JSNative property accessors Property accessors of JSPropertyOp and JSStrictPropertyOp are going away in future versions of SpiderMonkey, to be replaced by JSNative. Unfortunately the property name was passed to the PropertyOp callbacks as a jsid, and it is not available to JSNative callbacks. The property accessors in Boxed do require the property name, however, so they can look up the GIFieldInfo. In order to achieve this we follow a suggestion from a Mozilla mailing list [1]: wrap the JSNative accessors in JSFunction objects, which can have "reserved slots" in which to store information with which we can retrieve the field info. For this, we need to use the jsfriend API, but such use is isolated to two functions. [1] https://groups.google.com/forum/#!msg/mozilla.dev.tech.js-engine.internals/TZznbH80zJw/BmRrMsuuAwAJ
Created attachment 343929 [details] [review] format: More fixes for standard String.replace() This is a followup to commit 2cf42c598c0e43394b3fe1cb7d50d3ed716ca3c4 where parameters to the callback given to String.replace() may be undefined, not '', for groups that are not matched. In SpiderMonkey 38, String.replace() was changed to have the standard behaviour.
Yanko, you may be suffering from https://bugzilla.mozilla.org/show_bug.cgi?id=1083464. I don't know if Fedora's mozjs31 has a new enough version that includes the fix for that; I've been using 31.5.0 and I believe Fedora has 31.2.0. In any case, I was able to reproduce your crash, and fix it by adding "#define JSGC_USE_EXACT_ROOTING 1" to gjs/jsapi-wrapper.h. However, the real fix is to upgrade Fedora's mozjs31 to 31.5.0 or compile it with these two patches applied: - https://hg.mozilla.org/releases/mozilla-esr31/rev/15d1c2a4ea55 - https://hg.mozilla.org/releases/mozilla-esr31/rev/d74ec3052a66
Thanks for looking into it Philip. Shortly after mozjs31 in rawhide was updated to 31.5.0 and the crash was fixed. I didn't realize there was a newer mozjs31 or I probably would've tested that before bothering you. Sorry.
Created attachment 344081 [details] [review] js: Refactor gjs_object_*_property() This code was due for some refactoring, and changing GjsConstString to be rooted in the following commit was the opportunity for it. We now have gjs_object_get_property(), gjs_object_has_property(), gjs_object_set_property(), and gjs_object_define_property() as wrappers for JS_GetPropertyById(), etc., that take a GjsConstString constant instead of a jsid. In addition, we rename gjs_object_require_property_value() to be an overload of gjs_object_require_property(); the old name was confusing because the name _without_ "value" was the one that dealt with a JS::Value! Same rename for gjs_object_require_converted_property_value(). This whole thing allows us to get rid of some code, moving a bunch of roots into these new functions. These roots are strictly speaking still necessary, but in the next commit we're going to root all the interned strings permanently, which should have been done all along.
Created attachment 344082 [details] [review] context: Root interned strings We put the collection of interned strings into an array of JS::PersistentRootedId to make sure they are not garbage-collected; otherwise there was nothing keeping them from being freed.
Created attachment 344190 [details] [review] keep-alive: Remove custom hash table handling The gjs_g_hash_table_steal_one() function was probably written before GHashTableIter was in the API. We can replace it by GHashTableIter and remove the custom API. For some reason, the custom API was causing the keep-alive finalizer to crash under SpiderMonkey 38.
Created attachment 344285 [details] [review] js: Refactor dual use of JS::Heap wrapper The previous situation was that a JS::Heap wrapper was used to root a GC thing under some conditions using JS::AddFooRoot() or the keep-alive object, and trace or maintain a weak pointer otherwise. This will not be possible anymore in SpiderMonkey 38. JS::AddFooRoot() and JS::RemoveFooRoot() are going away, in favour of JS::PersistentRootedFoo. The keep-alive object has its own problems, because the SpiderMonkey 38 garbage collector will move GC things around, so anything referring to a GC thing on the keep-alive object will have to know when to update its JS::Heap wrapper when the GC thing moves. This previous situation existed in two places. (1) The JS::Value holding a trampoline's function. If the function was owned by the trampoline (the normal case), then it was rooted. If the function was a vfunc, in which case it was owned by the GObject class prototype and the trampoline was essentially leaked, then it remained a weak pointer. (2) The JSObject associated with a closure. In the normal case the object and the closure had the same lifetime and the object was rooted. Similar to above, if the closure was a signal it was owned by the GObject class, and traced. For both of these places we now use GjsMaybeOwned, a wrapper that sticks its GC thing in either a JS::PersistentRootedFoo, if the thing is intended to be rooted, or a JS::Heap<Foo>, if it is not. If rooted, the GjsMaybeOwned holds a weak reference to the GjsContext, and therefore can send out a notification when the context's dispose function is run, similar to existing functionality of the keep-alive object. This will still need to change further after the switch to SpiderMonkey 38, since weak pointers must be updated when they are moved by the GC. (This is technically already the case in SpiderMonkey 31, but the API makes it difficult to do correctly, and in practice it isn't necessary.)
Created attachment 344287 [details] [review] js: Refactor dual use of JS::Heap wrapper The previous situation was that a JS::Heap wrapper was used to root a GC thing under some conditions using JS::AddFooRoot() or the keep-alive object, and trace or maintain a weak pointer otherwise. This will not be possible anymore in SpiderMonkey 38. JS::AddFooRoot() and JS::RemoveFooRoot() are going away, in favour of JS::PersistentRootedFoo. The keep-alive object has its own problems, because the SpiderMonkey 38 garbage collector will move GC things around, so anything referring to a GC thing on the keep-alive object will have to know when to update its JS::Heap wrapper when the GC thing moves. This previous situation existed in two places. (1) The JS::Value holding a trampoline's function. If the function was owned by the trampoline (the normal case), then it was rooted. If the function was a vfunc, in which case it was owned by the GObject class prototype and the trampoline was essentially leaked, then it remained a weak pointer. (2) The JSObject associated with a closure. In the normal case the object and the closure had the same lifetime and the object was rooted. Similar to above, if the closure was a signal it was owned by the GObject class, and traced. For both of these places we now use GjsMaybeOwned, a wrapper that sticks its GC thing in either a JS::PersistentRootedFoo, if the thing is intended to be rooted, or a JS::Heap<Foo>, if it is not. If rooted, the GjsMaybeOwned holds a weak reference to the GjsContext, and therefore can send out a notification when the context's dispose function is run, similar to existing functionality of the keep-alive object. This will still need to change further after the switch to SpiderMonkey 38, since weak pointers must be updated when they are moved by the GC. (This is technically already the case in SpiderMonkey 31, but the API makes it difficult to do correctly, and in practice it isn't necessary.)
Review of attachment 343695 [details] [review]: Looks good.
Review of attachment 343764 [details] [review]: Makes sense to me.
Review of attachment 343847 [details] [review]: OK
Review of attachment 343927 [details] [review]: OK
Review of attachment 343928 [details] [review]: Looks good.
Review of attachment 343929 [details] [review]: OK
Review of attachment 344081 [details] [review]: Nice cleanup!
Review of attachment 344082 [details] [review]: Looks good.
Review of attachment 344190 [details] [review]: Sure
Review of attachment 344287 [details] [review]: I did not try to follow closely all the invariants in jsapi-util-root.h, which could probably use a comment in the file, but the approach and code look overall good to me.
Attachment 343695 [details] pushed as bb16545 - js: Implement barriers correctly in structs Attachment 343764 [details] pushed as 48a13bf - keep-alive: Update hash keys when objects move Attachment 343847 [details] pushed as c9c2c7d - jsapi-util: Use JS_PutEscapedString() Attachment 343927 [details] pushed as d857b26 - context: Empty heap wrapper after removing tracer Attachment 343928 [details] pushed as 5148c2e - boxed: Use JSNative property accessors Attachment 343929 [details] pushed as c5b0c1b - format: More fixes for standard String.replace() Attachment 344081 [details] pushed as f938056 - js: Refactor gjs_object_*_property() Attachment 344082 [details] pushed as bc988e5 - context: Root interned strings Attachment 344190 [details] pushed as 3bc03f1 - keep-alive: Remove custom hash table handling
(In reply to Cosimo Cecchi from comment #66) > Review of attachment 344287 [details] [review] [review]: > > I did not try to follow closely all the invariants in jsapi-util-root.h, > which could probably use a comment in the file, but the approach and code > look overall good to me. OK. You're right, this probably bears some more explanation. In any case I would like to add some tests before committing it. I think we can eventually replace the keep-alive object with this entirely, but I still need to figure out how to do it properly, and it seems not to be strictly necessary for the port to SpiderMonkey 38.
Created attachment 344378 [details] [review] js: Refactor dual use of JS::Heap wrapper The previous situation was that a JS::Heap wrapper was used to root a GC thing under some conditions using JS::AddFooRoot() or the keep-alive object, and trace or maintain a weak pointer otherwise. This will not be possible anymore in SpiderMonkey 38. JS::AddFooRoot() and JS::RemoveFooRoot() are going away, in favour of JS::PersistentRootedFoo. The keep-alive object has its own problems, because the SpiderMonkey 38 garbage collector will move GC things around, so anything referring to a GC thing on the keep-alive object will have to know when to update its JS::Heap wrapper when the GC thing moves. This previous situation existed in two places. (1) The JS::Value holding a trampoline's function. If the function was owned by the trampoline (the normal case), then it was rooted. If the function was a vfunc, in which case it was owned by the GObject class prototype and the trampoline was essentially leaked, then it remained a weak pointer. (2) The JSObject associated with a closure. In the normal case the object and the closure had the same lifetime and the object was rooted. Similar to above, if the closure was a signal it was owned by the GObject class, and traced. For both of these places we now use GjsMaybeOwned, a wrapper that sticks its GC thing in either a JS::PersistentRootedFoo, if the thing is intended to be rooted, or a JS::Heap<Foo>, if it is not. If rooted, the GjsMaybeOwned holds a weak reference to the GjsContext, and therefore can send out a notification when the context's dispose function is run, similar to existing functionality of the keep-alive object. This will still need to change further after the switch to SpiderMonkey 38, since weak pointers must be updated when they are moved by the GC. (This is technically already the case in SpiderMonkey 31, but the API makes it difficult to do correctly, and in practice it isn't necessary.)
Review of attachment 344378 [details] [review]: Thanks, looks good to me now.
Created attachment 344443 [details] [review] WIP - make closure lifetime less complicated?
Giovanni, you might be interested in the GjsMaybeOwned refactor. I still hope to refactor keep-alive into that eventually, but it requires some JS::PersistentRooted API that's only in SpiderMonkey 38. Also, this latest patch _might_ be able to be squashed into the GjsMaybeOwned patch, if I've understood closure lifetime correctly. It seems to me that we don't need to check if the JSContext is valid anymore, since we're guaranteed to have our destroy notifier called before it is shut down.
Created attachment 344504 [details] [review] coverage: Stop using JSUnit in tests There was some hidden usage of JSUnit in the C++ coverage tests, where we imported the module inside a generated script that was executed from C++. It only used assertEquals() from the module, so we just add an equivalent of that function instead, then we don't have to import the module multiple times per test run.
Created attachment 344505 [details] [review] js: Be careful about undefined property accesses SpiderMonkey 38 warns rather more loudly if you access an undefined property. This adds checks to all the places where a normal run of the test suite would generate the warning. On the C++ side, the checks are done with JS_AlreadyHasOwnProperty() in order not to re-enter the whole resolve -> get machinery. This also adds a comment to gjs_object_require_property() stating not to use it if the property is not actually required, which is now the best practice. On the JS side, we simply write our JS code a bit more carefully.
Created attachment 344599 [details] [review] importer: Fix importer enumeration I broke this sometime around the mozjs31 transition, and never noticed because it is little-used and had no tests. It's going to need to change in mozjs38, so this fixes it and adds tests. The real fix is .setPrivate(iter) instead of .setPrivate(context), that was just a refactoring error. In order to test the functionality properly, however, we also need to make enumeration work for an imports.searchPath that contains resource:/// entries. For this, we switch from g_dir_read_name() to g_file_enumerate_children().
OK, I believe all the patches for this bug are done at this point. Remaining issue to solve is whether the patch "WIP - make closure lifetime less complicated?" is correct, and if so, can be squashed into "js: Refactor dual use of JS::Heap wrapper". Like last time, I'll open a new bug for changes that can only be committed after the switch.
Review of attachment 344599 [details] [review]: Just one comment on this one -- feel free to fix when pushing. ::: gjs/importer.cpp @@ +792,1 @@ + char *filename = g_file_get_basename(file); I think you want g_autofree here or it will be leaked.
Review of attachment 344505 [details] [review]: ::: gi/repo.cpp @@ +80,3 @@ } + return gjs_object_require_property(context, versions, NULL, ns_id, version); Is the behavior change (return false and not clear the exception when this fails) intended as part of this patch? ::: gjs/importer.cpp @@ +428,3 @@ + */ +static bool +import_symbol_from_init_js(JSContext *cx, Not sure why this needs to return a bool here, since you never check the return value from the caller.
Review of attachment 344504 [details] [review]: Looks good.
Ugh, I accidentally pushed this one. Sorry! I'll make any changes in a follow-up patch. That said, I think if I can clarify the comments, you might agree with me that nothing needs to change. (In reply to Cosimo Cecchi from comment #78) > Review of attachment 344505 [details] [review] [review]: > > ::: gi/repo.cpp > @@ +80,3 @@ > } > > + return gjs_object_require_property(context, versions, NULL, ns_id, > version); > > Is the behavior change (return false and not clear the exception when this > fails) intended as part of this patch? I think there's only a slight behaviour change here and I believe it's an improvement. Clearing the exception was intended for the case where the property was not there. Now we check for that up front. If there actually is an error getting the property, then we should throw that. The previous code didn't distinguish between a "real" error and the property not being there. > ::: gjs/importer.cpp > @@ +428,3 @@ > + */ > +static bool > +import_symbol_from_init_js(JSContext *cx, > > Not sure why this needs to return a bool here, since you never check the > return value from the caller. I wanted to stick to the pattern. I could make *result the return value instead, but that would be confusing alongside all the other functions that do it this way.
Created attachment 344888 [details] [review] js: Refactor dual use of JS::Heap wrapper The previous situation was that a JS::Heap wrapper was used to root a GC thing under some conditions using JS::AddFooRoot() or the keep-alive object, and trace or maintain a weak pointer otherwise. This will not be possible anymore in SpiderMonkey 38. JS::AddFooRoot() and JS::RemoveFooRoot() are going away, in favour of JS::PersistentRootedFoo. The keep-alive object has its own problems, because the SpiderMonkey 38 garbage collector will move GC things around, so anything referring to a GC thing on the keep-alive object will have to know when to update its JS::Heap wrapper when the GC thing moves. This previous situation existed in two places. (1) The JS::Value holding a trampoline's function. If the function was owned by the trampoline (the normal case), then it was rooted. If the function was a vfunc, in which case it was owned by the GObject class prototype and the trampoline was essentially leaked, then it remained a weak pointer. (2) The JSObject associated with a closure. In the normal case the object and the closure had the same lifetime and the object was rooted. Similar to above, if the closure was a signal it was owned by the GObject class, and traced. For both of these places we now use GjsMaybeOwned, a wrapper that sticks its GC thing in either a JS::PersistentRootedFoo, if the thing is intended to be rooted, or a JS::Heap<Foo>, if it is not. If rooted, the GjsMaybeOwned holds a weak reference to the GjsContext, and therefore can send out a notification when the context's dispose function is run, similar to existing functionality of the keep-alive object. This will still need to change further after the switch to SpiderMonkey 38, since weak pointers must be updated when they are moved by the GC. (This is technically already the case in SpiderMonkey 31, but the API makes it difficult to do correctly, and in practice it isn't necessary.)
Created attachment 344889 [details] [review] WIP - make closure lifetime less complicated?
Created attachment 344890 [details] [review] WIP - object: switch to C++ struct
Created attachment 344891 [details] [review] WIP - object: split out clear pending toggles
Created attachment 344892 [details] [review] WIP - remove keep-alive
Created attachment 344893 [details] [review] WIP - object: fully use GjsMaybeOwned wrapper
These patches that I just added, could be squashed into "js: Refactor dual use of JS::Heap wrapper", depending on what is clearer. The problem now is that we really have to get rid of keep-alive.cpp. Adding the IMPLEMENTS_BARRIERS flag to all classes, causes SpiderMonkey to enable incremental GC. If incremental GC is enabled, then you need to adhere to the invariants of pre write barriers, and SpiderMonkey will check this for you if you run it with JS_GC_ZEAL=4. The problem is that keep-alive doesn't do this, because in gjs_keep_alive_remove_child() we have the following (abridged): Child child; child.child = obj; g_hash_table_remove(blah, &child); This allocates a Child, containing a JS::Heap<JSObject *>, on the stack, which is forbidden. I originally thought it could be fixed by allocating it on the heap: Child *child = new Child(); child->child = obj; g_hash_table_remove(blah, child); delete child; But that won't work either, since the JS::Heap in this heap-instance of Child is not traced. So, I went ahead with refactoring keep-alive. I hope I've been able to address some of Giovanni's comments from my original refactor. I think it's improved, but this is still some pretty gnarly code.
Created attachment 345010 [details] [review] fundamental: Don't trace uninitialized memory This was tracing both fundamental instances and prototypes. Only the prototype holds a reference to the constructor name which needs to be traced; in the instance case, we were just tracing random memory.
Created attachment 345011 [details] [review] closure: Clear closure functions in idle handler When closures are invalidated, they need to drop the reference to their JS functions. However, it appears that closure invalidate notifications can be sent out during garbage collection. Incremental GC does not allow changing what you trace while GC is going on, so we have to keep tracing the JS functions, and only drop them in an idle handler later on. We still decrement the memory counter in the invalidate notification, though, because if you are doing a memory counter check at the end of your program, the idle handler may not have run yet.
Created attachment 345012 [details] [review] WIP - object: fully use GjsMaybeOwned wrapper
Created attachment 345069 [details] [review] system: Add clarification to System.addressOf() SpiderMonkey can move objects around in memory during garbage collection, so System.addressOf() cannot be relied on to identify that an object is the same as another object. At the same time, SpiderMonkey 38 also gained the capability to deduplicate identical objects in memory, so System.addressOf() can also not be relied on to identify that two identical objects are different instances. Adjust our test suite accordingly.
Created attachment 345070 [details] [review] object: Trace vfunc trampolines Previously these trampolines were leaked, and the JS functions they pointed to were treated as weak pointers. However, we can tie them to the lifetime of the prototype and free them when the prototype's private struct is freed, as well as trace them during garbage collection.
Review of attachment 345010 [details] [review]: Looks good, with an optional suggestion ::: gi/fundamental.cpp @@ +516,3 @@ + auto priv = static_cast<Fundamental *>(JS_GetPrivate(obj)); + if (priv == nullptr || priv->prototype != nullptr) + return; /* Only prototypes need tracing */ Would be nice to add a fundamental_is_prototype() check, since it's done in a few slightly different ways at different places in fundamental.cpp
Review of attachment 345011 [details] [review]: OK
Review of attachment 345069 [details] [review]: Sure
Review of attachment 345070 [details] [review]: OK
Created attachment 345071 [details] [review] object: Trace vfunc trampolines Previously these trampolines were leaked, and the JS functions they pointed to were treated as weak pointers. We still have to leak the trampoline structs themselves, because the GType's vtable still refers to them. However, we can tie the JS functions to the lifetime of the prototype and trace them during garbage collection, which will become required in SpiderMonkey 38 when they can be moved around during GC.
Attachment 345010 [details] pushed as b5bab08 - fundamental: Don't trace uninitialized memory Attachment 345069 [details] pushed as 64d0b93 - system: Add clarification to System.addressOf()
Created attachment 345072 [details] [review] object: split out clear pending toggles FIXME: Maybe this should be squashed into the GjsMaybeOwned patch. We separate out the two steps in gjs_object_prepare_shutdown(): handling any pending toggle references, and releasing all the native objects. When switching to GjsContext destroy notifications with GjsMaybeOwned, we'll need to handle the idle toggles before doing a garbage collection.
Created attachment 345073 [details] [review] object: switch to C++ struct FIXME: Maybe this should be squashed into the GjsMaybeOwned patch. This will be necessary because we are going to convert the raw JSObject * member of ObjectInstance to a heap wrapper.
Created attachment 345074 [details] [review] js: Refactor dual use of JS::Heap wrapper The previous situation was that a JS::Heap wrapper was used to root a GC thing under some conditions using JS::AddFooRoot() or the keep-alive object, and trace or maintain a weak pointer otherwise. This will not be possible anymore in SpiderMonkey 38. JS::AddFooRoot() and JS::RemoveFooRoot() are going away, in favour of JS::PersistentRootedFoo. The keep-alive object has its own problems, because the SpiderMonkey 38 garbage collector will move GC things around, so anything referring to a GC thing on the keep-alive object will have to know when to update its JS::Heap wrapper when the GC thing moves. This previous situation existed in two places. (1) The JS::Value holding a trampoline's function. If the function was owned by the trampoline (the normal case), then it was rooted. If the function was a vfunc, in which case it was owned by the GObject class prototype and the trampoline was essentially leaked, then it remained a weak pointer. (2) The JSObject associated with a closure. In the normal case the object and the closure had the same lifetime and the object was rooted. Similar to above, if the closure was a signal it was owned by the GObject class, and traced. For both of these places we now use GjsMaybeOwned, a wrapper that sticks its GC thing in either a JS::PersistentRootedFoo, if the thing is intended to be rooted, or a JS::Heap<Foo>, if it is not. If rooted, the GjsMaybeOwned holds a weak reference to the GjsContext, and therefore can send out a notification when the context's dispose function is run, similar to existing functionality of the keep-alive object. This will still need to change further after the switch to SpiderMonkey 38, since weak pointers must be updated when they are moved by the GC. (This is technically already the case in SpiderMonkey 31, but the API makes it difficult to do correctly, and in practice it isn't necessary.)
Created attachment 345075 [details] [review] WIP - make closure lifetime less complicated? FIXME: This seems to work fine, but I'm not entirely sure about it. FIXME: Maybe this should be squashed into the GjsMaybeOwned patch. Since we now get a notification when the GjsContext is disposed, and therefore just before the JSContext is destroyed, we don't need to keep track of which JSContexts are still valid.
Created attachment 345076 [details] [review] WIP - remove keep-alive FIXME: Maybe this should be squashed into the GjsMaybeOwned patch. FIXME: Maybe needs some tests? FIXME: Still needs a commit message.
Created attachment 345077 [details] [review] WIP - object: fully use GjsMaybeOwned wrapper FIXME: Maybe should be squashed into the GjsMaybeOwned patch? FIXME: Still needs a commit message.
Created attachment 345078 [details] [review] object: Trace vfunc trampolines Previously these trampolines were leaked, and the JS functions they pointed to were treated as weak pointers. We still have to leak the trampoline structs themselves, because the GType's vtable still refers to them. However, we can tie the JS functions to the lifetime of the prototype and trace them during garbage collection, which will become required in SpiderMonkey 38 when they can be moved around during GC.
OK, rebased everything that's left; I'm still undecided whether to squash most of these patches together into one big GjsMaybeOwned refactor that gets rid of keep-alive at the same time.
Review of attachment 345072 [details] [review]: Looks good.
Review of attachment 345073 [details] [review]: OK
Created attachment 345162 [details] [review] build: Allow more compiler inlining jsapi-util-args.h relies on inlining a lot of functions, into functions that are relatively small and therefore grow a lot. This started to cause warnings when switching to SpiderMonkey 38 (presumably because more methods in SpiderMonkey headers are inlined as well.) The default value for inline-unit-growth is 30, which according to the GCC documentation allows functions to grow by 30% as a result of inlining. Presumably this empirically chosen value of 50 will allow them to grow by 50%.
Attachment 345072 [details] pushed as 145f4db - object: split out clear pending toggles Attachment 345073 [details] pushed as f4e5ef6 - object: switch to C++ struct
Review of attachment 345074 [details] [review]: Still looks good to me.
Review of attachment 345075 [details] [review]: I like a lot how this code gets simplified, but I can only really trust you that it works fine :-) It makes sense to me in principle though -- is there any way we could put the condition that the previous behavior was guarding against in a test and verify that this does not break it?
Review of attachment 345076 [details] [review]: ::: gi/object.cpp @@ +27,3 @@ #include <stack> #include <string.h> +#include <set> Alphabetical order @@ +1331,3 @@ + bool inserted; + std::tie(std::ignore, inserted) = dissociate_list.insert(priv); + g_assert(inserted); I suggest that you factor this out in a disassociate_list_add() function... @@ +1566,3 @@ + priv->keep_alive.reset(); + size_t erased = dissociate_list.erase(priv); + g_assert(erased > 0); ...and this in a disassociate_list_remove() function. ::: gjs/context.cpp @@ +401,3 @@ "Destroying JS context"); + gjs_object_clear_toggles(); Not immediately clear to me why this was added here in this patch?
Review of attachment 345077 [details] [review]: This refactor makes sense to me
Review of attachment 345078 [details] [review]: OK
Review of attachment 345162 [details] [review]: Sure
Attachment 345074 [details] pushed as cc71606 - js: Refactor dual use of JS::Heap wrapper Attachment 345162 [details] pushed as 62a998a - build: Allow more compiler inlining
(In reply to Cosimo Cecchi from comment #112) > Review of attachment 345075 [details] [review] [review]: > > I like a lot how this code gets simplified, but I can only really trust you > that it works fine :-) > It makes sense to me in principle though -- is there any way we could put > the condition that the previous behavior was guarding against in a test and > verify that this does not break it? I have thought pretty hard about this, but the only answer I have come up with is "does not apply". The previous code would clean up any closures left over when the GjsContext / JSContext were destroyed. However, now with GjsMaybeOwned, all outstanding roots on closures are released just before that, so it just shouldn't happen at all anymore. So, the best test in my opinion would be to try to make a closure outlast the context, and let our leak checker catch it if it's not freed. And (I think) that's what this test does: https://git.gnome.org/browse/gjs/tree/installed-tests/js/testMainloop.js#n85
(In reply to Philip Chimento from comment #118) > I have thought pretty hard about this, but the only answer I have come up > with is "does not apply". The previous code would clean up any closures left > over when the GjsContext / JSContext were destroyed. However, now with > GjsMaybeOwned, all outstanding roots on closures are released just before > that, so it just shouldn't happen at all anymore. > > So, the best test in my opinion would be to try to make a closure outlast > the context, and let our leak checker catch it if it's not freed. And (I > think) that's what this test does: > https://git.gnome.org/browse/gjs/tree/installed-tests/js/testMainloop.js#n85 Cool... I'd say then let's put this in.
Attachment 345011 [details] pushed as 334ba96 - closure: Clear closure functions in idle handler
Created attachment 345281 [details] [review] object: Refactor to use GjsMaybeOwned instead of keep-alive In object.cpp, we previously kept a GObject's JS wrapper object alive by adding it to the keep-alive object. We can now use GjsMaybeOwned for this purpose if we add an accessor function to check whether the GjsMaybeOwned is in rooted mode. This is in preparation for an even deeper refactor where we replace the JS::Heap wrapper stored in the object's qdata with GjsMaybeOwned as well. This requires a small change to GjsContext's dispose handler so that we handle all pending toggle notifications before doing the first garbage collection.
Created attachment 345282 [details] [review] WIP - object: Fully use GjsMaybeOwned wrapper FIXME: One test does not pass FIXME: I think we need to make GjsMaybeOwned thread safe Previously, when not needing to keep a JS wrapper object alive, we would unroot it from the GjsMaybeOwned that is part of its private struct, and instead move it to a JS::Heap wrapper in the GObject's qdata where it was kept as a weak pointer. By adding switch_to_rooted() and switch_to_unrooted() API to GjsMaybeOwned, we can instead use GjsMaybeOwned for both cases. We also need to add an update_after_gc() method in order to store a weak pointer in the GjsMaybeOwned, since we will need to update the pointer's location after every garbage collection in SpiderMonkey 38. Because we still need to get to the object's private struct given the GObject, we can use the object's qdata for that purpose instead.
Created attachment 345392 [details] [review] jsapi-util-root: Don't destroy GjsMaybeOwned in notify Previously the code claimed that it was safe to delete a GjsMaybeOwned inside its context-destroyed notification callback. However, this is not true. Remove the comment saying so, and remove the test that tests it. We did not actually use this feature in the code.
Created attachment 345509 [details] [review] object: Fully use GjsMaybeOwned wrapper Previously, when not needing to keep a JS wrapper object alive, we would unroot it from the GjsMaybeOwned that is part of its private struct, and instead move it to a JS::Heap wrapper in the GObject's qdata where it was kept as a weak pointer. By adding switch_to_rooted() and switch_to_unrooted() API to GjsMaybeOwned, we can instead use GjsMaybeOwned for both cases. We also need to add an update_after_gc() method in order to store a weak pointer in the GjsMaybeOwned, since we will need to update the pointer's location after every garbage collection in SpiderMonkey 38. Because we still need to get to the object's private struct given the GObject, we can use the object's qdata for that purpose instead. There is one test that must be skipped for the time being: SpiderMonkey 31's garbage collector is not smart enough to collect the object right after switch_to_unrooted(). This test passes in SpiderMonkey 38.
Review of attachment 345281 [details] [review]: Looks good.
Review of attachment 345392 [details] [review]: OK
Review of attachment 345509 [details] [review]: Feel free to push with this small comment revised. ::: gi/object.cpp @@ +2177,3 @@ + g_assert(priv->keep_alive == obj); + // g_assert(!priv->keep_alive.rooted()); Any reason this is commented out?
(In reply to Cosimo Cecchi from comment #127) > Review of attachment 345509 [details] [review] [review]: > > Feel free to push with this small comment revised. > > ::: gi/object.cpp > @@ +2177,3 @@ > > + g_assert(priv->keep_alive == obj); > + // g_assert(!priv->keep_alive.rooted()); > > Any reason this is commented out? Nope, leftover from debugging. The assertion is not correct anyhow, since associate_js_object() will root the object.
Attachment 345078 [details] pushed as a871796 - object: Trace vfunc trampolines Attachment 345281 [details] pushed as ea43a1c - object: Refactor to use GjsMaybeOwned instead of keep-alive Attachment 345392 [details] pushed as 4b0523c - jsapi-util-root: Don't destroy GjsMaybeOwned in notify Attachment 345509 [details] pushed as 7e19a0c - object: Fully use GjsMaybeOwned wrapper