Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

weakBind bug - only half gets called #29

Closed
comanche opened this issue Oct 15, 2013 · 3 comments
Closed

weakBind bug - only half gets called #29

comanche opened this issue Oct 15, 2013 · 3 comments
Labels

Comments

@comanche
Copy link

When listeners are added with weakBind, only half of them gets called. The problem seems to be with the following code starting on line 173 to 189:

for(var j = 0; j < listeners.length; j += 1) {
  switch(arguments.length) {
    case 1:
      _cancelBubble = listeners[j].call(this);
      break;
    case 2:
      _cancelBubble = listeners[j].call(this, arguments[1]);
      break;
    case 3:
      _cancelBubble = listeners[j].call(this, arguments[1], arguments[2]);
      break;
    default:
      var args = arguments.slice ? arguments.slice(1) : Array.prototype.slice.call(arguments, 1);
      _cancelBubble = listeners[j].apply(this, args);
  }
  if(_cancelBubble === false) { this.cancelBubble = false; }
}

As listener handlers are called, they are removed. listeners.length decreases at the same time as the counter j is incremented. This causes only half of the listeners to get called.

This issue does not affect listeners added via bind. However, if the listener removes itself with unbind when called, the same issue arises. While this loop is running, if anything affects the size of listeners, there will be issue.

@RobertWHurst
Copy link
Owner

Thanks for pointing that out. I'll get a fix in.

@comanche
Copy link
Author

Possibly the quickest fix is to check if the listener was removed and offset the counter.

for(var j = 0; j < listeners.length; j += 1) {
  var listener = listeners[j];
  switch(arguments.length) {
    case 1:
      _cancelBubble = listener.call(this);
      break;
    case 2:
      _cancelBubble = listener.call(this, arguments[1]);
      break;
    case 3:
      _cancelBubble = listener.call(this, arguments[1], arguments[2]);
      break;
    default:
      var args = arguments.slice ? arguments.slice(1) : Array.prototype.slice.call(arguments, 1);
      _cancelBubble = listener.apply(this, args);
  }
  if(_cancelBubble === false) { this.cancelBubble = false; }
  // If the listener was removed from the list of listeners after call,
  // offset the counter to prevent increment.
  if (listener !== listeners[j]) { j--; }
}

RobertWHurst added a commit that referenced this issue Sep 26, 2014
Breaking Changes:
- Browser namespace is now lucidJS instead of LucidJS.

Changes:
- Fix #36 - Listeners not invoked because of `weakBind`
- Fix #35 - Why isn't flag actually invoking listeners?
- Fix #29 - weakBind bug - only half gets called
- Fix #21 - "set" fires "once" listener twice
- Remove build script and makefile and use browerify CLI
- Add missing package.json fields
RobertWHurst added a commit that referenced this issue Sep 26, 2014
Breaking Changes:
- Browser namespace is now lucidJS instead of LucidJS.

Changes:
- Fix #36 - Listeners not invoked because of `weakBind`
- Fix #35 - Why isn't flag actually invoking listeners?
- Fix #29 - weakBind bug - only half gets called
- Fix #21 - "set" fires "once" listener twice
- Remove build script and makefile and use browerify CLI
- Add missing package.json fields
@RobertWHurst
Copy link
Owner

This should now be fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants