Merge pull request #343 from asutherland/email-getbody-fix

Bug 1070429 - [email/IMAP] premature saving leads to missing bodies. r=jrburke

js/activesync/folder.js

@@ -3,6 +3,7 @@ define(
     'rdcommon/log',
     '../date',
     '../syncbase',
+    '../allback',
     '../db/mail_rep',
     'activesync/codepages/AirSync',
     'activesync/codepages/AirSyncBase',
@@ -19,6 +20,7 @@ define(
     $log,
     $date,
     $sync,
+    allback,
     mailRep,
     $AirSync,
     $AirSyncBase,
@@ -844,44 +846,30 @@ ActiveSyncFolderConn.prototype = {
     if (this._account.conn.currentVersion.lt('12.0'))
       return this._syncBodies(headers, callback);
 
-    var anyErr,
-        pending = 1,
-        downloadsNeeded = 0,
+    var downloadsNeeded = 0,
         folderConn = this;
 
-    function next(err) {
-      if (err && !anyErr)
-        anyErr = err;
-
-      if (!--pending) {
-        folderConn._storage.runAfterDeferredCalls(function() {
-          callback(anyErr, /* number downloaded */ downloadsNeeded - pending);
-        });
-      }
-    }
-
+    var latch = allback.latch();
     for (var i = 0; i < headers.length; i++) {
+      var header = headers[i];
       // We obviously can't do anything with null header references.
       // To avoid redundant work, we also don't want to do any fetching if we
       // already have a snippet.  This could happen because of the extreme
       // potential for a caller to spam multiple requests at us before we
       // service any of them.  (Callers should only have one or two outstanding
       // jobs of this and do their own suppression tracking, but bugs happen.)
-      if (!headers[i] || headers[i].snippet !== null) {
+      if (!header || header.snippet !== null) {
         continue;
       }
 
-      pending++;
       // This isn't absolutely guaranteed to be 100% correct, but is good enough
       // for indicating to the caller that we did some work.
       downloadsNeeded++;
-      this.downloadBodyReps(headers[i], options, next);
+      this.downloadBodyReps(header, options, latch.defer(header.suid));
     }
-
-    // by having one pending item always this handles the case of not having any
-    // snippets needing a download and also returning in the next tick of the
-    // event loop.
-    window.setZeroTimeout(next);
+    latch.then(function(results) {
+      callback(allback.extractErrFromCallbackArgs(results), downloadsNeeded);
+    });
   },
 
   downloadBodyReps: lazyConnection(1, function(header, options, callback) {
@@ -1024,48 +1012,39 @@ ActiveSyncFolderConn.prototype = {
         return;
       }
 
-      var status, anyErr,
-          i = 0,
-          pending = 1;
-
-      function next(err) {
-        if (err && !anyErr)
-          anyErr = err;
-
-        if (!--pending) {
-          folderConn._storage.runAfterDeferredCalls(function() {
-            callback(anyErr);
-          });
-        }
-      }
+      var latch = allback.latch();
+      var iHeader = 0;
 
       var e = new $wbxml.EventParser();
       var base = [as.Sync, as.Collections, as.Collection];
       e.addEventListener(base.concat(as.SyncKey), function(node) {
         folderConn.syncKey = node.children[0].textContent;
       });
       e.addEventListener(base.concat(as.Status), function(node) {
-        status = node.children[0].textContent;
+        var status = node.children[0].textContent;
+        if (status !== asEnum.Status.Success) {
+          latch.defer('status')('unknown');
+        }
       });
       e.addEventListener(base.concat(as.Responses, as.Fetch,
                                      as.ApplicationData, em.Body),
                          function(node) {
         // We assume the response is in the same order as the request!
-        var header = headers[i++];
+        var header = headers[iHeader++];
         var bodyContent = node.children[0].textContent;
+        var latchCallback = latch.defer(header.suid);
 
-        pending++;
         folderConn._storage.getMessageBody(header.suid, header.date,
                                            function(body) {
-          folderConn._updateBody(header, body, bodyContent, false, next);
+          folderConn._updateBody(header, body, bodyContent, false,
+                                 latchCallback);
         });
       });
       e.run(aResponse);
 
-      if (status !== asEnum.Status.Success)
-        return next('unknown');
-
-      next(null);
+      latch.then(function(results) {
+        callback(allback.extractErrFromCallbackArgs(results));
+      });
     });
   },
 
@@ -1105,10 +1084,12 @@ ActiveSyncFolderConn.prototype = {
       }
     };
 
+    var latch = allback.latch();
     this._storage.updateMessageHeader(header.date, header.id, false, header,
-                                      bodyInfo);
-    this._storage.updateMessageBody(header, bodyInfo, {}, event);
-    this._storage.runAfterDeferredCalls(callback.bind(null, null, bodyInfo));
+                                      bodyInfo, latch.defer('header'));
+    this._storage.updateMessageBody(header, bodyInfo, {}, event,
+                                    latch.defer('body'));
+    latch.then(callback.bind(null, null, bodyInfo, /* flushed */ false));
   },
 
   sync: lazyConnection(1, function asfc_sync(accuracyStamp, doneCallback,
@@ -1140,6 +1121,7 @@ ActiveSyncFolderConn.prototype = {
         return;
       }
 
+      var latch = allback.latch();
       for (var iter in Iterator(added)) {
         var message = iter[1];
         // If we already have this message, it's probably because we moved it as
@@ -1148,8 +1130,8 @@ ActiveSyncFolderConn.prototype = {
         if (storage.hasMessageWithServerId(message.header.srvid))
           continue;
 
-        storage.addMessageHeader(message.header, message.body);
-        storage.addMessageBody(message.header, message.body);
+        storage.addMessageHeader(message.header, message.body, latch.defer());
+        storage.addMessageBody(message.header, message.body, latch.defer());
         addedMessages++;
       }
 
@@ -1175,7 +1157,7 @@ ActiveSyncFolderConn.prototype = {
         // Previously, this callback was called without safeguards in place
         // to prevent issues caused by the message variable changing,
         // so it is now bound to the function.
-        }.bind(null, message), /* body hint */ null);
+        }.bind(null, message), /* body hint */ null, latch.defer());
         changedMessages++;
         // XXX: update bodies
       }
