Gst.Message.type accessor gives random values instead of Gst.MessageType values

[sdroege]

If you run the code at the end you get lines like the following printed:

Running
Got message 139904264699968
Got message 139904264699968
Got message 139904264699968
Got message 139904264699968
Got message 139904264708096
[...]

These should instead be values from the Gst.MessageType enum, which are all powers of two.

    const gi = require('node-gtk');
    const GLib = gi.require('GLib', '2.0');
    const Gst = gi.require('Gst', '1.0');

    gi.startLoop();
    Gst.init();

    let loop = GLib.MainLoop.new(null, false);

    let pipeline = Gst.parseLaunch("videotestsrc ! autovideosink");

    let bus = pipeline.getBus();

    bus.addWatch(0, (bus, msg) => {
        switch (msg.type) {
            case Gst.MessageType.EOS:
                console.log("Got EOS");
                loop.quit();
                break;
            case Gst.MessageType.ERROR:
                let res = msg.parseError();
                console.log("Got error " + res);
                loop.quit();
                break;
            default:
                console.log("Got message " + msg.type)
                break;
        }

        return true;
    });

    if (pipeline.setState(Gst.State.PLAYING) == Gst.StateChangeReturn.Failure) {
        console.error("Failed to change pipeline state");
        return;
    }

    console.log("Running");
    loop.run();
    console.log("Exiting");

[wotzlaff]

This used to be a problem but it should be fixed in the current version. I cannot confirm the problem.

(Actually, I tried to rewrite some of the classical GStreamer examples here and most of them are working at least to some degree.)

[sdroege]

Hmm, I have a package.json that lists

{
  "dependencies": {
    "node-gtk": "file:../node-gtk"
  }
}

and at that place is git master of node-gtk (and I did npm install and npm link in there).

[wotzlaff]

Please try to npm run build it.

[romgrk]

I tried the example above with current master and I observed the same results described. I haven't looked at the issue yet but I'm thinking it's probably that we're assuming the wrong size for the enum, which makes sense here:

> n=139904264699968; n.toString(2)
'11111110011111000000000000000000000000001000000'

It's probably the 32-bit value 64 (0b00000000000000000000000001000000) but we think it's a 64-bit value so we read the garbage at the start of the storage.

[sdroege]

GLib enums/flags are always gint/guint or otherwise various code inside GLib/GValue would fall apart.

[sdroege]

Also I can confirm, working with msg.type >> 32 works around that problem but then runs into #222 as you noticed and would also not work on big endian architectures

[romgrk]

Yes. So this probably needs to be replaced: https://github.com/romgrk/node-gtk/blob/master/src/type.cc#L140-L144

[sdroege]

Interesting, what does that return there currently? GI_TYPE_TAG_INT64 / GI_TYPE_TAG_UINT64? That seems like a problem elsewhere (libgirepository) then and not on your side.

It should return GI_TYPE_TAG_UINT32, I guess. The highest value in the enum is 4294967295, which does not fit a (signed) INT32.

[romgrk]

Not sure, it doesn't seem to be hit at at first glance, which is strange. I don't have time to investigate more right now but I'll dig into it in the next days.

[wotzlaff]

The values are coming from here: https://github.com/romgrk/node-gtk/blob/master/src/value.cc#L134-L139 These are some lines of code which I could never really confirm. Maybe someone who is more involved in GIR (@sdroege !?) could say what's wrong there.

Edit: Maybe I could add that we are coming from here.

[sdroege]

I don't know the libgirepository API, sorry. I've never really worked on dynamic bindings, only on those that generate bindings code from the XML .gir files (Vala, C# and Rust).

[wotzlaff]

That's too bad :sweat_smile:

Probably a quick fix would be to use arg->v_int32 and arg->v_uint32 in the lines above. But I cannot test it because I do not observe the problem.

[sdroege]

That doesn't change anything and that code also does not seem to run when accessing the msg.type field (it runs many times earlier though).

[sdroege]

Also gint and gint32 are the same type, so it can't really change anything :)

[wotzlaff]

Now this is really strange. Because this is exactly where I get the values of MessageType from. Which system are you running?

(And of course the idea to use gint32 is stupid...)

[sdroege]

This is Linux (Debian unstable with nodejs v12.19.0) on x86-64.

[wotzlaff]

I still don't get it. I started from a debian:unstable docker container and build the master using npm run build:full (after installing some dependencies). Everything seems to work well:

Running
Got message 64
Got message 64
Got message 64
Got message 64
Got message 8192
Got message 64
Got message 8192
Got message 268435456
Got message 32768
Got message 32768
Got message 64
[...]

Edit: Of course, Tag: error is still there at the end...

[sdroege]

Are you running a 32 bit version of Debian in docker by any chance?

And I assume I would see things printed out on every access when adding a printf or similar to https://github.com/romgrk/node-gtk/blob/master/src/value.cc#L134-L139 ? I only see those things printed before, but not when handling each message. So I assume there is other code that is responsible for this?

[romgrk]

So I've inspected a bit:

• I'm unable to reproduce the issue :/ Tried node 12 but I'm on Arch, I'll try to reproduce in a Debian container
• Yes, that code is responsible for converting. I can also confirm that on my machine it hits the GI_INFO_TYPE_FLAGS case at every access of .type.
• You could apply the following patch to find out if it's really hitting other branches in that swtich statement.

diff --git a/lib/index.js b/lib/index.js
index 3ce95ae..dc8d054 100644
--- a/lib/index.js
+++ b/lib/index.js
@@ -171,13 +171,14 @@ function addVirtualFunction(object, info, implementor) {
     })
 }

