GNOME Bugzilla – Bug 751252
support new mozjs 31.5
Last modified: 2016-12-10 03:41:37 UTC
I am trying gnome3 for the first time this week since the gnome2 days. I noticed incremental memory increases every time I click Activities. this is gnome 3.16. I did a google search and found out this is because of the user interface is written in javascript. I couldn't find how to manually do garbage collection. Gjs is using mozjs 24. There is a more recent version https://people.mozilla.org/~sstangl/mozjs-31.5.0.tar.bz2 Is it possible to add support for it? Thank you!
Release notes https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Releases/31
Thanks for taking the time to report this. This particular bug has already been reported into our bug tracking system, but please feel free to report any further bugs you find. *** This bug has been marked as a duplicate of bug 742249 ***
Since I was going to open a new bug anyway for the post-mozjs31-switch patches, I may as well reopen this one. It turned out to be convenient to use #742249 for the patches that could apply to the existing mozjs24 code. But since that bug has over 70 patches now, it's getting a bit crowded. Use this bug also for discussion of when and how we should land this change and how to make sure it doesn't break stuff. The current status of this work is available at any time on the "wip/ptomato/mozjs31" branch. I will be rebasing and rewriting history heavily on that branch. Currently I have a bunch of patches that cannot be applied to master one by one, since all of them are needed to get GJS to compile with mozjs31. I could squash them all into one, but it would be a really awkward review. So I propose to post them here, then as they are reviewed land them on a separate branch: let's call it "mozjs31". When we are satisfied with the state of mozjs31, we can merge that branch into master.
Created attachment 339565 [details] [review] build: Build with mozjs31 Requires updating our includes, and also we can get rid of the jsfriendapi.h include altogether.
Created attachment 339566 [details] [review] js: Remove remaining usage of JSBool Since mozjs31 does away with JSBool entirely, we can now replace the remaining occurrences of it in the API with C++ bool.
Created attachment 339567 [details] [review] js: Use HandleValueArray as function param JS::HandleValueArray is a useful way to pass arrays of arguments around, has handy constructors such that you can pass in JS::CallArgs directly. Therefore we refactor some functions to take it instead of argc, argv. This will make the following commit easier where we adapt to the new APIs of JS_New(), JS_CallFunction(), etc. which require JS::HandleValueArray. This requires some changes in the closure callback marshal as previously we were passing in an array that was longer with some empty elements at the end, and relying on argc not to fall off the end. Now that there is no argc parameter, we have to rearrange things a bit in that function.
Created attachment 339568 [details] [review] js: Adapt to mozjs31 HandleValueArray APIs Some functions, such as JS_NewArrayObject() and JS_CallFunctionValue(), take a JS::HandleValueArray for their parameter lists instead of previously a number of arguments and vp pointer. This allows us to make some simplifications. In particular, JS_NewArrayObject() allows for one good refactor. With the mozjs24 API, the advice was to create an empty array with JS_NewArrayObject() and populate it with JS_DefineElement() so that the values did not get garbage collected from the vp array while the array was being populated [1]. However, that is not necessary anymore. Now we can simply fill up our rooted vector and pass it to JS_NewArrayObject(). [1] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_Reference/JS_NewArrayObject
Created attachment 339569 [details] [review] js: No need to use .address() in new API Since in mozjs31 all out parameters for GC things have been changed from pointers to MutableHandles, there is no need to get the underlying GC thing's address with JS::Handle::address(). Instead, we can simply use the unary & operator to get a MutableHandle from a Rooted.
Created attachment 339570 [details] [review] js: Pass JS::NullPtr() instead of NULL In APIs that now take a JS::HandleObject, we must pass JS::NullPtr() since JS::HandleObject cannot directly be constructed from NULL.
Created attachment 339571 [details] [review] js: Adhere to new JSClass struct layout This changes all JSClass declarations to be correct with the definition of JSClass in mozjs31.
Created attachment 339572 [details] [review] js: Adapt to new JS_DefineProperty API The setter and getter parameters now come after the flags, and passing NULL for them is no longer required, since they have default values in mozjs31. I believe that NULL (use the object's class's default setters and getters) is different from JSPropertyStub and JSStrictPropertyStub, so I left those in where they occurred. In mozjs31 a bunch of overloads were defined, where you can pass JS::HandleObject, JS::HandleString, int32_t, uint32_t, and double directly to JS_DefineProperty() instead of only JS::HandleValue. This allows us to get rid of some roots in our code where we created a JS::RootedValue for the sole purpose of passing some value to JS_DefineProperty().
Created attachment 339573 [details] [review] js: Remove deprecated JS_IsConstructing() In mozjs31 this is replaced with JS::CallArgs::isConstructing().
Created attachment 339574 [details] [review] js: Remove deprecated JS_ValueToString() In mozjs31 this is replaced with JS::ToString().
Created attachment 339575 [details] [review] js: Remove deprecated JS_{Get,Set}Options The options have been shuffled around, now some apply to the runtime (fetched with JS::RuntimeOptionsRef()) and some apply to the context (fetched with JS::ContextOptionsRef()).
Created attachment 339576 [details] [review] js: Remove deprecated JS_GetGlobalForScopeChain This was renamed to JS::CurrentGlobalOrNull in mozjs31.
Created attachment 339577 [details] [review] js: Remove deprecated jschar in favour of char16_t While jschar is not yet removed in mozjs31, it is now a simple typedef for char16_t. That means it switched signedness compared to mozjs24. This is slightly annoying for interoperability with GLib since gunichar2 is unsigned, but we reinterpret_cast where necessary.
Created attachment 339578 [details] [review] keep-alive: Remove deprecated JS_SET_TRACING_DETAILS This macro doesn't exist anymore in mozjs31, and its function seems to have been taken over by the "name" argument to JS_Call*Tracer().
Created attachment 339579 [details] [review] object: Use PersistentRooted in object_init_list Previously using JS_AddObjectRoot and JS_RemoveObjectRoot to keep objects alive from the time that the JS Object is created until the GObject instance_init function runs. Now we have JS::PersistentRootedObject which accomplishes the same thing but with a cleaner API. (The API of JS_AddObjectRoot and friends changes in mozjs31 to take only JS::Heap-wrapped objects, so it had to change one way or the other.)
Created attachment 339580 [details] [review] js: Fix misc APIs for mozjs31 This commit adapts to a few miscellaneous API changes in mozjs31 that don't fit anywhere else.
Created attachment 339581 [details] [review] js: Remove flags param from JSNewResolveOp The signature of JSNewResolveOp changes in mozjs31. It does not provide a flags parameter anymore to the callback. This is not caught at compile time because you have to cast the JSNewResolveOp callback pointer to JSResolveOp in order to fit it into the JSClass structure. Presumably this will go away once JSResolveOp is really deprecated in a future SpiderMonkey release.
Created attachment 339582 [details] [review] importer: Use new JS_GetOwnPropertyDescriptor() Technically it was incorrect to use JS_GetPropertyDescriptorById() here although it was unlikely to cause any problems.
Created attachment 339583 [details] [review] tests: Adapt to new Date.toLocaleDateString() This does not take a format string any longer, but instead locale and options parameters [1]. Now that it is possible to specify the locale, we can make this test into one with deterministic results. [1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl
Created attachment 339584 [details] [review] js: Set global object trace hook In mozjs31, global objects must have JS_GlobalObjectTraceHook() as their "trace" member.
Created attachment 339585 [details] [review] WIP - JS_Init() and JS_ShutDown() Starting with mozjs31, JS_Init() is required. Calling JS_ShutDown() on exit is not required, but may become so in the future. The one place where this is not working yet is in the case where a garbage collection is going on when you call System.exit(). In that case, the GC thread crashes.
Created attachment 339586 [details] [review] WIP - Destroy runtime If you call JS_ShutDown() with a runtime still in existence, then SpiderMonkey will shout at you. No kidding, the error message really says "FIX THIS!" This patch is for informational purposes only, I am going to rework it to do some reference counting so that the runtime is destroyed when the last GjsContext is destroyed. Or possibly make GjsContext a singleton. Note that in later SpiderMonkey versions there is only going to be one JSContext per JSRuntime allowed, and possibly the two are to be merged altogether.
Review of attachment 339565 [details] [review]: Looks good
Review of attachment 339566 [details] [review]: Looks good.
Review of attachment 339567 [details] [review]: Looks good to me.
Review of attachment 339568 [details] [review]: Nice, looks good.
Review of attachment 339569 [details] [review]: ::: gi/object.cpp @@ +2380,2 @@ underscore_name = hyphen_to_underscore((gchar *)pspec->name); + if (!JS_SetProperty(context, object, underscore_name, jsvalue)) This changes .address() for the actual reference. I think it makes sense, since jsvalue is the new value of the property, but the commit message does not mention this change. (Multiple occurrences in this patch.)
Review of attachment 339570 [details] [review]: Looks good.
Review of attachment 339571 [details] [review]: OK
Review of attachment 339572 [details] [review]: Nice cleanup!
Review of attachment 339573 [details] [review]: OK
Review of attachment 339574 [details] [review]: OK
Review of attachment 339575 [details] [review]: Looks good to me, with a small comment. ::: gjs/jsapi-util.cpp @@ -78,3 @@ if (!g_getenv("GJS_DISABLE_JIT")) { gjs_debug(GJS_DEBUG_CONTEXT, "Enabling JIT"); - options_flags |= JSOPTION_TYPE_INFERENCE | JSOPTION_ION | JSOPTION_BASELINE | JSOPTION_ASMJS; I guess JSOPTION_TYPE_INFERENCE has no replacement?
Review of attachment 339576 [details] [review]: OK
Review of attachment 339577 [details] [review]: OK
Review of attachment 339578 [details] [review]: OK
Review of attachment 339579 [details] [review]: Looks good.
Review of attachment 339580 [details] [review]: OK
Review of attachment 339581 [details] [review]: OK
Review of attachment 339582 [details] [review]: OK
Review of attachment 339583 [details] [review]: OK
Review of attachment 339584 [details] [review]: OK
Created attachment 339849 [details] [review] js: Adapt to mozjs31 HandleValueArray APIs Some functions, such as JS_NewArrayObject() and JS_CallFunctionValue(), take a JS::HandleValueArray for their parameter lists instead of previously a number of arguments and vp pointer. This allows us to make some simplifications. In particular, JS_NewArrayObject() allows for one good refactor. With the mozjs24 API, the advice was to create an empty array with JS_NewArrayObject() and populate it with JS_DefineElement() so that the values did not get garbage collected from the vp array while the array was being populated [1]. However, that is not necessary anymore. Now we can simply fill up our rooted vector and pass it to JS_NewArrayObject(). [1] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_Reference/JS_NewArrayObject
Created attachment 339850 [details] [review] maint: Remove erroneous comment I added this comment with an incorrect understanding of how JSAutoSaveExceptionState works. It can't be used for this purpose, since it doesn't save and restore the state of "no exception".
Created attachment 339851 [details] [review] WIP - JS_Init() and JS_ShutDown() Starting with mozjs31, JS_Init() is required. Calling JS_ShutDown() on exit is not required, but may become so in the future. The one place where this is not working yet is in the case where a garbage collection is going on when you call System.exit(). In that case, the GC thread crashes.
Created attachment 339852 [details] [review] WIP - Destroy runtime If you call JS_ShutDown() with a runtime still in existence, then SpiderMonkey will shout at you. No kidding, the error message really says "FIX THIS!" This patch is for informational purposes only, I am going to rework it to do some reference counting so that the runtime is destroyed when the last GjsContext is destroyed. Or possibly make GjsContext a singleton. Note that in later SpiderMonkey versions there is only going to be one JSContext per JSRuntime allowed, and possibly the two are to be merged altogether.
Created attachment 339855 [details] [review] js: Don't exit immediately from System.exit() Exiting directly will now crash the GC thread if there happens to be a GC going on at the time exit() is called; you can force this by running e.g. JS_GC_ZEAL=2,1 gjs -c 'imports.system.exit(0)' on a debug build of SpiderMonkey. This does what SpiderMonkey's shell itself does: sets a flag to tell the interpreter that the script wants to exit, then throws an uncatchable exception. When JS::Evaluate subsequently exits, the flag is checked and exit() is called.
Created attachment 339856 [details] [review] js: Call JS_Init() and JS_ShutDown() Starting with mozjs31, JS_Init() is required. Calling JS_ShutDown() on exit is not required, but may become so in the future. This makes two new public functions in the API which do the necessary initialization and shutdown, because gnome-shell will need to call them as well.
I committed the accepted patches to the "mozjs31" branch, instead of "master"; the whole thing is still available all together on "wip/ptomato/mozjs31". (In reply to Cosimo Cecchi from comment #30) > Review of attachment 339569 [details] [review] [review]: > > ::: gi/object.cpp > @@ +2380,2 @@ > underscore_name = hyphen_to_underscore((gchar *)pspec->name); > + if (!JS_SetProperty(context, object, underscore_name, jsvalue)) > > This changes .address() for the actual reference. I think it makes sense, > since jsvalue is the new value of the property, but the commit message does > not mention this change. (Multiple occurrences in this patch.) Not sure I get what you mean here. Did you mean that it's "jsvalue" and not "&jsvalue"? If so, that's just the SpiderMonkey API; JS_SetProperty used to take a JS::Value*, now it takes a JS::HandleValue (instead of JS::MutableHandleValue like many other functions do.) (In reply to Cosimo Cecchi from comment #36) > Review of attachment 339575 [details] [review] [review]: > ::: gjs/jsapi-util.cpp > @@ -78,3 @@ > if (!g_getenv("GJS_DISABLE_JIT")) { > gjs_debug(GJS_DEBUG_CONTEXT, "Enabling JIT"); > - options_flags |= JSOPTION_TYPE_INFERENCE | JSOPTION_ION | > JSOPTION_BASELINE | JSOPTION_ASMJS; > > I guess JSOPTION_TYPE_INFERENCE has no replacement? Correct, pushed with a clarification in the commit message.
Created attachment 339867 [details] [review] js: Don't exit immediately from System.exit() Exiting directly will now crash the GC thread if there happens to be a GC going on at the time exit() is called; you can force this by running e.g. JS_GC_ZEAL=2,1 gjs -c 'imports.system.exit(0)' on a debug build of SpiderMonkey. This does what SpiderMonkey's shell itself does: sets a flag to tell the interpreter that the script wants to exit, then throws an uncatchable exception. When JS::Evaluate subsequently exits, the flag is checked and exit() is called.
Created attachment 339868 [details] [review] js: Call JS_Init() and JS_ShutDown() Starting with mozjs31, JS_Init() is required. Calling JS_ShutDown() on exit is not required, but may become so in the future. This makes two new public functions in the API which do the necessary initialization and shutdown, because gnome-shell will need to call them as well.
Created attachment 339869 [details] [review] js: Destroy runtime when last context is destroyed If you call JS_ShutDown() with a runtime still in existence, then SpiderMonkey will shout at you. No kidding, the error message really says "FIX THIS!" This adds refcounting to the per-thread runtime, so that if no GjsContexts are referencing it anymore, then it is destroyed. Note that in later SpiderMonkey versions there is only going to be one JSContext per JSRuntime allowed, and in an as-yet unreleased version the two are to be merged altogether.
I wonder if we need version-checking macros now, so that gnome-shell can tell whether it needs to call gjs_init() and gjs_shutdown() or not?
(In reply to Philip Chimento from comment #52) > > This changes .address() for the actual reference. I think it makes sense, > > since jsvalue is the new value of the property, but the commit message does > > not mention this change. (Multiple occurrences in this patch.) > > Not sure I get what you mean here. Did you mean that it's "jsvalue" and not > "&jsvalue"? If so, that's just the SpiderMonkey API; JS_SetProperty used to > take a JS::Value*, now it takes a JS::HandleValue (instead of > JS::MutableHandleValue like many other functions do.) Exactly. Please mention that in the commit message before pushing it, which right now only mentions the unary & operator.
Review of attachment 339849 [details] [review]: OK
Review of attachment 339850 [details] [review]: OK
Review of attachment 339867 [details] [review]: Nice, makes sense.
Review of attachment 339869 [details] [review]: Looks good to me.
Review of attachment 339868 [details] [review]: I understand why you implemented it like this, but I don't like that gjs_context_eval() can return FALSE without setting the error. Any reason not to e.g. fabricate an error with a code that represents the fact that System.exit() was called?
Comment on attachment 339850 [details] [review] maint: Remove erroneous comment Attachment 339850 [details] pushed as 63f042c - maint: Remove erroneous comment
Created attachment 339979 [details] [review] js: Call JS_Init() and JS_ShutDown() Starting with mozjs31, JS_Init() is required. Calling JS_ShutDown() on exit is not required, but may become so in the future. This makes two new public functions in the API which do the necessary initialization and shutdown, because gnome-shell will need to call them as well.
Created attachment 339980 [details] [review] docs: Overview of new JS features in NEWS
(In reply to Cosimo Cecchi from comment #62) > Review of attachment 339868 [details] [review] [review]: > > I understand why you implemented it like this, but I don't like that > gjs_context_eval() can return FALSE without setting the error. Any reason > not to e.g. fabricate an error with a code that represents the fact that > System.exit() was called? This worked, though it was quite a bit more complicated, and I had to expose util/error.h in the public headers. (It actually should have been exposed already, because if the script failed then we would already throw an error in the GJS_ERROR domain.) This is going to have an effect on gnome-shell as well, since they will need to call gjs_init() and gjs_shutdown()... do you think we should add versioning macros to make it easier to tell whether those functions are present?
(In reply to Philip Chimento from comment #66) > This is going to have an effect on gnome-shell as well, since they will need > to call gjs_init() and gjs_shutdown()... do you think we should add > versioning macros to make it easier to tell whether those functions are > present? I am not a big fan of versioning macros... I have two thoughts on this: - can we make it so that GJS will abort or warn you when gjs_init() has not been called? This may be the case already, since you mentioned that JS_Init() is mandatory in mozjs31 - I'd rather gnome-shell depended directly on that specific version of GJS
Review of attachment 339979 [details] [review]: Looks good to me.
Review of attachment 339980 [details] [review]: Great!
(In reply to Cosimo Cecchi from comment #67) > (In reply to Philip Chimento from comment #66) > > This is going to have an effect on gnome-shell as well, since they will need > > to call gjs_init() and gjs_shutdown()... do you think we should add > > versioning macros to make it easier to tell whether those functions are > > present? > > I am not a big fan of versioning macros... > > I have two thoughts on this: > - can we make it so that GJS will abort or warn you when gjs_init() has not > been called? This may be the case already, since you mentioned that > JS_Init() is mandatory in mozjs31 It will abort, but the error message is quite cryptic if you aren't running a debug build of SpiderMonkey. > - I'd rather gnome-shell depended directly on that specific version of GJS OK, maybe I should send a message to gnome-shell-list explaining what changes will be required. Do you have any opinions on when we should merge the mozjs31 branch, now that all the commits have been acked?
(In reply to Philip Chimento from comment #70) > It will abort, but the error message is quite cryptic if you aren't running > a debug build of SpiderMonkey. I guess it's not worth it to try and catch that case to log a better message? > > - I'd rather gnome-shell depended directly on that specific version of GJS > > OK, maybe I should send a message to gnome-shell-list explaining what > changes will be required. Yeah, agreed. Would it be anything more complicated than simply calling gjs_init() and gjs_shutdown() at appropriate times? > Do you have any opinions on when we should merge the mozjs31 branch, now > that all the commits have been acked? I think so! Perhaps we should try to port gnome-shell and make sure it runs correctly first.
On the mailing list Emmanuele suggested using a static constructor and destructor. I think that could work, since there is no concern about ordering; as long as people don't try to use GJS API from other static constructors, which seems extremely unlikely to me.
Comment on attachment 339980 [details] [review] docs: Overview of new JS features in NEWS Attachment 339980 [details] pushed as b0eaf72 - docs: Overview of new JS features in NEWS
Created attachment 340097 [details] [review] js: Call JS_Init() and JS_ShutDown() Starting with mozjs31, JS_Init() is required. Calling JS_ShutDown() on exit is not required, but may become so in the future. This does so in the constructor and destructor of a static object. Normally this is discouraged because the order in which the constructors and destructors are called is not guaranteed, but I don't think that is a problem here since it's unlikely that anyone will try to use GJS API from a static constructor. However, API clients still must unref any GjsContext before the program shuts down. Usually this is not a problem, unless a JS script calls System.exit(), in which case the code would exit immediately without unreffing the GjsContext before JS_ShutDown was called. Instead, we make System.exit() throw an uncatchable exception, which is propagated all the way up to JS::Evaluate() and turned into a GError with code GJS_ERROR_SYSTEM_EXIT, thrown from gjs_context_eval(). For this, we need to expose GjsError and its error codes in the public API. (They should have been exposed already, because gjs_context_eval() could already throw GJS_ERROR_FAILED.) Finally, the tests added as part of this patch made the testSystemExit script obsolete.
Created attachment 340098 [details] [review] js: Destroy runtime when last context is destroyed If you call JS_ShutDown() with a runtime still in existence, then SpiderMonkey will shout at you. No kidding, the error message really says "FIX THIS!" This adds refcounting to the per-thread runtime, so that if no GjsContexts are referencing it anymore, then it is destroyed. Note that in later SpiderMonkey versions there is only going to be one JSContext per JSRuntime allowed, and in an as-yet unreleased version the two are to be merged altogether.
Review of attachment 340097 [details] [review]: I like this approach a lot better! I just have a comment on one part of this code that I did not fully grasp. ::: modules/console.cpp @@ +200,3 @@ + &result)) { + /* If this was an uncatchable exception, throw another uncatchable + * on up to the surrounding JS::Evaluate */ Not sure I fully understand under which circumstances this happens now
Review of attachment 340098 [details] [review]: Still looks good.
(In reply to Cosimo Cecchi from comment #76) > Review of attachment 340097 [details] [review] [review]: > > I like this approach a lot better! > I just have a comment on one part of this code that I did not fully grasp. > > ::: modules/console.cpp > @@ +200,3 @@ > + &result)) { > + /* If this was an uncatchable exception, throw another > uncatchable > + * on up to the surrounding JS::Evaluate */ > > Not sure I fully understand under which circumstances this happens now If you run gjs-console, and type imports.system.exit(0); at the prompt, then without this code nothing will happen. It's because main() evaluates the script 'imports.console.interact()', and 'Console.interact()' evaluates what you type in. So the uncatchable exception is stopped when the second JS::Evaluate() returns false, but is swallowed there unless we do this.
(In reply to Philip Chimento from comment #78) > If you run gjs-console, and type imports.system.exit(0); at the prompt, then > without this code nothing will happen. > It's because main() evaluates the script 'imports.console.interact()', and > 'Console.interact()' evaluates what you type in. So the uncatchable > exception is stopped when the second JS::Evaluate() returns false, but is > swallowed there unless we do this. Makes sense, thanks for explaining. Can you put this into the commit message or expand the comment in the code?
Review of attachment 340097 [details] [review]: Feel free to push with my other suggestion.
Attachment 340097 [details] pushed as 3244c9a - js: Call JS_Init() and JS_ShutDown() Attachment 340098 [details] pushed as 6348d8f - js: Destroy runtime when last context is destroyed
OK, added it to the comment. I've discovered one additional thing that needs to be fixed before we merge the branch. If you run any code that uses Lang.Class, you get this warning: Gjs-Message: JS WARNING: [resource:///org/gnome/gjs/modules/lang.js 221]: mutating the [[Prototype]] of an object will cause your code to run very slowly; instead create the object with the correct initial [[Prototype]] value using Object.create This is not trivial to fix, because directly setting __proto__ there is part of our Lang.Class hack.
Created attachment 340215 [details] [review] lang: Interfaces shouldn't clobber inherited props Working on the __proto__ stuff exposed a bug in interfaces: when an object implements an interface, all properties from the interface will be copied over to the object. However, this would hide any properties from the object's prototype chain with the same name, for example from a parent class which also implemented the interface. We had a test which purported to test this exact situation, but there was also a check that assumed if a parent class implemented an interface, then we didn't need to check if the derived class did so correctly too. This fixes both the test and the bug.
Created attachment 340216 [details] [review] WIP - Avoid setting __proto__ directly It was always the case that this made code run slower, apparently, but now since mozjs31 you get a big warning about that. We should not have our Lang.Class framework cause a warning every time it's used! The __proto__ trick was originally necessary because it's impossible to create a function (which you need in order to have a constructor that can be called) with a custom prototype (which you need in order to inherit from Lang.Class.) This replaces the direct __proto__ set, for Class objects, with a Proxy. The Proxy intercepts [[HasProperty]] and [[Get]] operations, and treats them as if the object's prototype were the new prototype that we previously would have set directly. For pure Javascript interfaces we don't need a Proxy, since they don't need constructors. (The only thing that their constructor did before was throw an exception. Now if you try to call them as a constructor, you'll get "SomeInterface is not a constructor" which is just as valid.) STILL TO FIX: - A test failure - "MyClass instanceof Class" doesn't work, it depends on a SpiderMonkey 52 feature, or else we have to build our proxies in C++
Review of attachment 340215 [details] [review]: Looks good, with one small nitpick. ::: modules/lang.js @@ +273,3 @@ Object.getOwnPropertyNames(iface.prototype) .filter((name) => !name.startsWith('__') && name !== 'constructor') + .filter(name => !(name in this.prototype)) Nitpick: all the other functions here have parens around the parameters.
Comment on attachment 340215 [details] [review] lang: Interfaces shouldn't clobber inherited props Attachment 340215 [details] pushed as f1d06ab - lang: Interfaces shouldn't clobber inherited props
Created attachment 340280 [details] [review] WIP - C++ proxy approach, does not work yet
Created attachment 340546 [details] [review] build: Allow compiling without RTTI In order to inherit from C++ classes defined in the SpiderMonkey "JS Friend API" we must match whichever of these flags SpiderMonkey was compiled with. Otherwise, linking will fail, complaining about missing type info for the parent class. I'm not sure of a good way to check programmatically how SpiderMonkey was compiled, so let's just add the same flag here and say that it must match SpiderMonkey. RTTI is turned off by default there, and probably not ever turned on except for debugging upstream. The check is implemented in the same way as in SpiderMonkey (the -GR- flag is for MinGW, although we can be much simpler and don't have to check the platform explicitly, since we have AX_APPEND_COMPILER_FLAG.)
Created attachment 340547 [details] [review] lang: No need to set prototype for Lang.Interface Since we are removing any direct alterations of objects' prototypes, we can remove this one altogether as it turns out that it isn't necessary. The normal way to create an object with a custom prototype is Object.create(), and since it's not necessary for the interface object to be callable, we don't have to do the trick where we give a function a custom prototype. We still need to do the trick for GObject interfaces, because the interface object needs to be the object returned from Gi.register_interface().
Created attachment 340548 [details] [review] js: Workaround for function with custom prototype It's not possible in JS to directly create a function object with a custom prototype. We previously got around this by directly altering the prototype by setting the __proto__ property, but SpiderMonkey now conspicuously warns that this will make your code slow. It would be possible to do this with ES6 Proxy objects, although SpiderMonkey 31 doesn't support the particular getPrototypeOf() proxy trap that we would need in order to implement this correctly --- at least not in JS. Therefore we implement the proxy in C++. We add a debug topic for proxies and a memory counter. All in all, the proxy is probably still slower than a function object with a real prototype would be, but hopefully faster than direct alteration of the prototype. At the very least we can avoid printing a big warning every time our class framework is used.
Review of attachment 340546 [details] [review]: OK
Review of attachment 340547 [details] [review]: Looks good to me.
Review of attachment 340548 [details] [review]: Nice, thanks for the generous comments, really helped understanding how this is supposed to work.
Attachment 340546 [details] pushed as 795e05e - build: Allow compiling without RTTI Attachment 340547 [details] pushed as fdd1325 - lang: No need to set prototype for Lang.Interface Attachment 340548 [details] pushed as 5a88580 - js: Workaround for function with custom prototype
We're done! All that's left is to figure out when to merge the mozjs31 branch into master.
(In reply to Philip Chimento from comment #95) > We're done! All that's left is to figure out when to merge the mozjs31 > branch into master. Hi. Will gnome-shell, gnome-maps, polari, etc... continue working after the merge or will they need some sort of porting? Thank you.
(In reply to Hussam Al-Tayeb from comment #96) > Hi. Will gnome-shell, gnome-maps, polari, etc... continue working after the > merge or will they need some sort of porting? Thanks to Emmanuele's suggestion on how to not have to call gjs_init(), they should not need any explicit porting to continue working. However, there are a few things which will work slightly differently and may need some adjusting, detailed in the NEWS file: - https://git.gnome.org/browse/gjs/tree/NEWS?h=mozjs31#n43 - various ES6 behaviour, if you relied on the previous behaviour, which is unlikely, then you'll need to change your code. - https://git.gnome.org/browse/gjs/tree/NEWS?h=mozjs31#n52 - the way numbers are rounded into integers across some GObject-introspection boundaries changed so it's in line with the JS standard. Again, if you relied on the previous behaviour, which is unlikely, then you'll need to change your code. This was actually already merged to master, so if things still work as of today, then it's probably fine. - https://git.gnome.org/browse/gjs/tree/NEWS?h=mozjs31#n111 - if any JS code executed by gjs_context_eval() or gjs_context_eval_file() could call System.exit(), then you'll need to figure out how you want to handle that in your application. I would expect that most cases would not need any porting, but here would be a good place to take a look at each use of gjs_context_eval[_file]() and double-check that nothing different needs to happen. I encourage you to give the branch a try, and check to make sure that any applications you are interested in do indeed still work. It's added value if someone who isn't me can do that, because more bugs are caught that way.
Ok thank you. I am compiling mozjs31 right now and will give it a try.
I won't compile: http://pastebin.com/raw/nLDeK8PD
I don't have a js-config.h in mozjs31. It looks like the fedora spec file manually installs it. I'll change my package to do so as well.
Sadly gnome-shell (master branch) won't start with mozjs31 based gjs. Reverting to gjs master branch makes it start again. But I'm not sure if this is something from my side. Perhaps I can ask you one day on IRC instead of posting in the bug tracker a lot.
I applied the patches from this Debian package [1] to mozjs31 before compiling. That includes a patch to install js-config.h. That said, I'm pretty sure Fedora must include a mozjs31 RPM somewhere, because it's a dependency of the 0AD game. I'll try to run gnome-shell myself with this branch today, but I might not get around to it until tomorrow. I'm on #javascript, nicknamed "ptomato" if you need help with anything! [1] http://www.ubuntuupdates.org/package/gnome_shell/xenial/main/base/mozjs31
I tested today and gnome-shell runs fine for me with this branch, except it fails an assertion when it exits, because it doesn't clean up the GjsContext properly. I opened bug 775374 for that.
I noticed the gnome-shell patch was committed. I will try the mozjs31 branch again today. Does alt-f2 -> lg -> System.gc(); work for you in gnome-shell with gjs from mozjs31 branch?
So far at least gnome-maps won't start http://pastebin.com/raw/xtijHiX6
System.gc() works fine for me, though I'm reasonably sure it did previously as well. I'll investigate gnome-maps...
I don't have any problems running gnome-maps, built fresh from jhbuild. I have seen a stack smashing error in the past when mixing up debug flags between SpiderMonkey versions or between the SpiderMonkey library and headers. Are you defining DEBUG or JS_DEBUG or something like that?
Hi. I'm not sure. The distribution I use sets CFLAGS="-march=x86-64 -mtune=native -O2 -pipe -fstack-protector-strong" CXXFLAGS="-march=x86-64 -mtune=native -O2 -pipe -fstack-protector-strong" They don't have a mozjs31 package so I made one myself. However, it wasn't compiling so I added CXXFLAGS+=' -fno-delete-null-pointer-checks -fpermissive' CFLAGS+=' -fpermissive' to the package script. js24 was built using CFLAGS="-march=x86-64 -mtune=native -O2 -pipe -fstack-protector-strong" CXXFLAGS="-march=x86-64 -mtune=native -O2 -pipe -fstack-protector-strong" And so was gjs. Would the existence of mozjs24 on the system affect mozjs31 or gjs's build against mozjs31?
The existence of mozjs24 will not affect anything, I've been developing this the whole time with both installed. I would not recommend compiling with `-fno-delete-null-pointer-checks`. If mozjs31 didn't compile out of the box (and indeed it didn't for me, either) did you apply the patches from this Ubuntu package [1] like I suggested? Those fix some compile errors. [1] http://www.ubuntuupdates.org/package/gnome_shell/xenial/main/base/mozjs31
Thank you. With the ubuntu patches and just CFLAGS="-march=x86-64 -mtune=native -O2 -pipe -fstack-protector-strong" CXXFLAGS="-march=x86-64 -mtune=native -O2 -pipe -fstack-protector-strong" everything works. Gnome-shell starts and works fine. So do gnome-maps and gnome-weather. Thank you!
Great, thanks! I'm going to merge this to master then, in preparation for 1.47.3 (tarballs for GNOME 3.23.3 are due Monday.)