From 17dfa3d7ba1dc9a5a8ec6a8f33851d71e8bcc121 Mon Sep 17 00:00:00 2001 From: JC Brand Date: Thu, 24 Oct 2019 17:55:20 +0200 Subject: [PATCH] Avoid race-condition that destroys vcards VCards were being created before `fetch` was completed, so once fetch was done those VCards were unset from their collection. Add a new event and promise `VCardsInitialized` that triggers after successful fetching and wait for it before creating VCards. --- spec/chatbox.js | 24 +++++------ spec/protocol.js | 2 +- spec/roster.js | 13 +++--- src/converse-chatview.js | 1 + src/converse-profile.js | 2 +- src/converse-rosterview.js | 10 +++-- src/headless/converse-chatboxes.js | 19 ++------- src/headless/converse-muc.js | 19 ++++++--- src/headless/converse-roster.js | 68 +++++++++++++++--------------- src/headless/converse-vcard.js | 64 +++++++++++++++++++--------- 10 files changed, 118 insertions(+), 104 deletions(-) diff --git a/spec/chatbox.js b/spec/chatbox.js index 7f4a4cfc9..7d449e953 100644 --- a/spec/chatbox.js +++ b/spec/chatbox.js @@ -722,10 +722,8 @@ var view = _converse.chatboxviews.get(sender_jid); expect(view).toBeDefined(); - await u.waitUntil(() => view.model.vcard.get('fullname') === mock.cur_names[1]) - // Check that the notification appears inside the chatbox in the DOM - let events = view.el.querySelectorAll('.chat-state-notification'); - expect(events[0].textContent).toEqual(mock.cur_names[1] + ' is typing'); + const event = await u.waitUntil(() => view.el.querySelector('.chat-state-notification')); + expect(event.textContent).toEqual(mock.cur_names[1] + ' is typing'); // Check that it doesn't appear twice msg = $msg({ @@ -735,7 +733,7 @@ id: (new Date()).getTime() }).c('composing', {'xmlns': Strophe.NS.CHATSTATES}).tree(); await _converse.chatboxes.onMessage(msg); - events = view.el.querySelectorAll('.chat-state-notification'); + const events = view.el.querySelectorAll('.chat-state-notification'); expect(events.length).toBe(1); expect(events[0].textContent).toEqual(mock.cur_names[1] + ' is typing'); done(); @@ -778,7 +776,8 @@ expect(msg_obj.get('sender')).toEqual('me'); expect(msg_obj.get('is_delayed')).toEqual(false); const chat_content = chatboxview.el.querySelector('.chat-content'); - const status_text = chat_content.querySelector('.chat-info.chat-state-notification').textContent; + const el = await u.waitUntil(() => chat_content.querySelector('.chat-info.chat-state-notification')); + const status_text = el.textContent; expect(status_text).toBe('Typing from another device'); done(); })); @@ -863,7 +862,7 @@ await _converse.chatboxes.onMessage(msg); expect(_converse.api.trigger).toHaveBeenCalledWith('message', jasmine.any(Object)); await u.waitUntil(() => view.model.vcard.get('fullname') === mock.cur_names[1]) - var event = view.el.querySelector('.chat-info.chat-state-notification'); + const event = await u.waitUntil(() => view.el.querySelector('.chat-state-notification')); expect(event.textContent).toEqual(mock.cur_names[1] + ' has stopped typing'); done(); })); @@ -904,9 +903,8 @@ const msg_obj = chatbox.messages.models[0]; expect(msg_obj.get('sender')).toEqual('me'); expect(msg_obj.get('is_delayed')).toEqual(false); - const chat_content = chatboxview.el.querySelector('.chat-content'); - const status_text = chat_content.querySelector('.chat-info.chat-state-notification').textContent; - expect(status_text).toBe('Stopped typing on the other device'); + const el = await u.waitUntil(() => chatboxview.el.querySelector('.chat-info.chat-state-notification')); + expect(el.textContent).toBe('Stopped typing on the other device'); done(); })); }); @@ -1045,7 +1043,7 @@ .c('composing', {'xmlns': Strophe.NS.CHATSTATES}).up() .tree(); await _converse.chatboxes.onMessage(msg); - await u.waitUntil(() => view.model.messages.length); + await u.waitUntil(() => view.el.querySelector('.chat-state-notification')); expect(view.el.querySelectorAll('.chat-state-notification').length).toBe(1); msg = $msg({ from: sender_jid, @@ -1056,7 +1054,7 @@ await _converse.chatboxes.onMessage(msg); await u.waitUntil(() => (view.model.messages.length > 1)); expect(_converse.api.trigger).toHaveBeenCalledWith('message', jasmine.any(Object)); - expect(view.el.querySelectorAll('.chat-state-notification').length).toBe(0); + await u.waitUntil(() => view.el.querySelectorAll('.chat-state-notification').length === 0); done(); })); }); @@ -1084,7 +1082,7 @@ expect(_converse.api.trigger).toHaveBeenCalledWith('message', jasmine.any(Object)); const view = _converse.chatboxviews.get(sender_jid); await u.waitUntil(() => view.model.vcard.get('fullname') === mock.cur_names[1]); - const event = view.el.querySelector('.chat-state-notification'); + const event = await u.waitUntil(() => view.el.querySelector('.chat-state-notification')); expect(event.textContent).toEqual(mock.cur_names[1] + ' has gone away'); done(); })); diff --git a/spec/protocol.js b/spec/protocol.js index 752759db6..c64addd52 100644 --- a/spec/protocol.js +++ b/spec/protocol.js @@ -165,7 +165,7 @@ // A contact should now have been created expect(_converse.roster.get('contact@example.org') instanceof _converse.RosterContact).toBeTruthy(); expect(contact.get('jid')).toBe('contact@example.org'); - expect(_converse.api.vcard.get).toHaveBeenCalled(); + await u.waitUntil(() => contact.initialized); /* To subscribe to the contact's presence information, * the user's client MUST send a presence stanza of diff --git a/spec/roster.js b/spec/roster.js index bd9efb25d..c6b9c7071 100644 --- a/spec/roster.js +++ b/spec/roster.js @@ -601,7 +601,7 @@ it("do not have a header if there aren't any", mock.initConverse( - ['rosterGroupsFetched'], {}, + ['rosterGroupsFetched', 'VCardsInitialized'], {}, async function (done, _converse) { await test_utils.openControlBox(_converse); @@ -613,16 +613,15 @@ ask: 'subscribe', fullname: name }); - spyOn(window, 'confirm').and.returnValue(true); await u.waitUntil(() => { const el = _converse.rosterview.get('Pending contacts').el; return u.isVisible(el) && _.filter(el.querySelectorAll('li'), li => u.isVisible(li)).length; }, 700) - spyOn(_converse.connection, 'sendIQ').and.callThrough(); - sizzle(`.remove-xmpp-contact[title="Click to remove ${name} as a contact"]`, _converse.rosterview.el).pop().click(); + const remove_el = await u.waitUntil(() => sizzle(`.remove-xmpp-contact[title="Click to remove ${name} as a contact"]`, _converse.rosterview.el).pop()); + spyOn(window, 'confirm').and.returnValue(true); + remove_el.click(); expect(window.confirm).toHaveBeenCalled(); - expect(_converse.connection.sendIQ).toHaveBeenCalled(); const iq = _converse.connection.IQ_stanzas.pop(); expect(Strophe.serialize(iq)).toBe( @@ -739,7 +738,7 @@ ask: null, fullname: name }); - return u.waitUntil(() => contact.vcard.get('fullname')); + return u.waitUntil(() => contact.initialized); })); await u.waitUntil(() => sizzle('li', _converse.rosterview.el).length, 600); // Check that they are sorted alphabetically @@ -1041,7 +1040,7 @@ requesting: true, nickname: name }); - return u.waitUntil(() => contact.vcard.get('fullname')); + return u.waitUntil(() => contact.initialized); })); await u.waitUntil(() => _converse.rosterview.get('Contact requests').el.querySelectorAll('li').length, 700); expect(_converse.rosterview.update).toHaveBeenCalled(); diff --git a/src/converse-chatview.js b/src/converse-chatview.js index 2371fa8dd..16faf4e90 100644 --- a/src/converse-chatview.js +++ b/src/converse-chatview.js @@ -701,6 +701,7 @@ converse.plugins.add('converse-chatview', { * @param { _converse.Message } message - The message object */ async showMessage (message) { + await message.initialized; const view = this.add(message.get('id'), new _converse.MessageView({'model': message})); await view.render(); // Clear chat state notifications diff --git a/src/converse-profile.js b/src/converse-profile.js index 7fc1e8f8c..c770cc38a 100644 --- a/src/converse-profile.js +++ b/src/converse-profile.js @@ -310,7 +310,7 @@ converse.plugins.add('converse-profile', { /******************** Event Handlers ********************/ _converse.api.listen.on('controlBoxPaneInitialized', async view => { - await _converse.api.waitUntil('statusInitialized'); + await _converse.api.waitUntil('VCardsInitialized'); _converse.xmppstatusview = new _converse.XMPPStatusView({'model': _converse.xmppstatus}); view.el.insertAdjacentElement('afterBegin', _converse.xmppstatusview.render().el); }); diff --git a/src/converse-rosterview.js b/src/converse-rosterview.js index 8c66a7edc..78b194cb1 100644 --- a/src/converse-rosterview.js +++ b/src/converse-rosterview.js @@ -336,7 +336,8 @@ converse.plugins.add('converse-rosterview', { "click .remove-xmpp-contact": "removeContact" }, - initialize () { + async initialize () { + await this.model.initialized; this.listenTo(this.model, "change", this.render); this.listenTo(this.model, "highlight", this.highlight); this.listenTo(this.model, "destroy", this.remove); @@ -345,6 +346,7 @@ converse.plugins.add('converse-rosterview', { this.listenTo(this.model.presence, "change:show", this.render); this.listenTo(this.model.vcard, 'change:fullname', this.render); + this.render(); }, render () { @@ -978,7 +980,7 @@ converse.plugins.add('converse-rosterview', { }); - function initRoster () { + function initRosterView () { /* Create an instance of RosterView once the RosterGroups * collection has been created (in @converse/headless/converse-core.js) */ @@ -996,8 +998,8 @@ converse.plugins.add('converse-rosterview', { */ _converse.api.trigger('rosterViewInitialized'); } - _converse.api.listen.on('rosterInitialized', initRoster); - _converse.api.listen.on('rosterReadyAfterReconnection', initRoster); + _converse.api.listen.on('rosterInitialized', initRosterView); + _converse.api.listen.on('rosterReadyAfterReconnection', initRosterView); _converse.api.listen.on('afterTearDown', () => { if (converse.rosterview) { diff --git a/src/headless/converse-chatboxes.js b/src/headless/converse-chatboxes.js index e0398e7d4..5dcd35ffb 100644 --- a/src/headless/converse-chatboxes.js +++ b/src/headless/converse-chatboxes.js @@ -123,11 +123,11 @@ converse.plugins.add('converse-chatboxes', { }; }, - initialize () { + async initialize () { + this.initialized = u.getResolveablePromise(); ModelWithContact.prototype.initialize.apply(this, arguments); if (this.get('type') === 'chat') { - this.setVCard(); this.setRosterContact(Strophe.getBareJidFromJid(this.get('from'))); } @@ -137,6 +137,8 @@ converse.plugins.add('converse-chatboxes', { if (this.isEphemeral()) { window.setTimeout(this.safeDestroy.bind(this), 10000); } + await _converse.api.trigger('messageInitialized', this, {'Synchronous': true}); + this.initialized.resolve(); }, safeDestroy () { @@ -147,19 +149,6 @@ converse.plugins.add('converse-chatboxes', { } }, - setVCard () { - if (!_converse.vcards) { - // VCards aren't supported - return; - } - if (this.get('type') === 'error') { - return; - } else { - const jid = Strophe.getBareJidFromJid(this.get('from')); - this.vcard = _converse.vcards.findWhere({'jid': jid}) || _converse.vcards.create({'jid': jid}); - } - }, - isOnlyChatStateNotification () { return u.isOnlyChatStateNotification(this); }, diff --git a/src/headless/converse-muc.js b/src/headless/converse-muc.js index 5412a1e67..626b16fd2 100644 --- a/src/headless/converse-muc.js +++ b/src/headless/converse-muc.js @@ -340,10 +340,10 @@ converse.plugins.add('converse-muc', { } }, - setVCard () { + async setVCard () { + await _converse.api.waitUntil('VCardsInitialized'); if (!_converse.vcards) { - // VCards aren't supported - return; + return; // VCards aren't supported } if (['error', 'info'].includes(this.get('type'))) { return; @@ -403,10 +403,7 @@ converse.plugins.add('converse-muc', { }, async initialize() { - if (_converse.vcards) { - this.vcard = _converse.vcards.findWhere({'jid': this.get('jid')}) || - _converse.vcards.create({'jid': this.get('jid')}); - } + this.setVCard(); this.set('box_id', `box-${btoa(this.get('jid'))}`); this.initFeatures(); // sendChatState depends on this.features @@ -421,6 +418,14 @@ converse.plugins.add('converse-muc', { this.enterRoom(); }, + async setVCard () { + await _converse.api.waitUntil('VCardsInitialized'); + if (_converse.vcards) { + this.vcard = _converse.vcards.findWhere({'jid': this.get('jid')}) || + _converse.vcards.create({'jid': this.get('jid')}); + } + }, + async enterRoom () { const conn_status = this.get('connection_status'); _converse.log( diff --git a/src/headless/converse-roster.js b/src/headless/converse-roster.js index 07bfc1fff..6f13a1fca 100644 --- a/src/headless/converse-roster.js +++ b/src/headless/converse-roster.js @@ -74,39 +74,6 @@ converse.plugins.add('converse-roster', { }; - /** - * Initialize the Bakcbone collections that represent the contats - * roster and the roster groups. - * @private - * @method _converse.initRoster - */ - _converse.initRoster = function () { - const storage = _converse.config.get('storage'); - _converse.roster = new _converse.RosterContacts(); - let id = `converse.contacts-${_converse.bare_jid}`; - _converse.roster.browserStorage = _converse.createStore(id, storage); - - _converse.roster.data = new Backbone.Model(); - id = `converse-roster-model-${_converse.bare_jid}`; - _converse.roster.data.id = id; - _converse.roster.data.browserStorage = _converse.createStore(id, storage); - _converse.roster.data.fetch(); - - id = `converse.roster.groups${_converse.bare_jid}`; - _converse.rostergroups = new _converse.RosterGroups(); - _converse.rostergroups.browserStorage = _converse.createStore(id, storage); - /** - * Triggered once the `_converse.RosterContacts` and `_converse.RosterGroups` have - * been created, but not yet populated with data. - * This event is useful when you want to create views for these collections. - * @event _converse#chatBoxMaximized - * @example _converse.api.listen.on('rosterInitialized', () => { ... }); - * @example _converse.api.waitUntil('rosterInitialized').then(() => { ... }); - */ - _converse.api.trigger('rosterInitialized'); - }; - - _converse.sendInitialPresence = function () { if (_converse.send_initial_presence) { _converse.xmppstatus.sendPresence(); @@ -243,6 +210,7 @@ converse.plugins.add('converse-roster', { }, async initialize (attributes) { + this.initialized = u.getResolveablePromise(); this.setPresence(); const { jid } = attributes; const bare_jid = Strophe.getBareJidFromJid(jid).toLowerCase(); @@ -268,6 +236,7 @@ converse.plugins.add('converse-roster', { * @param { _converse.RosterContact } contact */ await _converse.api.trigger('rosterContactInitialized', this, {'Synchronous': true}); + this.initialized.resolve(); }, setPresence () { @@ -995,7 +964,36 @@ converse.plugins.add('converse-roster', { }); - _converse.api.listen.on('presencesInitialized', (reconnecting) => { + async function initRoster () { + // Initialize the Bakcbone collections that represent the contats + // roster and the roster groups. + await _converse.api.waitUntil('VCardsInitialized'); + const storage = _converse.config.get('storage'); + _converse.roster = new _converse.RosterContacts(); + let id = `converse.contacts-${_converse.bare_jid}`; + _converse.roster.browserStorage = _converse.createStore(id, storage); + + _converse.roster.data = new Backbone.Model(); + id = `converse-roster-model-${_converse.bare_jid}`; + _converse.roster.data.id = id; + _converse.roster.data.browserStorage = _converse.createStore(id, storage); + _converse.roster.data.fetch(); + + id = `converse.roster.groups${_converse.bare_jid}`; + _converse.rostergroups = new _converse.RosterGroups(); + _converse.rostergroups.browserStorage = _converse.createStore(id, storage); + /** + * Triggered once the `_converse.RosterContacts` and `_converse.RosterGroups` have + * been created, but not yet populated with data. + * This event is useful when you want to create views for these collections. + * @event _converse#chatBoxMaximized + * @example _converse.api.listen.on('rosterInitialized', () => { ... }); + * @example _converse.api.waitUntil('rosterInitialized').then(() => { ... }); + */ + _converse.api.trigger('rosterInitialized'); + } + + _converse.api.listen.on('presencesInitialized', async (reconnecting) => { if (reconnecting) { /** * Similar to `rosterInitialized`, but instead pertaining to reconnection. @@ -1006,7 +1004,7 @@ converse.plugins.add('converse-roster', { */ _converse.api.trigger('rosterReadyAfterReconnection'); } else { - _converse.initRoster(); + await initRoster(); } _converse.roster.onConnected(); _converse.registerPresenceHandler(); diff --git a/src/headless/converse-vcard.js b/src/headless/converse-vcard.js index 57eee44f7..641c2f301 100644 --- a/src/headless/converse-vcard.js +++ b/src/headless/converse-vcard.js @@ -65,6 +65,8 @@ converse.plugins.add('converse-vcard', { */ const { _converse } = this; + _converse.api.promises.add('VCardsInitialized'); + _converse.VCard = Backbone.Model.extend({ defaults: { @@ -101,7 +103,9 @@ converse.plugins.add('converse-vcard', { model: _converse.VCard, initialize () { - this.on('add', vcard => _converse.api.vcard.update(vcard)); + this.on('add', vcard => { + _converse.api.vcard.update(vcard); + }); } }); @@ -157,26 +161,36 @@ converse.plugins.add('converse-vcard', { } /************************ BEGIN Event Handlers ************************/ - _converse.initVCardCollection = function () { + _converse.initVCardCollection = async function () { _converse.vcards = new _converse.VCards(); - const id = `${_converse.bare_jid}-converse.vcards`; - _converse.vcards.browserStorage = _converse.createStore(id, _converse.config.get('storage')); - _converse.vcards.fetch(); - } - - _converse.api.listen.on('statusInitialized', () => { - _converse.initVCardCollection(); + _converse.vcards.browserStorage = _converse.createStore(`${_converse.bare_jid}-converse.vcards`); + await new Promise(resolve => { + _converse.vcards.fetch({ + 'success': resolve, + 'error': resolve + }, {'silent': true}); + }); const vcards = _converse.vcards; if (_converse.session) { const jid = _converse.session.get('bare_jid'); _converse.xmppstatus.vcard = vcards.findWhere({'jid': jid}) || vcards.create({'jid': jid}); } - }); + /** + * Triggered as soon as the `_converse.vcards` collection has been initialized and populated from cache. + * @event _converse#VCardsInitialized + */ + _converse.api.trigger('VCardsInitialized'); + } + + _converse.api.listen.on('statusInitialized', _converse.initVCardCollection); _converse.api.listen.on('clearSession', () => { - if (_converse.shouldClearCache() && _converse.vcards) { - _converse.vcards.clearSession(); - delete _converse.vcards; + if (_converse.shouldClearCache()) { + _converse.api.promises.add('VCardsInitialized'); + if (_converse.vcards) { + _converse.vcards.clearSession(); + delete _converse.vcards; + } } }); @@ -184,16 +198,24 @@ converse.plugins.add('converse-vcard', { _converse.api.listen.on('addClientFeatures', () => _converse.api.disco.own.features.add(Strophe.NS.VCARD)); - function setVCardOnModel (model) { - // TODO: if we can make this method async and wait for the VCard to - // be updated, then we'll avoid unnecessary re-rendering of roster contacts. - const jid = model.get('jid'); + async function setVCardOnModel (model) { + let jid; + if (model instanceof _converse.Message) { + if (model.get('type') === 'error') { + return; + } + jid = model.get('from'); + } else { + jid = model.get('jid'); + } + await _converse.api.waitUntil('VCardsInitialized'); model.vcard = _converse.vcards.findWhere({'jid': jid}); if (!model.vcard) { model.vcard = _converse.vcards.create({'jid': jid}); } } - _converse.api.listen.on('rosterContactInitialized', contact => setVCardOnModel(contact)); + _converse.api.listen.on('rosterContactInitialized', m => setVCardOnModel(m)); + _converse.api.listen.on('messageInitialized', m => setVCardOnModel(m)); /************************ BEGIN API ************************/ @@ -286,9 +308,9 @@ converse.plugins.add('converse-vcard', { * }); */ async update (model, force) { - const vcard = await this.get(model, force); - delete vcard['stanza'] - model.save(vcard); + const data = await this.get(model, force); + delete data['stanza'] + model.save(data); } } });