Skip to content
This repository has been archived by the owner on Nov 3, 2021. It is now read-only.

Commit

Permalink
Merge pull request #275 from mcav/downloadbodyreps-idempotent
Browse files Browse the repository at this point in the history
Bug 912825 - [email] Message reader appears to duplicate body contents when buildBodyDom is called multiple times. r=asuth
  • Loading branch information
mcav committed Jan 3, 2014
2 parents dbf456e + 34ead0a commit c348db8
Show file tree
Hide file tree
Showing 8 changed files with 159 additions and 25 deletions.
14 changes: 13 additions & 1 deletion data/lib/mailapi/jobmixins.js
Expand Up @@ -430,7 +430,19 @@ 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 {
// passing flushed = true because we don't need to save anything
onDownloadReps(null, body, /* flushed = */ true);
}
});
};

var onDownloadReps = function onDownloadReps(err, bodyInfo, flushed) {
Expand Down
10 changes: 8 additions & 2 deletions data/lib/mailapi/mailapi.js
Expand Up @@ -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[
Expand Down
6 changes: 3 additions & 3 deletions data/lib/mailapi/pop3/account.js
Expand Up @@ -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
Expand All @@ -76,7 +76,7 @@ var properties = {
}
}.bind(this);
if (!this._conn || this._conn.state === 'disconnected') {
this._makeConnection(next);
this._makeConnection(next, whyLabel);
} else {
next();
}
Expand Down Expand Up @@ -225,7 +225,7 @@ var properties = {
}
this.withConnection(function(err) {
callback(err);
});
}, 'checkAccount');
},

/**
Expand Down
8 changes: 6 additions & 2 deletions data/lib/mailapi/pop3/sync.js
Expand Up @@ -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 () {
Expand All @@ -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.
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
13 changes: 13 additions & 0 deletions data/lib/mailapi/syncbase.js
Expand Up @@ -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

Expand Down
25 changes: 8 additions & 17 deletions 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
Expand Down Expand Up @@ -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
*
Expand Down Expand Up @@ -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({
Expand Down Expand Up @@ -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;
}

Expand Down
4 changes: 4 additions & 0 deletions test/test-files.json
Expand Up @@ -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"]
},
Expand Down
104 changes: 104 additions & 0 deletions test/unit/test_downloadbodyreps_idempotency.js
@@ -0,0 +1,104 @@
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';
// 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
// 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', '');
if (messageCount > 0) {
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() {
// 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.

// 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 });

// 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();
}

// 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');
}
});

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');
});
});
});

});

}); // end define

0 comments on commit c348db8

Please sign in to comment.