Fix infinite loop in service discovery

When there is a circular dependency between disco entities (via their
advertised `disco#items`), Converse went into an infinite loop because
even though there was a check whether an entity already existed, it
failed to add newly created entities to the global
`_converse.disco_entities` collection.

CHANGES.md

@@ -6,6 +6,7 @@
 - Improve how the `muc_domain` setting is populated via service discovery
 - Remove local (non-requesting) contacts not returned from a full roster response
 - Improve performance by looking up VCards via map instead of traversing an array
+- Fix infinite loop when receiving service discovery entities with a circular dependency
 - #2746: Always reply to all iqs, even those not understood
 - #2794: Some display problems with mobile view mode
 - #2868: Selected emoji is inserted into all open chat boxes

src/headless/log.js

@@ -41,7 +41,7 @@ const log = {
      * When using the 'error' or 'warn' loglevels, a full stacktrace will be
      * logged as well.
      * @method log#log
-     * @param { string } message - The message to be logged
+     * @param { string | Error } message - The message to be logged
      * @param { integer } level - The loglevel which allows for filtering of log messages
      */
     log (message, level, style='') {

src/headless/plugins/disco/entity.js

@@ -19,7 +19,7 @@ const { Strophe } = converse.env;
 const DiscoEntity = Model.extend({
     idAttribute: 'jid',
 
-    initialize (_, options) {
+    async initialize (_, options) {
         this.waitUntilFeaturesDiscovered = getOpenPromise();
 
         this.dataforms = new Collection();
@@ -36,15 +36,15 @@ const DiscoEntity = Model.extend({
         this.fields.browserStorage = _converse.createStore(id, 'session');
         this.listenTo(this.fields, 'add', this.onFieldAdded)
 
+        this.items = new _converse.DiscoEntities();
+        id = `converse.disco-items-${this.get('jid')}`;
+        this.items.browserStorage = _converse.createStore(id, 'session');
+        await new Promise(f => this.items.fetch({'success': f, 'error': f}));
+
         this.identities = new Collection();
         id = `converse.identities-${this.get('jid')}`;
         this.identities.browserStorage = _converse.createStore(id, 'session');
         this.fetchFeatures(options);
-
-        this.items = new _converse.DiscoEntities();
-        id = `converse.disco-items-${this.get('jid')}`;
-        this.items.browserStorage = _converse.createStore(id, 'session');
-        this.items.fetch();
     },
 
     /**
@@ -142,12 +142,9 @@ const DiscoEntity = Model.extend({
             }
             const jid = item.getAttribute('jid');
             if (this.items.get(jid) === undefined) {
-                const entity = _converse.disco_entities.get(jid);
-                if (entity) {
-                    this.items.add(entity);
-                } else {
-                    this.items.create({'jid': jid});
-                }
+                const entities = _converse.disco_entities;
+                const entity = entities.get(jid) || entities.create({ jid, name: item.getAttribute('name') });
+                this.items.add(entity);
             }
         });
     },

src/headless/plugins/disco/tests/disco.js

@@ -138,7 +138,6 @@ describe("Service Discovery", function () {
                 .c('item', {
                     'jid': 'words.shakespeare.lit',
                     'name': 'Gateway to Marlowe IM'}).up()
-
                 .c('item', {
                     'jid': 'montague.lit',
                     'node': 'books',
@@ -151,16 +150,28 @@ describe("Service Discovery", function () {
                     'node': 'music',
                     'name': 'Music from the time of Shakespeare'
                 });
+
             _converse.connection._dataRecv(mock.createRequest(stanza));
             await u.waitUntil(() => _converse.disco_entities);
             entities = _converse.disco_entities;
-            expect(entities.length).toBe(2); // We have an extra entity, which is the user's JID
-            expect(entities.get(_converse.domain).items.length).toBe(3);
-            expect(entities.get(_converse.domain).items.pluck('jid').includes('people.shakespeare.lit')).toBeTruthy();
-            expect(entities.get(_converse.domain).items.pluck('jid').includes('plays.shakespeare.lit')).toBeTruthy();
-            expect(entities.get(_converse.domain).items.pluck('jid').includes('words.shakespeare.lit')).toBeTruthy();
-            expect(entities.get(_converse.domain).identities.where({'category': 'conference'}).length).toBe(1);
-            expect(entities.get(_converse.domain).identities.where({'category': 'directory'}).length).toBe(1);
+            expect(entities.length).toBe(5);
+            expect(entities.map(e => e.get('jid'))).toEqual([
+                'montague.lit',
+                'romeo@montague.lit',
+                'people.shakespeare.lit',
+                'plays.shakespeare.lit',
+                'words.shakespeare.lit'
+            ]);
+            let entity = entities.get(_converse.domain);
+            expect(entity.items.length).toBe(3);
+            expect(entity.items.pluck('jid').includes('people.shakespeare.lit')).toBeTruthy();
+            expect(entity.items.pluck('jid').includes('plays.shakespeare.lit')).toBeTruthy();
+            expect(entity.items.pluck('jid').includes('words.shakespeare.lit')).toBeTruthy();
+            expect(entity.identities.where({'category': 'conference'}).length).toBe(1);
+            expect(entity.identities.where({'category': 'directory'}).length).toBe(1);
+
+            entity = entities.get('people.shakespeare.lit');
+            expect(entity.get('name')).toBe('Directory of Characters');
         }));
     });
 

src/headless/plugins/disco/utils.js

@@ -29,7 +29,7 @@ function onDiscoInfoRequest (stanza) {
         }
         iqresult.c('identity', attrs).up();
     });
-    _converse.disco._features.forEach(feature => iqresult.c('feature', {'var': feature}).up());
+    _converse.disco._features.forEach(f => iqresult.c('feature', {'var': f}).up());
     api.send(iqresult.tree());
     return true;
 }
@@ -67,10 +67,10 @@ export async function initializeDisco () {
     _converse.disco_entities = new _converse.DiscoEntities();
     const id = `converse.disco-entities-${_converse.bare_jid}`;
     _converse.disco_entities.browserStorage = _converse.createStore(id, 'session');
+
     const collection = await _converse.disco_entities.fetchEntities();
     if (collection.length === 0 || !collection.get(_converse.domain)) {
-        // If we don't have an entity for our own XMPP server,
-        // create one.
+        // If we don't have an entity for our own XMPP server, create one.
         _converse.disco_entities.create({'jid': _converse.domain});
     }
     /**

src/headless/plugins/smacks/tests/smacks.js

@@ -53,15 +53,16 @@ describe("XEP-0198 Stream Management", function () {
             `<iq id="${IQ_stanzas[2].getAttribute('id')}" type="get" xmlns="jabber:client"><query xmlns="jabber:iq:roster"/></iq>`);
         await mock.waitForRoster(_converse, 'current', 1);
 
-        expect(Strophe.serialize(IQ_stanzas[3])).toBe(
-            `<iq from="romeo@montague.lit/orchard" id="${IQ_stanzas[3].getAttribute('id')}" to="romeo@montague.lit" type="get" xmlns="jabber:client">`+
-                `<query xmlns="http://jabber.org/protocol/disco#info"/></iq>`);
-
-        const omemo_iq = IQ_stanzas[4];
+        const omemo_iq = IQ_stanzas[3];
         expect(Strophe.serialize(omemo_iq)).toBe(
             `<iq from="romeo@montague.lit" id="${omemo_iq.getAttribute('id')}" to="romeo@montague.lit" type="get" xmlns="jabber:client">`+
             `<pubsub xmlns="http://jabber.org/protocol/pubsub"><items node="eu.siacs.conversations.axolotl.devicelist"/></pubsub></iq>`);
 
+        expect(Strophe.serialize(IQ_stanzas[4])).toBe(
+            `<iq from="romeo@montague.lit/orchard" id="${IQ_stanzas[4].getAttribute('id')}" to="romeo@montague.lit" type="get" xmlns="jabber:client">`+
+                `<query xmlns="http://jabber.org/protocol/disco#info"/></iq>`);
+
+
         await u.waitUntil(() => sent_stanzas.filter(s => (s.nodeName === 'presence')).length);
 
         expect(sent_stanzas.filter(s => (s.nodeName === 'r')).length).toBe(3);
@@ -98,14 +99,8 @@ describe("XEP-0198 Stream Management", function () {
         _converse.connection._dataRecv(mock.createRequest(ack));
         expect(_converse.session.get('unacked_stanzas').length).toBe(3);
 
-        expect(_converse.session.get('unacked_stanzas')[0]).toBe(
-            `<iq from="romeo@montague.lit/orchard" id="${IQ_stanzas[3].getAttribute('id')}" to="romeo@montague.lit" type="get" xmlns="jabber:client">`+
-                `<query xmlns="http://jabber.org/protocol/disco#info"/></iq>`);
-
-        expect(_converse.session.get('unacked_stanzas')[1]).toBe(
-            `<iq from="romeo@montague.lit" id="${omemo_iq.getAttribute('id')}" to="romeo@montague.lit" type="get" xmlns="jabber:client">`+
-                `<pubsub xmlns="http://jabber.org/protocol/pubsub"><items node="eu.siacs.conversations.axolotl.devicelist"/></pubsub></iq>`);
-
+        expect(_converse.session.get('unacked_stanzas')[0]).toBe(Strophe.serialize(IQ_stanzas[3]));
+        expect(_converse.session.get('unacked_stanzas')[1]).toBe(Strophe.serialize(IQ_stanzas[4]));
         expect(_converse.session.get('unacked_stanzas')[2]).toBe(
             `<presence xmlns="jabber:client"><priority>0</priority>`+
                 `<c hash="sha-1" node="https://conversejs.org" ver="TfHz9vOOfqIG0Z9lW5CuPaWGnrQ=" xmlns="http://jabber.org/protocol/caps"/>`+
@@ -144,13 +139,13 @@ describe("XEP-0198 Stream Management", function () {
 
         iq = IQ_stanzas.pop();
         expect(Strophe.serialize(iq)).toBe(
-            `<iq from="romeo@montague.lit" id="${iq.getAttribute('id')}" to="romeo@montague.lit" type="get" xmlns="jabber:client">`+
-            `<pubsub xmlns="http://jabber.org/protocol/pubsub"><items node="eu.siacs.conversations.axolotl.devicelist"/></pubsub></iq>`);
+            `<iq from="romeo@montague.lit/orchard" id="${iq.getAttribute('id')}" to="romeo@montague.lit" type="get" xmlns="jabber:client">`+
+                `<query xmlns="http://jabber.org/protocol/disco#info"/></iq>`);
 
         iq = IQ_stanzas.pop();
         expect(Strophe.serialize(iq)).toBe(
-            `<iq from="romeo@montague.lit/orchard" id="${iq.getAttribute('id')}" to="romeo@montague.lit" type="get" xmlns="jabber:client">`+
-                `<query xmlns="http://jabber.org/protocol/disco#info"/></iq>`);
+            `<iq from="romeo@montague.lit" id="${iq.getAttribute('id')}" to="romeo@montague.lit" type="get" xmlns="jabber:client">`+
+            `<pubsub xmlns="http://jabber.org/protocol/pubsub"><items node="eu.siacs.conversations.axolotl.devicelist"/></pubsub></iq>`);
 
         expect(IQ_stanzas.filter(iq => sizzle('query[xmlns="jabber:iq:roster"]', iq).pop()).length).toBe(0);
     }));

src/plugins/chatview/tests/http-file-upload.js

@@ -44,8 +44,7 @@ describe("XEP-0363: HTTP File Upload", function () {
 
             let entities = await _converse.api.disco.entities.get();
             expect(entities.length).toBe(2);
-            expect(entities.pluck('jid').includes('montague.lit')).toBe(true);
-            expect(entities.pluck('jid').includes('romeo@montague.lit')).toBe(true);
+            expect(entities.pluck('jid')).toEqual(['montague.lit', 'romeo@montague.lit']);
 
             expect(entities.get(_converse.domain).features.length).toBe(2);
             expect(entities.get(_converse.domain).identities.length).toBe(1);
@@ -79,7 +78,8 @@ describe("XEP-0363: HTTP File Upload", function () {
             _converse.connection._dataRecv(mock.createRequest(stanza));
 
             _converse.api.disco.entities.get().then(entities => {
-                expect(entities.length).toBe(2);
+                expect(entities.length).toBe(3);
+                expect(entities.pluck('jid')).toEqual(['montague.lit', 'romeo@montague.lit', 'upload.montague.lit']);
                 expect(entities.get('montague.lit').items.length).toBe(1);
                 // Converse.js sees that the entity has a disco#info feature, so it will make a query for it.
                 const selector = 'iq[to="upload.montague.lit"] query[xmlns="http://jabber.org/protocol/disco#info"]';
@@ -342,7 +342,8 @@ describe("XEP-0363: HTTP File Upload", function () {
 
                     entities = await _converse.api.disco.entities.get()
 
-                    expect(entities.length).toBe(2);
+                    expect(entities.length).toBe(3);
+                    expect(entities.pluck('jid')).toEqual(['montague.lit', 'romeo@montague.lit', 'upload.montague.lit']);
                     expect(entities.get('montague.lit').items.length).toBe(1);
                     await u.waitUntil(function () {
                         // Converse.js sees that the entity has a disco#info feature,

src/plugins/muc-views/tests/muc-api.js

@@ -106,7 +106,6 @@ describe("Groupchats", function () {
             spyOn(_converse.ChatRoom.prototype, 'getDiscoInfo').and.callFake(() => Promise.resolve());
 
             let jid = 'lounge@montague.lit';
-            let chatroomview, IQ_id;
             await mock.openControlBox(_converse);
             await mock.waitForRoster(_converse, 'current');
             const rosterview = document.querySelector('converse-roster');
@@ -115,43 +114,39 @@ describe("Groupchats", function () {
             let room = await _converse.api.rooms.open(jid);
             // Test on groupchat that's not yet open
             expect(room instanceof Model).toBeTruthy();
-            chatroomview = await u.waitUntil(() => _converse.chatboxviews.get(jid));
-            expect(chatroomview.is_chatroom).toBeTruthy();
-            await u.waitUntil(() => u.isVisible(chatroomview));
+            let mucview = await u.waitUntil(() => _converse.chatboxviews.get(jid));
+            expect(mucview.is_chatroom).toBeTruthy();
+            await u.waitUntil(() => u.isVisible(mucview));
 
             // Test again, now that the room exists.
             room = await _converse.api.rooms.open(jid);
             expect(room instanceof Model).toBeTruthy();
-            chatroomview = await u.waitUntil(() => _converse.chatboxviews.get(jid));
-            expect(chatroomview.is_chatroom).toBeTruthy();
-            expect(u.isVisible(chatroomview)).toBeTruthy();
-            await chatroomview.close();
+            mucview = await u.waitUntil(() => _converse.chatboxviews.get(jid));
+            expect(mucview.is_chatroom).toBeTruthy();
+            expect(u.isVisible(mucview)).toBeTruthy();
+            await mucview.close();
 
             // Test with mixed case in JID
             jid = 'Leisure@montague.lit';
             room = await _converse.api.rooms.open(jid);
             expect(room instanceof Model).toBeTruthy();
-            chatroomview = await u.waitUntil(() => _converse.chatboxviews.get(jid.toLowerCase()));
-            await u.waitUntil(() => u.isVisible(chatroomview));
+            mucview = await u.waitUntil(() => _converse.chatboxviews.get(jid.toLowerCase()));
+            await u.waitUntil(() => u.isVisible(mucview));
 
             jid = 'leisure@montague.lit';
             room = await _converse.api.rooms.open(jid);
             expect(room instanceof Model).toBeTruthy();
-            chatroomview = await u.waitUntil(() => _converse.chatboxviews.get(jid.toLowerCase()));
-            await u.waitUntil(() => u.isVisible(chatroomview));
+            mucview = await u.waitUntil(() => _converse.chatboxviews.get(jid.toLowerCase()));
+            await u.waitUntil(() => u.isVisible(mucview));
 
             jid = 'leiSure@montague.lit';
             room = await _converse.api.rooms.open(jid);
             expect(room instanceof Model).toBeTruthy();
-            chatroomview = await u.waitUntil(() => _converse.chatboxviews.get(jid.toLowerCase()));
-            await u.waitUntil(() => u.isVisible(chatroomview));
-            chatroomview.close();
+            mucview = await u.waitUntil(() => _converse.chatboxviews.get(jid.toLowerCase()));
+            await u.waitUntil(() => u.isVisible(mucview));
+            mucview.close();
 
             api.settings.set('muc_instant_rooms', false);
-            const sendIQ = _converse.connection.sendIQ;
-            spyOn(_converse.connection, 'sendIQ').and.callFake(function (iq, callback, errback) {
-                IQ_id = sendIQ.bind(this)(iq, callback, errback);
-            });
             // Test with configuration
             room = await _converse.api.rooms.open('room@conference.example.org', {
                 'nick': 'some1',
@@ -168,10 +163,14 @@ describe("Groupchats", function () {
             });
             expect(room instanceof Model).toBeTruthy();
 
+            const IQ_stanzas = _converse.connection.IQ_stanzas;
+            const selector = `iq[to="room@conference.example.org"] query[xmlns="http://jabber.org/protocol/disco#info"]`;
+            const features_query = await u.waitUntil(() => IQ_stanzas.filter(iq => iq.querySelector(selector)).pop());
+
             // We pretend this is a new room, so no disco info is returned.
             const features_stanza = $iq({
                     from: 'room@conference.example.org',
-                    'id': IQ_id,
+                    'id': features_query.getAttribute('id'),
                     'to': 'romeo@montague.lit/desktop',
                     'type': 'error'
                 }).c('error', {'type': 'cancel'})
@@ -199,9 +198,7 @@ describe("Groupchats", function () {
                 .c('status', {code:'110'}).up()
                 .c('status', {code:'201'});
             _converse.connection._dataRecv(mock.createRequest(presence));
-            expect(_converse.connection.sendIQ).toHaveBeenCalled();
 
-            const IQ_stanzas = _converse.connection.IQ_stanzas;
             const iq = await u.waitUntil(() => IQ_stanzas.filter(s => s.querySelector(`query[xmlns="${Strophe.NS.MUC_OWNER}"]`)).pop());
             expect(Strophe.serialize(iq)).toBe(
                 `<iq id="${iq.getAttribute('id')}" to="room@conference.example.org" type="get" xmlns="jabber:client">`+
@@ -245,10 +242,10 @@ describe("Groupchats", function () {
                 </query>
                 </iq>`);
 
-            chatroomview = _converse.chatboxviews.get('room@conference.example.org');
-            spyOn(chatroomview.model, 'sendConfiguration').and.callThrough();
+            mucview = _converse.chatboxviews.get('room@conference.example.org');
+            spyOn(mucview.model, 'sendConfiguration').and.callThrough();
             _converse.connection._dataRecv(mock.createRequest(node));
-            await u.waitUntil(() => chatroomview.model.sendConfiguration.calls.count() === 1);
+            await u.waitUntil(() => mucview.model.sendConfiguration.calls.count() === 1);
 
             const sent_stanza = IQ_stanzas.filter(s => s.getAttribute('type') === 'set').pop();
             expect(sizzle('field[var="muc#roomconfig_roomname"] value', sent_stanza).pop().textContent.trim()).toBe('Room');