From 9655d0fd873e9692d9ab047d17cc783aa5c205f8 Mon Sep 17 00:00:00 2001 From: Marcus Cavanaugh Date: Thu, 12 Dec 2013 13:15:40 -0800 Subject: [PATCH 1/2] Bug 912825 - [email] Message reader appears to duplicate body contents when buildBodyDom is called multiple times ( --- data/lib/mailapi/jobmixins.js | 13 ++- data/lib/mailapi/mailapi.js | 10 ++- data/lib/mailapi/syncbase.js | 13 +++ data/lib/pop3/pop3.js | 25 ++---- test/test-files.json | 4 + .../unit/test_downloadbodyreps_idempotency.js | 79 +++++++++++++++++++ 6 files changed, 124 insertions(+), 20 deletions(-) create mode 100644 test/unit/test_downloadbodyreps_idempotency.js diff --git a/data/lib/mailapi/jobmixins.js b/data/lib/mailapi/jobmixins.js index 61660c6a..85a5cadc 100644 --- a/data/lib/mailapi/jobmixins.js +++ b/data/lib/mailapi/jobmixins.js @@ -430,7 +430,18 @@ exports.do_downloadBodyReps = function(op, callback) { return; } - folderConn.downloadBodyReps(header, onDownloadReps); + // Check to see if we've already downloaded the bodyReps for this + // message. If so, no need to even try to fetch them again. This + // allows us to enforce an idempotency guarantee regarding how + // many times body change notifications will be fired. + folderStorage.getMessageBody(header.suid, header.date, + function(body) { + if (!body.bodyReps.every(function(rep) { return rep.isDownloaded; })) { + folderConn.downloadBodyReps(header, onDownloadReps); + } else { + onDownloadReps(null, body, true); + } + }); }; var onDownloadReps = function onDownloadReps(err, bodyInfo, flushed) { diff --git a/data/lib/mailapi/mailapi.js b/data/lib/mailapi/mailapi.js index f009675f..3305b6a2 100644 --- a/data/lib/mailapi/mailapi.js +++ b/data/lib/mailapi/mailapi.js @@ -913,8 +913,14 @@ MailHeader.prototype = { }, /** - * Request the `MailBody` instance for this message, passing it to the - * provided callback function once retrieved. + * Request the `MailBody` instance for this message, passing it to + * the provided callback function once retrieved. If you request the + * bodyReps as part of this call, the backend guarantees that it + * will only call the "onchange" notification when the body has + * actually changed. In other words, if you end up calling getBody() + * multiple times for some reason, the backend will be smart about + * only fetching the bodyReps the first time and generating change + * notifications as one would expect. * * @args[ * @param[options @dict[ diff --git a/data/lib/mailapi/syncbase.js b/data/lib/mailapi/syncbase.js index 061f21f8..0252fb5a 100644 --- a/data/lib/mailapi/syncbase.js +++ b/data/lib/mailapi/syncbase.js @@ -129,6 +129,19 @@ exports.POP3_SAVE_STATE_EVERY_N_MESSAGES = 50; */ exports.POP3_MAX_MESSAGES_PER_SYNC = 100; + +/** + * If a message is larger than INFER_ATTACHMENTS_SIZE bytes, guess + * that it has an attachment. + */ +exports.POP3_INFER_ATTACHMENTS_SIZE = 512 * 1024; + + +/** + * Attempt to fetch this many bytes of messages during snippet fetching. + */ +exports.POP3_SNIPPET_SIZE_GOAL = 4 * 1024; // in bytes + //////////////////////////////////////////////////////////////////////////////// // General Sync Constants diff --git a/data/lib/pop3/pop3.js b/data/lib/pop3/pop3.js index ac9dff17..1cd9496e 100644 --- a/data/lib/pop3/pop3.js +++ b/data/lib/pop3/pop3.js @@ -1,9 +1,10 @@ define(['module', 'exports', 'rdcommon/log', 'net', 'crypto', './transport', 'mailparser/mailparser', '../mailapi/imap/imapchew', + '../mailapi/syncbase', './mime_mapper', '../mailapi/allback'], function(module, exports, log, net, crypto, transport, mailparser, imapchew, - mimeMapper, allback) { + syncbase, mimeMapper, allback) { /** * The Pop3Client modules and classes are organized according to @@ -53,20 +54,6 @@ function(module, exports, log, net, crypto, clearTimeout = clear; } - // CONSTANTS: - - // If a message is larger than INFER_ATTACHMENTS_SIZE bytes, guess - // that it has an attachment. - var INFER_ATTACHMENTS_SIZE = 512 * 1024; - - // Attempt to fetch SNIPPET_SIZE_GOAL bytes for each message to - // generate the snippet. - var SNIPPET_SIZE_GOAL = 4 * 1024; // in bytes - // Based on SNIPPET_SIZE_GOAL, calculate approximately how many - // lines we'll need to fetch in order to roughly retrieve - // SNIPPET_SIZE_GOAL bytes. - var LINES_TO_FETCH_FOR_SNIPPET = Math.floor(SNIPPET_SIZE_GOAL / 80); - /*************************************************************************** * Pop3Client * @@ -680,7 +667,11 @@ function(module, exports, log, net, crypto, // it creates unnecessary garbage. Clean this up when we switch over // to jsmime. Pop3Client.prototype.downloadPartialMessageByNumber = function(number, cb) { - this.protocol.sendRequest('TOP', [number, LINES_TO_FETCH_FOR_SNIPPET], + // Based on SNIPPET_SIZE_GOAL, calculate approximately how many + // lines we'll need to fetch in order to roughly retrieve + // SNIPPET_SIZE_GOAL bytes. + var numLines = Math.floor(syncbase.POP3_SNIPPET_SIZE_GOAL / 80); + this.protocol.sendRequest('TOP', [number, numLines], true, function(err, rsp) { if(err) { cb && cb({ @@ -882,7 +873,7 @@ function(module, exports, log, net, crypto, !rep.header.hasAttachments && (rootNode.parsedHeaders['x-ms-has-attach'] || rootNode.meta.mimeMultipart === 'mixed' || - estSize > INFER_ATTACHMENTS_SIZE)) { + estSize > syncbase.POP3_INFER_ATTACHMENTS_SIZE)) { rep.header.hasAttachments = true; } diff --git a/test/test-files.json b/test/test-files.json index e266eb21..cc116791 100644 --- a/test/test-files.json +++ b/test/test-files.json @@ -95,6 +95,10 @@ "variants": ["activesync:fake", "pop3:fake"] }, + "test_downloadbodyreps_idempotency.js": { + "variants": ["imap:fake", "activesync:fake", "pop3:fake"] + }, + "test_pop3_checkpoint_sync.js": { "variants": ["pop3:fake"] }, diff --git a/test/unit/test_downloadbodyreps_idempotency.js b/test/unit/test_downloadbodyreps_idempotency.js new file mode 100644 index 00000000..b947048c --- /dev/null +++ b/test/unit/test_downloadbodyreps_idempotency.js @@ -0,0 +1,79 @@ +define(['rdcommon/testcontext', './resources/th_main', 'exports'], + function($tc, $th_imap, exports) { + +var TD = exports.TD = $tc.defineTestsFor( + { id: 'test_downloadbodyreps_idempotency' }, + null, + [$th_imap.TESTHELPER], ['app'] +); + +TD.commonCase('fetch only snippets', function(T, RT) { + var testUniverse = T.actor('testUniverse', 'U'), + testAccount = T.actor('testAccount', 'A', { universe: testUniverse }); + + // Create a folder to test on + var eLazy = T.lazyLogger('misc'); + var folderName = 'test_downloadbodyreps_idempotency'; + var messageCount = 3; + + // Set POP3 to not retrieve any of the message when fetching + // headers. Otherwise it might have already finished downloading + // short messages, which would make the assertions below + // inconsistent between prototols. + testUniverse.do_adjustSyncValues({ + POP3_SNIPPET_SIZE_GOAL: 0 + }); + + // Use the inbox, so that POP3 will actually run its sync logic. + var testFolder = testAccount.do_useExistingFolderWithType('inbox', ''); + testAccount.do_addMessagesToFolder(testFolder, { count: messageCount }); + var testView = testAccount.do_openFolderView( + 'syncs', testFolder, null, null, { syncedToDawnOfTime: 'ignore' }); + + // When requesting bodyReps multiple times, we should only see one + // set of "onchange" notifications -- after we actually download and + // change the body. In other words, we enforce an idempotency + // guarantee that the frontend doesn't have to worry about + // spurious/redundant body "onchange" notifications. + T.action('request full body after snippets', eLazy, function() { + eLazy.expectUseSetMatching(); + + testView.slice.items.forEach(function(header, idx) { + var whichCall = 0; + + // The first call should receive a modified onchange event. + eLazy.expect_value('modified-' + idx); + // Then we called getBody twice, so we should see two more + // "done" events _without_ seeing more change events. + eLazy.expect_value('done-' + idx); + eLazy.expect_value('done-' + idx); + + function gotBody(body) { + whichCall++; + if (whichCall === 1) { + // Attach the handler for this body here; it should only be + // called once even though we're calling getBody multiple + // times. + body.onchange = function() { + eLazy.value('modified-' + idx); + } + } else { + header.getBody({ withBodyReps: true }, function() { + eLazy.value('done-' + idx); + }); + } + } + + // Fetch the body thrice; the first will generate onchange; + // the other two should just indicate that we've finished. + header.getBody({ downloadBodyReps: true }, gotBody); + header.getBody({ downloadBodyReps: true }, gotBody); + header.getBody({ downloadBodyReps: true }, gotBody); + }); + }); + +// testAccount.do_closeFolderView(testView); + +}); + +}); // end define From 34ead0ab2eee06709c84a1abe64524fab31a26be Mon Sep 17 00:00:00 2001 From: Marcus Cavanaugh Date: Mon, 16 Dec 2013 11:08:09 -0800 Subject: [PATCH 2/2] Clean up tests per @asutherland --- data/lib/mailapi/jobmixins.js | 3 +- data/lib/mailapi/pop3/account.js | 6 +- data/lib/mailapi/pop3/sync.js | 8 +- .../unit/test_downloadbodyreps_idempotency.js | 89 ++++++++++++------- 4 files changed, 68 insertions(+), 38 deletions(-) diff --git a/data/lib/mailapi/jobmixins.js b/data/lib/mailapi/jobmixins.js index 85a5cadc..d454f730 100644 --- a/data/lib/mailapi/jobmixins.js +++ b/data/lib/mailapi/jobmixins.js @@ -439,7 +439,8 @@ exports.do_downloadBodyReps = function(op, callback) { if (!body.bodyReps.every(function(rep) { return rep.isDownloaded; })) { folderConn.downloadBodyReps(header, onDownloadReps); } else { - onDownloadReps(null, body, true); + // passing flushed = true because we don't need to save anything + onDownloadReps(null, body, /* flushed = */ true); } }); }; diff --git a/data/lib/mailapi/pop3/account.js b/data/lib/mailapi/pop3/account.js index f82ce7dd..78cfe14d 100644 --- a/data/lib/mailapi/pop3/account.js +++ b/data/lib/mailapi/pop3/account.js @@ -58,7 +58,7 @@ var properties = { * abstracts the `done` callback.) * @param {function(err, conn, done)} cb */ - withConnection: function(cb) { + withConnection: function(cb, whyLabel) { // This implementation serializes withConnection requests so that // we don't step on requests' toes. While Pop3Client wouldn't mix // up the requests themselves, interleaving different operations @@ -76,7 +76,7 @@ var properties = { } }.bind(this); if (!this._conn || this._conn.state === 'disconnected') { - this._makeConnection(next); + this._makeConnection(next, whyLabel); } else { next(); } @@ -225,7 +225,7 @@ var properties = { } this.withConnection(function(err) { callback(err); - }); + }, 'checkAccount'); }, /** diff --git a/data/lib/mailapi/pop3/sync.js b/data/lib/mailapi/pop3/sync.js index 86f15d70..075898f3 100644 --- a/data/lib/mailapi/pop3/sync.js +++ b/data/lib/mailapi/pop3/sync.js @@ -37,8 +37,9 @@ exports.Pop3FolderSyncer = Pop3FolderSyncer; * * @param {boolean} getNew If a fresh connection should always be made. * @param {int} cbIndex Index of the parent function's callback in args + * @param {string} whyLabel Description for why we need the connection */ -function lazyWithConnection(getNew, cbIndex, fn) { +function lazyWithConnection(getNew, cbIndex, whyLabel, fn) { return function pop3LazyWithConnection() { var args = Array.slice(arguments); require([], function () { @@ -61,7 +62,7 @@ function lazyWithConnection(getNew, cbIndex, fn) { }; fn.apply(this, [conn].concat(args)); } - }.bind(this)); + }.bind(this), whyLabel); }.bind(this); // if we require a fresh connection, close out the old one first. @@ -90,6 +91,7 @@ Pop3FolderSyncer.prototype = { * body part/message downloading. XXX rename this family of methods. */ downloadBodies: lazyWithConnection(/* getNew = */ false, /* cbIndex = */ 2, + /* whyLabel = */ 'downloadBodies', function(conn, headers, options, callback) { var latch = allback.latch(); var storage = this.storage; @@ -117,6 +119,7 @@ Pop3FolderSyncer.prototype = { * all in one go. */ downloadBodyReps: lazyWithConnection(/* getNew = */ false, /* cbIndex = */ 2, + /* whyLabel = */ 'downloadBodyReps', function(conn, header, options, callback) { if (options instanceof Function) { callback = options; @@ -445,6 +448,7 @@ Pop3FolderSyncer.prototype = { * `this.storeMessageUidlForMessageId`). */ sync: lazyWithConnection(/* getNew = */ true, /* cbIndex = */ 2, + /* whyLabel = */ 'sync', function(conn, syncType, slice, doneCallback, progressCallback) { // if we could not establish a connection, abort the sync. var self = this; diff --git a/test/unit/test_downloadbodyreps_idempotency.js b/test/unit/test_downloadbodyreps_idempotency.js index b947048c..e3c33d49 100644 --- a/test/unit/test_downloadbodyreps_idempotency.js +++ b/test/unit/test_downloadbodyreps_idempotency.js @@ -14,7 +14,10 @@ TD.commonCase('fetch only snippets', function(T, RT) { // Create a folder to test on var eLazy = T.lazyLogger('misc'); var folderName = 'test_downloadbodyreps_idempotency'; - var messageCount = 3; + // We want just one message in the inbox; IMAP already adds one for + // tests so that we can detect timezones, so only add one for other + // account types. + var messageCount = (testAccount.type === 'imap' ? 0 : 1); // Set POP3 to not retrieve any of the message when fetching // headers. Otherwise it might have already finished downloading @@ -26,7 +29,9 @@ TD.commonCase('fetch only snippets', function(T, RT) { // Use the inbox, so that POP3 will actually run its sync logic. var testFolder = testAccount.do_useExistingFolderWithType('inbox', ''); - testAccount.do_addMessagesToFolder(testFolder, { count: messageCount }); + if (messageCount > 0) { + testAccount.do_addMessagesToFolder(testFolder, { count: messageCount }); + } var testView = testAccount.do_openFolderView( 'syncs', testFolder, null, null, { syncedToDawnOfTime: 'ignore' }); @@ -36,44 +41,64 @@ TD.commonCase('fetch only snippets', function(T, RT) { // guarantee that the frontend doesn't have to worry about // spurious/redundant body "onchange" notifications. T.action('request full body after snippets', eLazy, function() { - eLazy.expectUseSetMatching(); + // only the first job will actually download the bodies, the other + // jobs will still happen but will turn into no-ops + // this might need conn: true/etc. - testView.slice.items.forEach(function(header, idx) { - var whichCall = 0; + // We need three of these: Two for "downloadBodyReps" calls, and + // one for the "withBodyReps" call. Only the first will actually + // cause us to download and save the bodyReps. + testAccount.expect_runOp('downloadBodyReps', { + local: false, server: true, save: 'server' }); + testAccount.expect_runOp('downloadBodyReps', { + local: false, server: true, save: false }); + testAccount.expect_runOp('downloadBodyReps', { + local: false, server: true, save: false }); - // The first call should receive a modified onchange event. - eLazy.expect_value('modified-' + idx); - // Then we called getBody twice, so we should see two more - // "done" events _without_ seeing more change events. - eLazy.expect_value('done-' + idx); - eLazy.expect_value('done-' + idx); + // TODO: POP3's logging (or th_main helper utilities) should be + // cleaned up to handle connection expectations in a way that lets + // us use the same expectations for IMAP as POP3. In this case, + // POP3 sets "USES_CONN = false", which renders the expect_runOp's + // "conn" helper useless. For IMAP, we already have a connection + // from the do_openFolderView, so we don't need conn: true here. + // But for POP3, the following expectations are needed to perform + // the same function as what "conn: true" woul dhave done in + // expect_runOp above, if POP3 supported that properly. + if (testAccount.type === 'pop3') { + RT.reportActiveActorThisStep(testAccount.eFolderAccount); + testAccount.eFolderAccount.ignore_createConnection(); + testAccount.eFolderAccount.ignore_saveAccountState(); + } - function gotBody(body) { - whichCall++; - if (whichCall === 1) { - // Attach the handler for this body here; it should only be - // called once even though we're calling getBody multiple - // times. - body.onchange = function() { - eLazy.value('modified-' + idx); - } - } else { - header.getBody({ withBodyReps: true }, function() { - eLazy.value('done-' + idx); - }); - } + // there's only one message in the inbox + var header = testView.slice.items[0]; + + // The first call should receive a modified onchange event. + eLazy.expect_value('modified'); + // Then we called getBody twice, so we should see two more + // "done" events _without_ seeing more change events. + eLazy.expect_value('done'); + + // Fetch the body thrice; the first will generate onchange; + // the other two should just indicate that we've finished. + header.getBody({ downloadBodyReps: true }, function (body) { + // Attach the handler for this body here; it should only be + // called once even though we're calling getBody multiple + // times. + body.onchange = function() { + eLazy.value('modified'); } + }); - // Fetch the body thrice; the first will generate onchange; - // the other two should just indicate that we've finished. - header.getBody({ downloadBodyReps: true }, gotBody); - header.getBody({ downloadBodyReps: true }, gotBody); - header.getBody({ downloadBodyReps: true }, gotBody); + header.getBody({ downloadBodyReps: true }, function(body) { + // Use { withBodyReps: true } so that the 'done' event + // happens after we see onchange. + header.getBody({ withBodyReps: true }, function() { + eLazy.value('done'); + }); }); }); -// testAccount.do_closeFolderView(testView); - }); }); // end define