Subtle bug in jquery-function

[boliveira]

Hi!

I found a very subtle bug in jquery-function that impacts the functioning of this library with signalr client. It's about this line of code:

const events = subject.events || {};

You are caching the events instance and using that reference to call the bind, unbind and triggerHandler functions. That is a problem because signalr caches this reference sometimes but other times it doesn't and calls to bind/unbind will make the cached copies have a stale events reference, so when it calls triggerHandler on a cached reference it will not find the bound callbacks because the bind was called on yet another jquery-function object.

I was having a problem caused by this. The scenario was when I received messages before the connection was moved to the "connected" state. When this happens, the signalr client buffers the messages and when it transitions to the connected state, it will drain this buffer and trigger onReceived event handlers.

But signalr was calling the triggerhandler on a cached jquery-function object, and the bind methods to onReceived on another, which causes the method I bound was not being called!

I fixed this by reading the events reference from subject in each function body call declared (unbind, bind and triggerHandler).

Do you understand what I am saying? If not, I can try to explain a little better :)

Can you provide an official fix, so that I can I have a permanent fix to this problem? Many thanks!

[olofd]

Nice find! And very good reporting. I do undertand the issue.

Do you have any code that fixes the problem? Anything you can post here? Otherwise you could maybe post a reproduce senario or something?

Olof

[boliveira]

The way I solved it was to move the line const events = subject.events || {}; to the inside of every emulated jquery function (unbind, bind, triggerhandler) like this:

`unbind(event, handler) { const events = subject.events || {}; let handlers = events[event] || [];

  if (handler) {
    const idx = handlers.indexOf(handler);
    if (idx !== -1) {
      handlers.splice(idx, 1);
    }
  } else {
    handlers = [];
  }

  events[event] = handlers;
  subject.events = events;
},
bind(event, handler) {
  const events = subject.events || {};
  const current = events[event] || [];
  events[event] = current.concat(handler)
  subject.events = events;
},
triggerHandler(event, args) {
  const events = subject.events || {};
  const handlers = events[event] || [];
  handlers.forEach(fn => {
    if (args === undefined) {
      args = { type: event };
    }
    if (!Array.isArray(args)) {
      args = [args];
    }
    if (args && args[0] && args[0].type === undefined) {
      args = [{
        type: event
      }].concat(args || []);
    } else {
      args = args || [];
    }

    fn.apply(this, args);
  });
}`

After these changes, everything worked just fine.

[olofd]

Ah! Great, than I understood you correctly. I did a change here last night before pushing the new version. I did not do any difference for basic functionality, so I included what I thought might work. Can you take a look here:

https://github.com/olofd/react-native-signalr/blob/master/src/jquery-function.js#L16

And test out the new version on npm that the problem is solved?

[boliveira]

Hi again :)

Unfortunately with the new version I have another error when connecting to SignalR: "method GET must not have a request body"

I traced it to this line in ajax.js

request.send(options.data && qs(options.data));

It seems options.data is an empty object and it's always true, so it will try to send an empty body, even when there is no body to be sent.

If I set options.data = null before this call, it works correctly.

Can you check this out? Thanks for the work!