Remove triggering Destroy: triggers Remove. Bug causes second Remove to remove the wrong thing. #3923

sberney · 2016-01-11T17:55:51Z

See comment below, this bug can be recreated without using {silent: true} at all.

fiddle without {silent: true}
original fiddle with {silent: true}

I recently upgraded from Backbone 1.1.2 to Backbone 1.2.3. With my project as it is, when I remove a single item from a collection, that item and the last item (erroneously) in the collection are removed. The reason for this has to do with two things:

In the relevant code, remove is typically mapped to destroy. The reasoning is that we don't want uncollected detritus floating about in case someone accidentally uses remove on the collection.
In the newer backbone version, destroy no longer behaves as it did before as regards the silent: true option.

Destroy triggers a remove event that can no longer be silenced, which is conceptually fine as we have indeed already removed the model from the collection. However as occurs in #3847 , collection.get(model) succeeds where collection.indexOf(model) returns -1. The logic leading to the faulty removal occurs at line 1103, and just below line 1106 contains a splice that given -1 as an index removes the last element in the collection. this.get(model), which is tested to prevent attempts to remove already removed items, delegates to _byId which does not contain the model itself as a key, but does contain the model's id and cid when it shouldn't.

Any one of the following changes will fix this bug:

destroy changed to not trigger a 'remove' event when given the { silent: true } option
_removeModels modified to not splice out the last list element when indexOf(model) == -1
removed model's cid and id removed from _byId before second remove call occurs

Please let me know if there is a simple fix for my code which maintains the functionality of destroying an object when a remove event occurs.

The following code will reproduce this bug (or just use the fiddle):

<html>
  <body>
    <div id="collection"></div>
    <div id="buttons"></div>
  </body>
</html>

define([
  'backbone'
],
function (Backbone) {
  var Collection = Backbone.Collection.extend({
    endsInDestruction: function (model, collection, options) {
      // the silent option will apply to remove events, preventing an infinite loop here.
      // You can remove it to observe the infinite loop.
      model.destroy({ silent: true });
    },
    initialize: function (options) {
      this.listenTo(this, 'remove', this.endsInDestruction);
      this.listenTo(this, 'add change remove', this.render);
    },

    el: $('#collection'),
    render: function () {
      $(this.el).html('<ul></ul>');
      var list = $('ul', this.el);
      this.forEach(function(model) {
        list.append('<li>' + model.get('name') + '</li>');
      });
    }
  });
  var collection = new Collection();
  collection.render();

  // add two models, keeping a reference to one of them.
  collection.add({
    name: 'foobardi'
  });
  var model = collection.add({
    name: 'caesar'
  });

  // This erroneously removes two items from the collection
  function removeIt () {
    collection.remove(model);
    console.log('done');
  }

  // binds a button to remove an item at your leisure.
  $('#buttons').html('<button type="button" id="main-button">Remove One Model</button>');
  $('#main-button').click(removeIt);
});

The text was updated successfully, but these errors were encountered:

akre54 · 2016-01-12T21:15:24Z

We usually say that passing {silent: true} is an anti-pattern.

The reasoning is that we don't want uncollected detritus floating about in case someone accidentally uses remove on the collection.

Can you explain this more? Our preferred way is for you to call model.destroy() if you want to get rid of the model completely, or collection.remove(model) when you just want it out of the collection. Garbage collection and taking care with your event binding will handle the rest.

sberney · 2016-01-12T22:52:49Z

This isn't an issue with silent true. This is an issue with the cleanup of the ._byId hash. Here is a fork of the previous fiddle with two lines modified: it does not use {silent: true} anywhere.

I have an existing project that sometimes, after hearing a remove event on model, calls model.destroy(). After remove and destroy both execute, the _byId hash still contains references to the allegedly removed and destroyed model, via the keys id and cid, but not via the model itself as a key. So, after the model has supposedly been removed, the second, unstoppable remove event that occurs after model destruction makes an inadequate test of the model's membership in the collection. Membership tests positive because id and cid still map to the model (they presumably shouldn't, as remove has been called once already), while the model itself is not a key in _byId because it has been removed by the previous remove call. The second remove event continues execution because of this positive membership test. It removes the last item in the collection, which it shouldn't do.

