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 785424 - Patch: backport of changes required for use with mozjs-55
Patch: backport of changes required for use with mozjs-55
Status: RESOLVED FIXED
Product: gjs
Classification: Bindings
Component: general
1.49.x
Other Linux
: Normal normal
: ---
Assigned To: gjs-maint
gjs-maint
Depends on:
Blocks:
 
 
Reported: 2017-07-26 03:01 UTC by luke.nukem.jones@gmail.com
Modified: 2017-07-31 14:22 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Backport of patch required for mozjs-55 (32.58 KB, patch)
2017-07-26 03:01 UTC, luke.nukem.jones@gmail.com
none Details | Review
Backport of patch required for JSObject compare to nullptr (23.41 KB, patch)
2017-07-28 05:10 UTC, luke.nukem.jones@gmail.com
none Details | Review
Backport of patch required for vectors MOZ_MUST_USE returns (9.84 KB, patch)
2017-07-28 05:11 UTC, luke.nukem.jones@gmail.com
none Details | Review
Backport of patch required for JSObject compare to nullptr (24.85 KB, patch)
2017-07-30 08:54 UTC, luke.nukem.jones@gmail.com
committed Details | Review
Backport of patch required for vectors MOZ_MUST_USE returns (9.82 KB, patch)
2017-07-30 08:54 UTC, luke.nukem.jones@gmail.com
none Details | Review
Backport of patch required for vectors MOZ_MUST_USE returns (9.74 KB, patch)
2017-07-30 21:10 UTC, luke.nukem.jones@gmail.com
committed Details | Review

Description luke.nukem.jones@gmail.com 2017-07-26 03:01:16 UTC
Created attachment 356392 [details] [review]
Backport of patch required for mozjs-55

git log
--------------------------------------------------
mozjs52+: Backport of changes required for mozjs55
    
In mozjs-55 there are a few changes internally that require changes to the way things are handled by GJS: vector resizing operations that return a value now have MOZ_MUST_USE, and Rooted<T> + Handle<T> have changes which require comparing to nullptr instead of NULL.
    
The changes to GJS for the above are safe for use in the mozjs-52 version of GJS and have been backported.
--------------------------------------------------
Comment 1 Philip Chimento 2017-07-27 00:30:28 UTC
Review of attachment 356392 [details] [review]:

Hi Luke, I already looked at this patch in general when you showed it to me the other day, so +1. But would you mind splitting it up into two patches as mentioned on IRC? One for the nullptr changes, and one for the MOZ_MUST_USE changes. (Use `git reset HEAD^` to "uncommit" it while keeping the changes, and `git add -p` on each file to selectively add changes.)
Comment 2 luke.nukem.jones@gmail.com 2017-07-28 05:10:38 UTC
Created attachment 356495 [details] [review]
Backport of patch required for JSObject compare to nullptr
Comment 3 luke.nukem.jones@gmail.com 2017-07-28 05:11:34 UTC
Created attachment 356496 [details] [review]
Backport of patch required for vectors MOZ_MUST_USE returns
Comment 4 luke.nukem.jones@gmail.com 2017-07-28 05:12:33 UTC
Hi Philip,

Very sorry, I had forgotten about that (and learned something new about git). Hopefully the patches are now what you were asking for.
Comment 5 Philip Chimento 2017-07-29 16:20:52 UTC
Review of attachment 356495 [details] [review]:

I realized a couple of things while reviewing that would make this patch better.

1) Instead of comparing against nullptr explicitly, let's use `if (thing)` and `if (!thing)` so that we can just take advantage of convert-to-bool operators. I have been doing this already in most of the code that I've been writing and it's probably good to make it an official style preference.

2) I realized that the cause for why you had to add the `.get()` was probably that GjsMaybeOwned<T>'s operator== takes a T* pointer. I think if you add an overload for JS::Handle<T> then the errors will just go away and you won't have to add `.get()`. i.e.

    inline bool operator==(JS::Handle<T> other) { return *this == other.get(); }

If both of these suggestions still work on your mozjs55 branch, then I think that's what I'd prefer to merge.

::: gjs/jsapi-class.h
@@ +308,3 @@
     }                                                                         \
     object = JS_NewObjectForConstructor(context, &gjs_##name##_class, argv);  \
+    if (object == nullptr)                                                       \

Make sure the \'s still line up as much as possible if you are modifying the line.

::: gjs/jsapi-util.cpp
@@ +316,3 @@
         gjs_build_string_array(context, array_length, (char **) array_values));
 
+    if (array == nullptr)

Most of the changes in this file aren't needed for the JS::Rooted / JS::Handle change.
Comment 6 Philip Chimento 2017-07-29 16:22:44 UTC
Review of attachment 356496 [details] [review]:

Just a couple of comments

::: gi/arg.cpp
@@ +592,3 @@
+        if (!elems.growBy(1)) {
+            g_error("out of memory in gjs_array_from_strv");
+            return false;

No need for `return false` after `g_error()`: it aborts the program. (You can remove the curly braces from all these blocks if there's only one line in the body of the if statement.)

@@ +2471,3 @@
             arg.v_##type = array[i]; \
+            if (!elems.growBy(1)) { \
+                gjs_##throw(context, "out of memory in gjs_array_from_g_list"); \

g_error
Comment 7 luke.nukem.jones@gmail.com 2017-07-30 05:07:26 UTC
::: gi/arg.cpp
@@ +592,3 @@
+        if (!elems.growBy(1)) {
+            g_error("out of memory in gjs_array_from_strv");
+            return false;

I must have missed that one when I converted from gjs_throw. I'll get cracking on your other suggestions too
Comment 8 luke.nukem.jones@gmail.com 2017-07-30 08:53:31 UTC
With
inline bool operator!=(const T& other)
and
inline bool operator!=(JS::Handle<T> other)
(plus the ==)
I get ambiguous operators, so I've left that out.

The two patches have been run through the cheese grater and cleaned up a bit with your suggestions.
Comment 9 luke.nukem.jones@gmail.com 2017-07-30 08:54:11 UTC
Created attachment 356585 [details] [review]
Backport of patch required for JSObject compare to nullptr
Comment 10 luke.nukem.jones@gmail.com 2017-07-30 08:54:40 UTC
Created attachment 356586 [details] [review]
Backport of patch required for vectors MOZ_MUST_USE returns
Comment 11 Philip Chimento 2017-07-30 15:40:04 UTC
Review of attachment 356585 [details] [review]:

A couple of stray whitespace changes, but I will fix it and push anyway. Thanks.
Comment 12 Philip Chimento 2017-07-30 15:46:35 UTC
Review of attachment 356586 [details] [review]:

I think you might have missed the comment about the curly braces. All the one-line bodies of the if statements don't need braces.

In addition there's one thing I forgot to comment on last time: the error messages will already include the information about the file and line in the system log, so no need to repeat the function name and expression in each error message. "Out of memory" or "Unable to append to vector" is probably enough. (And the location information will become stale if code is moved around.)

::: gi/value.cpp
@@ +223,2 @@
     JS::AutoValueVector argv(context);
+    if (!argv.reserve(n_param_values)) {

Let's keep the comment here, I think it's still informative. (It refers to sometimes needing less space than is reserved here.)

::: gjs/importer.cpp
@@ +397,3 @@
+        if (!prop_ids.append(ids[ix])) {
+            g_error("prop_ids.append(), out of memory in load_module_elements");
+            return;

This return isn't needed.

::: gjs/jsapi-util.cpp
@@ +846,3 @@
+    if (!scope_chain.append(eval_obj)) {
+        g_error("gjs_eval_with_scope(), out of memory, unable to append to vector ");
+        return false;

This return isn't needed.
Comment 13 Philip Chimento 2017-07-30 15:48:28 UTC
Comment on attachment 356585 [details] [review]
Backport of patch required for JSObject compare to nullptr

Attachment 356585 [details] pushed as c1bc509 - Backport of patch required for JSObject compare to nullptr
Comment 14 luke.nukem.jones@gmail.com 2017-07-30 21:10:53 UTC
Created attachment 356614 [details] [review]
Backport of patch required for vectors MOZ_MUST_USE returns

Much cleaned up, properly caught many issues, used suggestions.
Comment 15 Philip Chimento 2017-07-31 13:57:37 UTC
Review of attachment 356614 [details] [review]:

Looks great. I had a few nitpicks but I'll just fix them as I push it. Thanks!

::: gi/arg.cpp
@@ +2463,3 @@
             arg.v_##type = array[i]; \
+            if (!elems.growBy(1)) \
+                g_##error("Unable to grow vector"); \

No need for ## here - it's not an escape for _, it's an obscure corner of the preprocessor. To find out more, read https://en.wikipedia.org/wiki/C_preprocessor#Token_concatenation

::: gjs/jsapi-util.cpp
@@ +843,3 @@
     JS::AutoObjectVector scope_chain(context);
+    if (!scope_chain.append(eval_obj))
+        g_error("Unable to append to vector ");

Delete extra space
Comment 16 Philip Chimento 2017-07-31 14:22:43 UTC
Attachment 356614 [details] pushed as e6cc09d - Backport of patch required for vectors MOZ_MUST_USE returns