@@ -1187,17 +1169,17 @@ ActiveSyncFolderConn.prototype = {
         if (!storage.hasMessageWithServerId(messageGuid))
           continue;
 
-        storage.deleteMessageByServerId(messageGuid);
+        storage.deleteMessageByServerId(messageGuid, latch.defer());
         deletedMessages++;
       }
 
       if (!moreAvailable) {
         var messagesSeen = addedMessages + changedMessages + deletedMessages;
 
         // Do not report completion of sync until all of our operations have
-        // been persisted to our in-memory database.  (We do not wait for
-        // things to hit the disk.)
-        storage.runAfterDeferredCalls(function() {
+        // been persisted to our in-memory database.  We tell this via their
+        // callbacks having completed.
+        latch.then(function() {
           // Note: For the second argument here, we report the number of
           // messages we saw that *changed*. This differs from IMAP, which
           // reports the number of messages it *saw*.

js/allback.js

@@ -26,7 +26,8 @@ define(['exports'], function(exports) {
  * potential memory leaks is not currently provided, but could be.
  */
 exports.allbackMaker = function allbackMaker(names, allDoneCallback) {
-  var aggrData = {}, callbacks = {}, waitingFor = names.concat();
+  var aggrData = Object.create(null), callbacks = {},
+      waitingFor = names.concat();
 
   names.forEach(function(name) {
     // (build a consistent shape for aggrData regardless of callback ordering)
@@ -92,7 +93,8 @@ exports.allbackMaker = function allbackMaker(names, allDoneCallback) {
 exports.latch = function() {
   var ready = false;
   var deferred = {};
-  var results = {};
+  // Avoid Object.prototype and any for-enumerations getting tripped up
+  var results = Object.create(null);
   var count = 0;
 
   deferred.promise = new Promise(function(resolve, reject) {
@@ -139,9 +141,37 @@ exports.latch = function() {
   };
 };
 
+/**
+ * Given the results object from an allback.latch() where named callbacks were
+ * used (or else we won't save the result!) and the callbacks use the form of
+ * callback(errIfAny, ...), find and return the first error object, or return
+ * null if none was found.
+ *
+ * Important notes:
+ * - Use this for callback-based idioms in the node style
+ * - You MUST use latch.defer(name), not latch.defer()!
+ * - Because of JS object property ordering, we actually will return the result
+ *   of the first callback that fired with an error value, but you probably
+ *   do not want to be depending on this too much.
+ */
+exports.extractErrFromCallbackArgs = function(results) {
+  // If there are any errors, find and propagate.
+  var anyErr = null;
+  for (var key in results) {
+    var args = results[key];
+    var errIfAny = args[0];
+    if (errIfAny) {
+      anyErr = errIfAny;
+      break;
+    }
+  }
+  return anyErr;
+};
+
 exports.latchedWithRejections = function(namedPromises) {
   return new Promise(function(resolve, reject) {
-    var results = {};
+    // Avoid Object.prototype
+    var results = Object.create(null);
     var pending = 0;
     Object.keys(namedPromises).forEach(function(name) {
       pending++;

js/db/mail_rep.js

@@ -107,10 +107,6 @@ function makeHeaderInfo(raw) {
  *     from the body without this.
  *   }
  *   @key[size Number]
- *   @key[to @listof[NameAddressPair]]
- *   @key[cc @listof[NameAddressPair]]
- *   @key[bcc @listof[NameAddressPair]]
- *   @key[replyTo NameAddressPair]
  *   @key[attaching #:optional AttachmentInfo]{
  *     Because of memory limitations, we need to encode and attach attachments
  *     in small pieces.  An attachment in the process of being attached is

js/ext/rdcommon/log.js

@@ -1795,14 +1795,13 @@ exports.DEBUG_realtimeLogEverything = function(dumpFunc) {
     reportNewLogger: function(logger, parentLogger) {
       logger._actor = {
         __loggerFired: function() {
-          // Destructively pop it off the end of the list; inductively this
-          // should always be index 0, but you never know!
-          var entry = logger._entries.pop();
+          var entry = logger._entries[logger._entries.length - 1];
           // Let's look like: LoggerType(semanticIdent)["name", ...]
           dumpFunc(logger.__defName + '(' + logger._ident + ')' +
                    JSON.stringify(entry) + '\n');
         }
       };
+      return parentLogger;
     }
   };
   UNDER_TEST_DEFAULT = EverythingTester;