From 34231168a753cec6c0657ac38eab0b294ce92bec Mon Sep 17 00:00:00 2001 From: Scott Nonnenberg Date: Tue, 24 Jul 2018 11:55:24 -0700 Subject: [PATCH] On message delete, ensure that all external files are deleted --- js/background.js | 2 +- js/models/conversations.js | 1 - js/models/messages.js | 51 ++++------------------------------ js/modules/signal.js | 8 ++++-- js/modules/types/attachment.js | 22 ++++++++------- js/modules/types/message.js | 46 ++++++++++++++++++++++++++++++ test/models/messages_test.js | 22 +++------------ 7 files changed, 73 insertions(+), 79 deletions(-) diff --git a/js/background.js b/js/background.js index eaff4e251..72966ea42 100644 --- a/js/background.js +++ b/js/background.js @@ -880,7 +880,7 @@ messageDescriptor.id, messageDescriptor.type ); - await conversation.save({ profileSharing: true }); + await wrapDeferred(conversation.save({ profileSharing: true })); return confirm(); } diff --git a/js/models/conversations.js b/js/models/conversations.js index 1d3aea838..c3e57e6e6 100644 --- a/js/models/conversations.js +++ b/js/models/conversations.js @@ -1769,7 +1769,6 @@ Whisper.Notifications.add({ conversationId, iconUrl, - imageUrl: message.getImageUrl(), isExpiringMessage, message: message.getNotificationText(), messageId, diff --git a/js/models/messages.js b/js/models/messages.js index 31f89dcb4..312a8db15 100644 --- a/js/models/messages.js +++ b/js/models/messages.js @@ -20,8 +20,7 @@ const { Message: TypedMessage, Contact, PhoneNumber } = Signal.Types; const { - // loadAttachmentData, - deleteAttachmentData, + deleteExternalMessageFiles, getAbsoluteAttachmentPath, } = Signal.Migrations; @@ -69,7 +68,6 @@ this.OUR_NUMBER = textsecure.storage.user.getNumber(); - this.on('change:attachments', this.updateImageUrl); this.on('destroy', this.onDestroy); this.on('change:expirationStartTimestamp', this.setToExpire); this.on('change:expireTimer', this.setToExpire); @@ -223,54 +221,15 @@ return ''; }, - async onDestroy() { - this.revokeImageUrl(); - const attachments = this.get('attachments'); - await Promise.all(attachments.map(deleteAttachmentData)); - }, - updateImageUrl() { - this.revokeImageUrl(); - const attachment = this.get('attachments')[0]; - if (attachment) { - const blob = new Blob([attachment.data], { - type: attachment.contentType, - }); - this.imageUrl = URL.createObjectURL(blob); - } else { - this.imageUrl = null; - } + onDestroy() { + this.unload(); + + return deleteExternalMessageFiles(this.attributes); }, unload() { - if (this.quoteThumbnail) { - URL.revokeObjectURL(this.quoteThumbnail.objectUrl); - this.quoteThumbnail = null; - } if (this.quotedMessage) { this.quotedMessage = null; } - const quote = this.get('quote'); - const attachments = (quote && quote.attachments) || []; - attachments.forEach(attachment => { - if (attachment.thumbnail && attachment.thumbnail.objectUrl) { - URL.revokeObjectURL(attachment.thumbnail.objectUrl); - // eslint-disable-next-line no-param-reassign - attachment.thumbnail.objectUrl = null; - } - }); - - this.revokeImageUrl(); - }, - revokeImageUrl() { - if (this.imageUrl) { - URL.revokeObjectURL(this.imageUrl); - this.imageUrl = null; - } - }, - getImageUrl() { - if (this.imageUrl === undefined) { - this.updateImageUrl(); - } - return this.imageUrl; }, getQuoteObjectUrl() { const thumbnail = this.quoteThumbnail; diff --git a/js/modules/signal.js b/js/modules/signal.js index 4db7680bc..3357baf6f 100644 --- a/js/modules/signal.js +++ b/js/modules/signal.js @@ -110,12 +110,14 @@ function initializeMigrations({ const readAttachmentData = createReader(attachmentsPath); const loadAttachmentData = Type.loadData(readAttachmentData); const getAbsoluteAttachmentPath = createAbsolutePathGetter(attachmentsPath); + const deleteOnDisk = Attachments.createDeleter(attachmentsPath); return { attachmentsPath, - deleteAttachmentData: Type.deleteData( - Attachments.createDeleter(attachmentsPath) - ), + deleteExternalMessageFiles: MessageType.deleteAllExternalFiles({ + deleteAttachmentData: Type.deleteData(deleteOnDisk), + deleteOnDisk, + }), getAbsoluteAttachmentPath, getPlaceholderMigrations, loadAttachmentData, diff --git a/js/modules/types/attachment.js b/js/modules/types/attachment.js index 36cda90b6..89df04d8b 100644 --- a/js/modules/types/attachment.js +++ b/js/modules/types/attachment.js @@ -158,26 +158,28 @@ exports.loadData = readAttachmentData => { // deleteData :: (RelativePath -> IO Unit) // Attachment -> // IO Unit -exports.deleteData = deleteAttachmentData => { - if (!is.function(deleteAttachmentData)) { - throw new TypeError("'deleteAttachmentData' must be a function"); +exports.deleteData = deleteOnDisk => { + if (!is.function(deleteOnDisk)) { + throw new TypeError('deleteData: deleteOnDisk must be a function'); } return async attachment => { if (!exports.isValid(attachment)) { - throw new TypeError("'attachment' is not valid"); + throw new TypeError('deleteData: attachment is not valid'); } - const hasDataInMemory = exports.hasData(attachment); - if (hasDataInMemory) { - return; + const { path, thumbnail, screenshot } = attachment; + if (is.string(path)) { + await deleteOnDisk(path); } - if (!is.string(attachment.path)) { - throw new TypeError("'attachment.path' is required"); + if (thumbnail && is.string(thumbnail.path)) { + await deleteOnDisk(thumbnail.path); } - await deleteAttachmentData(attachment.path); + if (screenshot && is.string(screenshot.path)) { + await deleteOnDisk(screenshot.path); + } }; }; diff --git a/js/modules/types/message.js b/js/modules/types/message.js index 7af7c4b53..711ec6c1a 100644 --- a/js/modules/types/message.js +++ b/js/modules/types/message.js @@ -361,6 +361,52 @@ exports.createAttachmentLoader = loadAttachmentData => { }); }; +exports.deleteAllExternalFiles = ({ deleteAttachmentData, deleteOnDisk }) => { + if (!isFunction(deleteAttachmentData)) { + throw new TypeError( + 'deleteAllExternalFiles: deleteAttachmentData must be a function' + ); + } + + if (!isFunction(deleteOnDisk)) { + throw new TypeError( + 'deleteAllExternalFiles: deleteOnDisk must be a function' + ); + } + + return async message => { + const { attachments, quote, contact } = message; + + if (attachments && attachments.length) { + await Promise.all(attachments.map(deleteAttachmentData)); + } + + if (quote && quote.attachments && quote.attachments.length) { + await Promise.all( + quote.attachments.map(async attachment => { + const { thumbnail } = attachment; + + if (thumbnail.path) { + await deleteOnDisk(thumbnail.path); + } + }) + ); + } + + if (contact && contact.length) { + await Promise.all( + contact.map(async item => { + const { avatar } = item; + + if (avatar && avatar.avatar && avatar.avatar.path) { + await deleteOnDisk(avatar.avatar.path); + } + }) + ); + } + }; +}; + // createAttachmentDataWriter :: (RelativePath -> IO Unit) // Message -> // IO (Promise Message) diff --git a/test/models/messages_test.js b/test/models/messages_test.js index 78536c467..e6708fc86 100644 --- a/test/models/messages_test.js +++ b/test/models/messages_test.js @@ -27,29 +27,15 @@ var source = '+14155555555'; describe('MessageCollection', function() { - before(function() { - return Promise.all([deleteAllMessages(), ConversationController.load()]); + before(async function() { + await deleteAllMessages(); + ConversationController.reset(); + await ConversationController.load(); }); after(function() { return deleteAllMessages(); }); - it('has no image url', function() { - var messages = new Whisper.MessageCollection(); - var message = messages.add(attributes); - assert.isNull(message.getImageUrl()); - }); - - it('updates image url', function() { - var messages = new Whisper.MessageCollection(); - var message = messages.add({ attachments: [attachment] }); - - var firstUrl = message.getImageUrl(); - message.updateImageUrl(); - var secondUrl = message.getImageUrl(); - assert.notEqual(secondUrl, firstUrl); - }); - it('gets outgoing contact', function() { var messages = new Whisper.MessageCollection(); var message = messages.add(attributes);