-function fieldGetter(fieldInfo) {
+function fieldGetter(fieldInfo, name) {
     return function() {
+        console.log('accessing', name)
         return internal.StructFieldGetter(this, fieldInfo);
     };
 }

-function fieldSetter(fieldInfo) {
+function fieldSetter(fieldInfo, name) {
     return function(value) {
         return internal.StructFieldSetter(this, fieldInfo, value);
     };
@@ -193,8 +194,8 @@ function addField(object, fieldInfo) {
     Object.defineProperty(object, name, {
         configurable: true,
         enumerable: readable,
-        get: readable ? fieldGetter(fieldInfo) : undefined,
-        set: writable ? fieldSetter(fieldInfo) : undefined
+        get: readable ? fieldGetter(fieldInfo, name) : undefined,
+        set: writable ? fieldSetter(fieldInfo, name) : undefined
     })
 }

diff --git a/src/value.cc b/src/value.cc
index 345fb81..0b35a9e 100644
--- a/src/value.cc
+++ b/src/value.cc
@@ -40,6 +40,8 @@ static bool IsUint8Array (GITypeInfo *type_info);
 Local<Value> GIArgumentToV8(GITypeInfo *type_info, GIArgument *arg, long length, bool mustCopy) {
     GITypeTag type_tag = g_type_info_get_tag (type_info);

+    printf(" -> tag: %s\n", g_type_tag_to_string(type_tag));
+
     switch (type_tag) {
     case GI_TYPE_TAG_VOID:
         return Nan::Undefined ();
@@ -115,6 +117,8 @@ Local<Value> GIArgumentToV8(GITypeInfo *type_info, GIArgument *arg, long length,
             GIInfoType interface_type = g_base_info_get_type (interface_info);
             Local<Value> value;

+            printf(" -> interface: %s\n", g_info_type_to_string(interface_type));
+
             switch (interface_type) {
             /** from documentation:
              * GIObjectInfo represents a GObject. This doesn't represent a specific instance

[wotzlaff]

For your reference, I compiled a Dockerfile to setup debian:unstable, build node-gtk and run the example here. If I understand correctly, it is not 32 bit.

[sdroege]

Yes, that code is responsible for converting. I can also confirm that on my machine it hits the GI_INFO_TYPE_FLAGS case at every access of .type.

Ah that was stupid. I added the printfs to the enum case, not the flags case. I'll report back tomorrow with more details

[sdroege]

Not sure what changed, but I can't reproduce this anymore with the latest version of node-gtk as of today.

[romgrk]

For the record, here is the log for the last days:

git log

9074ccf  (15 hours ago)  (HEAD -> pr-237) Clean up GStreamer example - Sebastian Dröge
f95f3c0  (34 hours ago)  (origin/master, origin/HEAD, master) Merge pull request #230 from wotzlaff/fix-using-boolean - Rom Grk
4a7abe7  (2 days ago)  Merge pull request #227 from wotzlaff/fix-error-test - Rom Grk
bfebfea  (2 days ago)  Remove using v8::Boolean - Nico Strasdat
1381418  (2 days ago)  Rewrite error test again - Nico Strasdat
2b17d7f  (2 days ago)  Rewrite error test - Nico Strasdat
7153e49  (2 days ago)  Fix handling of GQuarks - Nico Strasdat
651da6e  (2 days ago)  [publish binary][skip tests] - Rom Grk
21e536b  (2 days ago)  build: remove nodejs 11, add nodejs 15 - Rom Grk
7d54013  (2 days ago)  [publish binary][skip tests] - Rom Grk
6291c41  (2 days ago)  (tag: v0.5.0) 0.5.0 - Rom Grk
cb319d4  (2 days ago)  doc: update changelog - Rom Grk
f54bd5b  (2 days ago)  Merge pull request #225 from wotzlaff/handle-gerrors - Rom Grk
cfa2c91  (2 days ago)  (pr-225) Remove WrapperFromBoxed - Nico Strasdat
cafec4f  (2 days ago)  Remove console.log - Nico Strasdat
236b341  (2 days ago)  Implement boxed-like handling - Nico Strasdat
5200c8a  (3 days ago)  Add test for GError - Nico Strasdat
61b7aa6  (3 days ago)  Copy GError - Nico Strasdat
9230c47  (3 days ago)  Handle GError return type - Nico Strasdat
ce909e4  (4 days ago)  (fix-signal-after, fix-gobject-initialization) Merge pull request #202 from romgrk/fix-memory-leaks - Rom Grk

I would have liked to understand the issue though :/

[sdroege]

Yeah it's not one of those commits, I tried bisecting that already. It doesn't happen anymore with the same version where it happened before, and I was definitely running that version back then because the printfs I added were output. This is a bit concerning :)

[romgrk]

Yes. I'll re-open and give it one more check, if I don't find anything we'll close until someone else hits this.