After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 785040 - Misc 1.49 and mozjs52 enhancements
Misc 1.49 and mozjs52 enhancements
Status: RESOLVED FIXED
Product: gjs
Classification: Bindings
Component: general
1.49.x
Other All
: Normal enhancement
: ---
Assigned To: gjs-maint
gjs-maint
Depends on: 784196
Blocks:
 
 
Reported: 2017-07-17 21:16 UTC by Philip Chimento
Modified: 2017-07-18 15:36 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
build: Use 'always inline' macro in more places (5.37 KB, patch)
2017-07-17 21:16 UTC, Philip Chimento
committed Details | Review
js: Use correct autoptr in gjs_string_to_filename() (25.95 KB, patch)
2017-07-17 21:16 UTC, Philip Chimento
committed Details | Review
function: Better message about call during GC (3.01 KB, patch)
2017-07-17 21:16 UTC, Philip Chimento
committed Details | Review

Description Philip Chimento 2017-07-17 21:16:13 UTC
A few minor fixes found while testing existing code with the new branch:

- Better debug info for the "Attempting to call back into JSAPI during GC" message
- Use correct autoptr in gjs_string_to_filename(), which cascades into a bunch of other places
- Adjust ALWAYS_INLINE macro to avoid warnings on GCC 4.9
Comment 1 Philip Chimento 2017-07-17 21:16:49 UTC
Created attachment 355780 [details] [review]
build: Use 'always inline' macro in more places

On GCC 4.9, the inliner heuristic starts to complain again, because
SpiderMonkey 52 includes more 'always inline' functions, so when we add
ours in there, the inliner decides that functions are growing too much in
size due to inlined code.

Add a GJS_ALWAYS_INLINE macro just like Mozilla does in their code, and
use it for our templated code and other boilerplate.
Comment 2 Philip Chimento 2017-07-17 21:16:52 UTC
Created attachment 355781 [details] [review]
js: Use correct autoptr in gjs_string_to_filename()

The return value of g_filename_from_utf8() (must be freed with g_free())
was getting assigned to a GjsAutoJSChar (which would be freed with
JS_free()). Instead gjs_string_to_filename() must have a GjsAutoChar
output parameter.

This requires changing gjs_parse_call_args() as well. We improve it by
allowing to pass in autoptrs for 's' or 'F' args instead of char**.

Fixes a few minor memory leaks, mostly in error paths in Cairo functions.
Comment 3 Philip Chimento 2017-07-17 21:16:56 UTC
Created attachment 355782 [details] [review]
function: Better message about call during GC

GJS can attempt to call back into JS during garbage collection if you
implement vfunc_remove() on your container class. (Destroying a container
will remove all its child widgets, and if you have implemented
vfunc_remove() in JS then that will cause a JS call.)

Note this possibility in the warning message, and also add whatever
information we can get about the offending callback without calling into
JSAPI.
Comment 4 Cosimo Cecchi 2017-07-18 07:23:05 UTC
Review of attachment 355780 [details] [review]:

OK
Comment 5 Cosimo Cecchi 2017-07-18 07:28:40 UTC
Review of attachment 355781 [details] [review]:

Looks good

::: modules/cairo-pdf-surface.cpp
@@ +58,2 @@
         return false;
+

Remove empty line
Comment 6 Cosimo Cecchi 2017-07-18 07:29:29 UTC
Review of attachment 355782 [details] [review]:

OK
Comment 7 Philip Chimento 2017-07-18 15:36:15 UTC
Attachment 355780 [details] pushed as c1b352a - build: Use 'always inline' macro in more places
Attachment 355781 [details] fixed and pushed as 9ddca8b - js: Use correct autoptr in gjs_string_to_filename()
Attachment 355782 [details] pushed as 62a2b79 - function: Better message about call during GC