For your curiosity, a little bit of extra information about my project:

My project extends backbone to provide some asynchronous control flow. Sometimes it uses {silent: true} to wait for promises to resolve before triggering events. As this is used as the backbone for a corporate data-driven website, it also needs to coordinate multiple collections which involve the same data (data views). Some of the subsequent validation + synchronization behavior has been implemented with {silent: true} as well, although that might be more easily changed. Anyway, it doesn't matter if {silent: true} is used at all.

ivarni · 2016-01-15T14:35:23Z

For what it's worth, this test:

  QUnit.test('deleting', function(assert) {
    var Collection = Backbone.Collection.extend({
      initialize: function() {
        this.listenTo(this, 'remove', this.destroyModel);
      },
      destroyModel: function(model, collection, options) {
        if (!options.stopLoop) {
          model.destroy({stopLoop: true});
        }
      }
    });
    var collection = new Collection();
    collection.add({a: 1});
    collection.add({a: 2});
    collection.remove(collection.at(1));
    assert.equal(collection.length, 1);
  });

Fails on the 1.2.3 tag but passes on master.

akre54 · 2016-01-15T15:21:59Z

Right, again why are you calling destroy from the remove event? This is backwards from the intended design.

We don't actually remove the reference until after the remove event is fired.

sberney · 2016-01-15T17:46:42Z

@ivarni Thanks! If a new version rolled out without this problem sometime soon, that would be fantastic.
@akre54 Because remove is part of the public API, we would like the public to be presented with a consistent state. At the point of time when the second remove event is being handled, the model itself isn't a key in the collection, but its id and cid are; which is inconsistent. I think it would be more intuitive and robust to remove all three keys at once. Why not call _removeReference before triggering remove? That would fix the problem.

There's some complicated business logic; database sync assurance, abstractions to support moving away from strict REST if necessary, and other collections which should be tied to the state of the collection in question. That kind of thing. It is necessary to be able to call model.destroy() when I determine a model should be destroyed.

jridgewell · 2016-01-15T17:54:54Z

Dupe of #3693? If so, this'll be fixed in v1.2.4.

sberney · 2016-01-15T18:34:51Z

Possibly? I don't know. When is v1.2.4 scheduled for?

On line 1146, a non-optimistic model.remove() call might fix this issue. For example, testing

if (event === 'destroy' && this.indexOf(model) > -1) this.remove(model, options);;

Update:

@jridgewell On a more careful look (I've visited this issue before, as it happens), #3693 appears to me to be related, and fixing its underlying issue will likely fix this. ivarni pointed out a test for this is passing on master. So while I hope that this issue has been fixed by the work on 3693, it's important to me to have an estimate for when the next release will occur, to gauge whether a work-around will be necessary.

sberney · 2016-04-26T23:18:03Z

Fixed in version 1.3.1 and 1.3.3. Thanks!

sberney changed the title ~~Unsupported destroy({ silent: true }) causes bug during destruction bound to remove event.~~ Remove triggering Destroy: triggers Remove. Bug causes second Remove to remove the wrong thing. Jan 14, 2016

sberney closed this as completed Apr 26, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove triggering Destroy: triggers Remove. Bug causes second Remove to remove the wrong thing. #3923

Remove triggering Destroy: triggers Remove. Bug causes second Remove to remove the wrong thing. #3923

sberney commented Jan 11, 2016

akre54 commented Jan 12, 2016

sberney commented Jan 12, 2016

ivarni commented Jan 15, 2016

akre54 commented Jan 15, 2016

sberney commented Jan 15, 2016

jridgewell commented Jan 15, 2016

sberney commented Jan 15, 2016

sberney commented Apr 26, 2016

Remove triggering Destroy: triggers Remove. Bug causes second Remove to remove the wrong thing. #3923

Remove triggering Destroy: triggers Remove. Bug causes second Remove to remove the wrong thing. #3923

Comments

sberney commented Jan 11, 2016

akre54 commented Jan 12, 2016

sberney commented Jan 12, 2016

ivarni commented Jan 15, 2016

akre54 commented Jan 15, 2016

sberney commented Jan 15, 2016

jridgewell commented Jan 15, 2016

sberney commented Jan 15, 2016

Update:

sberney commented Apr 26, 2016