Bug 564423: Get rid of "Part 1.2" attachments being displayed unless "Show all message parts" is selected r=asuth
authorJonathan Protzenko <jonathan.protzenko@gmail.com>
Tue, 09 Aug 2011 09:48:29 -0700
changeset 8274 5441c058d7145561172523c7a945deb70270b9af
parent 8273 34d03378d72b58da2c9185ad774f209bdc081371
child 8275 447605dd23677f139d315bde7caeac06ff1507de
push id6362
push userjonathan.protzenko@gmail.com
push dateTue, 09 Aug 2011 16:51:12 +0000
treeherdercomm-central@71b93e1bf96c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersasuth
bugs564423
Bug 564423: Get rid of "Part 1.2" attachments being displayed unless "Show all message parts" is selected r=asuth
mailnews/compose/public/nsIMsgSend.idl
mailnews/db/gloda/components/jsmimeemitter.js
mailnews/db/gloda/modules/fundattr.js
mailnews/db/gloda/modules/mimemsg.js
mailnews/db/gloda/test/unit/base_index_messages.js
mailnews/db/gloda/test/unit/test_mime_attachments_size.js
mailnews/db/gloda/test/unit/test_mime_emitter.js
mailnews/mime/src/mimemoz2.cpp
mailnews/mime/src/modlmime.h
--- a/mailnews/compose/public/nsIMsgSend.idl
+++ b/mailnews/compose/public/nsIMsgSend.idl
@@ -113,16 +113,17 @@ struct nsMsgAttachmentData
   char *description;      // If you put a string here, it will show up as the Content-Description header.  
                           // This can be any explanatory text; it's not a file name.             
 
   char *x_mac_type, *x_mac_creator; // Mac-specific data that should show up as optional parameters
                                     // to the content-type header.
   PRInt32 size;                  // The size of the attachment. May be 0.
   PRBool  isExternalAttachment;  // Flag for determining if the attachment is external
   PRBool  isDownloaded;          // Flag for determining if the attachment has already been downloaded
+  PRBool  hasFilename;           // Tells whether the name is provided by us or if it's a Part 1.2-like attachment
 };
 
 
 //
 // When we have downloaded a URL to a tmp file for attaching, this
 // represents everything we learned about it (and did to it) in the
 // process. 
 //
--- a/mailnews/db/gloda/components/jsmimeemitter.js
+++ b/mailnews/db/gloda/components/jsmimeemitter.js
@@ -366,25 +366,21 @@ MimeMessageEmitter.prototype = {
         this._partMap[partName].isRealAttachment = true;
       }
     }
     else if (aIsExternalAttachment) {
       // external attachments do not pass their part path information.
       // both aUrl in this call and the call to addAttachmentField (with
       // X-Mozilla-PartURL) receive the same thing; the URL to the file on disk.
     }
-    else {
-      if (partName) {
-        let part = new this._mimeMsg.MimeMessageAttachment(partName,
-            aName, aContentType, aUrl, aIsExternalAttachment);
-        if (part.isRealAttachment) {
-          // replace the existing part with the attachment...
-          this._replacePart(part);
-        }
-      }
+    else if (partName) {
+      let part = new this._mimeMsg.MimeMessageAttachment(partName,
+          aName, aContentType, aUrl, aIsExternalAttachment);
+      // replace the existing part with the attachment...
+      this._replacePart(part);
     }
   },
   addAttachmentField: function mime_emitter_addAttachmentField(aField, aValue) {
     // What gets passed in here is X-Mozilla-PartURL with a value that
     //  is completely identical to aUrl from the call to startAttachment.
     //  (it's the same variable they use in each case).  As such, there is
     //  no reason to handle that here.
     // However, we also pass information about the size of the attachment, and
--- a/mailnews/db/gloda/modules/fundattr.js
+++ b/mailnews/db/gloda/modules/fundattr.js
@@ -542,31 +542,53 @@ var GlodaFundAttr = {
     aGlodaMessage.bcc = bccIdentities;
 
     // -- Mailing List
     if (listIdentities.length)
       aGlodaMessage.mailingLists = listIdentities;
 
     // -- Attachments
     if (aMimeMsg) {
+      // nsParseMailbox.cpp puts the attachment flag on msgHdrs as soon as it
+      // finds a multipart/mixed part. This is a good heuristic, but if it turns
+      // out the part has no filename, then we don't treat it as an attachment.
+      // We just streamed the message, and we have all the information to figure
+      // that out, so now is a good place to clear the flag if needed.
+      let foundRealAttachment = false;
       let attachmentTypes = [];
       for each (let [, attachment] in Iterator(aMimeMsg.allAttachments)) {
         // We don't care about would-be attachments that are not user-intended
         //  attachments but rather artifacts of the message content.
         // We also want to avoid dealing with obviously bogus mime types.
         //  (If you don't have a "/", you are probably bogus.)
         if (attachment.isRealAttachment &&
             (attachment.contentType.indexOf("/") != -1)) {
           attachmentTypes.push(MimeTypeNoun.getMimeType(attachment.contentType));
         }
+        if (attachment.isRealAttachment)
+          foundRealAttachment = true;
       }
       if (attachmentTypes.length) {
         aGlodaMessage.attachmentTypes = attachmentTypes;
       }
 
+      let aMsgHdr = aRawReps.header;
+      let findIsEncrypted = function (x)
+        x.isEncrypted || (x.parts ? x.parts.some(findIsEncrypted) : false);
+      let wasStreamed = aMsgHdr &&
+        !findIsEncrypted(aMimeMsg)
+        ((aMsgHdr.flags & Ci.nsMsgMessageFlags.Offline) ||
+        (aMsgHdr.folder instanceof Ci.nsIMsgLocalMailFolder));
+
+      // Clear the flag if it turns out there's no attachment after all and we
+      // streamed completely the message (if we didn't, then we have no
+      // knowledge of attachments, unless bug 673370 is fixed).
+      if (!foundRealAttachment && wasStreamed)
+        aMsgHdr.markHasAttachments(false);
+
       // This is not the same kind of attachments as above. Now, we want to
       // provide convenience attributes to Gloda consumers, so that they can run
       // through the list of attachments of a given message, to possibly build a
       // visualization on top of it. We still reject bogus mime types, which
       // means yencode won't be supported. Oh, I feel really bad.
       let attachmentInfos = [];
       for each (let [, att] in Iterator(aMimeMsg.allUserAttachments)) {
         if (att.isRealAttachment) {
--- a/mailnews/db/gloda/modules/mimemsg.js
+++ b/mailnews/db/gloda/modules/mimemsg.js
@@ -617,33 +617,16 @@ MimeUnknown.prototype = {
       s += this._prettyHeaderString(aIndent + "  ");
     return s;
   },
   toString: function MimeUnknown_toString() {
     return "Unknown: " + this.contentType;
   }
 };
 
-const MIME_MSG_DEFAULT_ATTACHMENT_NAME = 1040;
-let localizedPartStr;
-
-/**
- * Part names are localized for display purposes; a longer term fix is proposed
- * on bug 554294, although since this will fix that bug, a new bug will likely
- * be filed and referenced by that bug.
- */
-function getLocalizedPartStr() {
-  let stringBundleService = Cc["@mozilla.org/intl/stringbundle;1"]
-                              .getService(Ci.nsIStringBundleService);
-  let mimeBundle = stringBundleService.createBundle(
-                     "chrome://messenger/locale/mime.properties");
-  localizedPartStr = mimeBundle.GetStringFromID(
-                       MIME_MSG_DEFAULT_ATTACHMENT_NAME);
-}
-
 /**
  * @class An attachment proper.  We think it's an attachment because it has a
  *  filename that libmime was able to figure out.
  *
  * @ivar partName @see{MimeMessage.partName}
  * @ivar name The filename of this attachment.
  * @ivar contentType The MIME content type of this part.
  * @ivar url The URL to stream if you want the contents of this part.
@@ -659,29 +642,19 @@ function MimeMessageAttachment(aPartName
   this.url = aUrl;
   this.isExternal = aIsExternal;
   // parts is copied over from the part instance that preceded us
   // headers is copied over from the part instance that preceded us
 }
 
 MimeMessageAttachment.prototype = {
   __proto__: HeaderHandlerBase,
-  /**
-   * Is this an actual attachment, as far as we can tell?  An example of
-   *  something that's not a real attachment is a mailing list footer that
-   *  gets its own MIME part because the original message had both HTML and text
-   *  as alternatives.
-   * Our super-advanced heuristic is to check whether the attachment name is
-   *  the same as the part name. Beware, this is localized.
-   */
+  // This is a legacy property.
   get isRealAttachment() {
-    if (!localizedPartStr)
-      getLocalizedPartStr();
-    let partName = localizedPartStr.replace("%s", this.partName);
-    return this.name != partName;
+    return true;
   },
   get allAttachments() {
     return [this]; // we are a leaf, so just us.
   },
   get allUserAttachments() {
     return [this];
   },
   prettyString: function MimeMessageAttachment_prettyString(aVerbose, aIndent,
--- a/mailnews/db/gloda/test/unit/base_index_messages.js
+++ b/mailnews/db/gloda/test/unit/base_index_messages.js
@@ -352,16 +352,56 @@ function test_threading() {
   mark_sub_test_start("missing intermediary");
   yield indexAndPermuteMessages(scenarios.missingIntermediary,
                                 allMessageInSameConversation);
   mark_sub_test_start("siblings missing parent");
   yield indexAndPermuteMessages(scenarios.siblingsMissingParent,
                                 allMessageInSameConversation);
 }
 
+/**
+ * Test the bit that says "if we're fulltext-indexing the message and we
+ *  discover it didn't have any attachments, clear the attachment bit from the
+ *  message header".
+ */
+function test_attachment_flag() {
+  // create a synthetic message with attachment
+  let smsg = msgGen.makeMessage({
+    name: 'test message with part 1.2 attachment',
+    bodyPart: new SyntheticPartMultiMixed([
+      new SyntheticPartLeaf({body: 'I like cheese!'}),
+      let (m = msgGen.makeMessage({ body: { body: 'I like wine!' }}))
+      (m._contentType = "roquefort", m)
+    ]),
+  });
+  // save it off for test_attributes_fundamental_from_disk
+  let msgSet = new SyntheticMessageSet([smsg]);
+  let folder = fundamentalFolderHandle = make_empty_folder();
+  yield add_sets_to_folders(folder, [msgSet]);
+
+  // if we need to go offline, let the indexing pass run, then force us offline
+  if (goOffline) {
+    yield wait_for_gloda_indexer(msgSet);
+    yield make_folder_and_contents_offline(folder);
+    // now the next indexer wait will wait for the next indexing pass...
+  }
+
+  yield wait_for_gloda_indexer(msgSet,
+                               {verifier: verify_attachment_flag});
+
+}
+
+function verify_attachment_flag(smsg, gmsg) {
+  // -- attachments. We won't have these if we don't have fulltext results
+  if (expectFulltextResults) {
+    do_check_eq(gmsg.attachmentNames.length, 0);
+    do_check_eq(gmsg.attachmentInfos.length, 0);
+    do_check_false(gmsg.folderMessage.flags & Ci.nsMsgMessageFlags.Attachment);
+  }
+}
 /* ===== Fundamental Attributes (per fundattr.js) ===== */
 
 /**
  * Save the synthetic message created in test_attributes_fundamental for the
  *  benefit of test_attributes_fundamental_from_disk.
  */
 var fundamentalSyntheticMessage;
 var fundamentalFolderHandle;
@@ -399,16 +439,17 @@ function test_attributes_fundamental() {
   if (goOffline) {
     yield wait_for_gloda_indexer(msgSet);
     yield make_folder_and_contents_offline(folder);
     // now the next indexer wait will wait for the next indexing pass...
   }
 
   yield wait_for_gloda_indexer(msgSet,
                                {verifier: verify_attributes_fundamental});
+
 }
 
 function verify_attributes_fundamental(smsg, gmsg) {
   // save off the message id for test_attributes_fundamental_from_disk
   fundamentalGlodaMessageId = gmsg.id;
 
   do_check_eq(gmsg.folderURI,
               get_real_injection_folder(fundamentalFolderHandle).URI);
@@ -1019,16 +1060,17 @@ function test_filthy_moves_slash_move_fr
 
 var tests = [
   test_pending_commit_tracker_flushes_correctly,
   test_pending_commit_causes_msgdb_commit,
   test_indexing_sweep,
   test_event_driven_indexing_does_not_mess_with_filthy_folders,
 
   test_threading,
+  test_attachment_flag,
   test_attributes_fundamental,
   test_attributes_fundamental_from_disk,
   test_attributes_explicit,
 
   test_streamed_bodies_are_size_capped,
 
   test_imap_add_unread_to_folder,
   test_message_moving,
--- a/mailnews/db/gloda/test/unit/test_mime_attachments_size.js
+++ b/mailnews/db/gloda/test/unit/test_mime_attachments_size.js
@@ -113,16 +113,20 @@ var yencText =
   "\x0d\x0a"+
   "=yend size=174 crc32=7efccd8e\n";
 
 // that completely exotic encoding is only detected if there is no content type
 // on the message, which is usually the case in newsgroups. I hate you yencode!
 var partYencText = new SyntheticPartLeaf("I am text! Woo!\n\n"+yencText,
   { contentType: '' } );
 
+var partUUText = new SyntheticPartLeaf(
+  "I am text! With uuencode... noes...\n\n"+uuText,
+  { contentType: '' });
+
 var tachText = {filename: 'bob.txt', body: originalText};
 
 var tachInlineText = {filename: 'foo.txt', body: originalText,
                       format: null, charset: null,
                       disposition: 'inline'};
 
 // images have a different behavior than other attachments: they are displayed
 // inline most of the time, so there are two different code paths that need to
@@ -142,16 +146,21 @@ var tachUU = {filename: 'john.doe', cont
 
 var tachApplication = {filename: 'funky.funk',
                        contentType: 'application/x-funky', body: originalText};
 
 var relImage = {contentType: 'image/png',
                 encoding: 'base64', charset: null, format: null,
                 contentId: 'part1.foo@bar.com',
                 body: b64Text};
+
+var tachVCard = {filename: 'bob.vcf', contentType: 'text/x-vcard',
+                 encoding: '7bit', body: 'begin:vcard\nfn:Bob\nend:vcard\n'};
+var partTachVCard = new SyntheticPartLeaf(tachVCard.body, tachVCard);
+
 var partRelImage = new SyntheticPartLeaf(relImage.body, relImage);
 
 var messageInfos = [
   // encoding type specific to newsgroups, not interested, gloda doesn't even
   // treat this as an attachment (probably because gloda requires an attachment
   // to have a content-type, which these yencoded parts don't have), but size IS
   // counted properly nonetheless
   /*{
@@ -257,29 +266,16 @@ function test_message_attachments(info) 
 
   yield false;
 }
 
 var bogusMessage = msgGen.makeMessage({ body: { body: originalText } });
 bogusMessage._contentType = "woooooo"; // Breaking abstraction boundaries. Bad.
 
 var bogusMessageInfos = [
-  // This message has a malformed part as an attachment, so it will end up as a
-  // MimeUnknown part. However, because that part precisely happens to be an
-  // attachment, it will have its size counted, so we do a specific count to
-  // make sure the size matches.
-  {
-    name: 'MimeUnknown attachment (actually, a message)',
-    bodyPart: new SyntheticPartMultiMixed([
-      partHtml,
-      bogusMessage,
-    ]),
-    epsilon: 6,
-    checkSize: true,
-  },
   // In this case, the wooooo part is not an attachment, so its bytes won't be
   // counted (size will end up being 0 bytes). We don't check the size, but
   // check_bogus_parts makes sure we're able to come up with a resulting size
   // for the MimeMessage.
   //
   // In that very case, since message M is an attachment, libmime will count M's
   // bytes, and we could have MimeMessages prefer the size libmime tells them
   // (when they have it), rather than recursively computing their sizes. I'm not
@@ -349,21 +345,51 @@ function test_bogus_messages(info) {
     } catch (e) {
       do_throw(e);
     }
   });
 
   yield false;
 }
 
+// The goal here is to explicitly check that these messages have attachments.
+var messageHaveAttachmentsInfos = [
+  {
+    name: 'multipart/related',
+    bodyPart: new SyntheticPartMultiMixed([partHtml, partTachVCard]),
+    number: 1,
+  },
+];
+
+function test_have_attachments(info) {
+  let synMsg = gMessageGenerator.makeMessage(info);
+  let synSet = new SyntheticMessageSet([synMsg]);
+  yield add_sets_to_folder(gInbox, [synSet]);
+
+  let msgHdr = synSet.getMsgHdr(0);
+  // dump(synMsg.toMboxString());
+
+  MsgHdrToMimeMessage(msgHdr, null, function(aMsgHdr, aMimeMsg) {
+    try {
+      do_check_eq(aMimeMsg.allUserAttachments.length, info.number);
+      async_driver();
+    } catch (e) {
+      do_throw(e);
+    }
+  });
+
+  yield false;
+}
+
 /* ===== Driver ===== */
 
 var tests = [
   parameterizeTest(test_message_attachments, messageInfos),
   parameterizeTest(test_bogus_messages, bogusMessageInfos),
+  parameterizeTest(test_have_attachments, messageHaveAttachmentsInfos),
 ];
 
 var gInbox;
 
 function run_test() {
   // use mbox injection because the fake server chokes sometimes right now
   gInbox = configure_message_injection({mode: "local"});
   async_run_tests(tests);
--- a/mailnews/db/gloda/test/unit/test_mime_emitter.js
+++ b/mailnews/db/gloda/test/unit/test_mime_emitter.js
@@ -521,22 +521,61 @@ function test_attachments_correctness ()
 
       async_driver();
     }, false);
 
     yield false;
   }
 }
 
+var bogusMessage = msgGen.makeMessage({ body: { body: "whatever" } });
+bogusMessage._contentType = "woooooo"; // Breaking abstraction boundaries. Bad.
+
+let weirdMessageInfos = [
+  // This message has a malformed part as an attachment, so it will end up as a
+  // MimeUnknown part. Previously, libmime would emit notifications for this to
+  // be treated as an attachment, name Part 1.2. Now it's not the case anymore,
+  // so we should ensure this message has no attachments.
+  {
+    name: 'MimeUnknown attachment (actually, a message)',
+    bodyPart: new SyntheticPartMultiMixed([
+      partHtml,
+      bogusMessage,
+    ]),
+  },
+];
+
+function test_part12_not_an_attachment() {
+  let synMsg = gMessageGenerator.makeMessage(weirdMessageInfos[0]);
+  let synSet = new SyntheticMessageSet([synMsg]);
+  yield add_sets_to_folder(gInbox, [synSet]);
+
+  let msgHdr = synSet.getMsgHdr(0);
+  // dump(synMsg.toMboxString());
+
+  MsgHdrToMimeMessage(msgHdr, null, function(aMsgHdr, aMimeMsg) {
+    try {
+      do_check_true(aMimeMsg.allUserAttachments.length == 0);
+      do_check_true(aMimeMsg.allAttachments.length == 0);
+    } catch (e) {
+      do_throw(e);
+    }
+    async_driver();
+  });
+
+  yield false;
+}
+
 /* ===== Driver ===== */
 
 var tests = [
   parameterizeTest(test_stream_message, messageInfos),
   test_sane_bodies,
   test_attachments_correctness,
+  test_part12_not_an_attachment,
 ];
 
 var gInbox;
 
 function run_test() {
   // use mbox injection because the fake server chokes sometimes right now
   gInbox = configure_message_injection({mode: "local"});
   async_run_tests(tests);
--- a/mailnews/mime/src/mimemoz2.cpp
+++ b/mailnews/mime/src/mimemoz2.cpp
@@ -433,29 +433,34 @@ GenerateAttachmentData(MimeObject *objec
   }
 
   tmp->description = MimeHeaders_get(object->headers, HEADER_CONTENT_DESCRIPTION,
                                      PR_FALSE, PR_FALSE);
 
   // Now, do the right thing with the name!
   if (!tmp->real_name && PL_strcasecmp(tmp->real_type, MESSAGE_RFC822))
   {
+    // Keep in mind that the name was provided by us and this is probably not a
+    // real attachment.
+    tmp->hasFilename = PR_FALSE;
     /* If this attachment doesn't have a name, just give it one... */
     tmp->real_name = MimeGetStringByID(MIME_MSG_DEFAULT_ATTACHMENT_NAME);
     if (tmp->real_name)
     {
       char *newName = PR_smprintf(tmp->real_name, part.get());
       if (newName)
       {
         PR_Free(tmp->real_name);
         tmp->real_name = newName;
       }
     }
     else
       tmp->real_name = mime_part_address(object);
+  } else {
+    tmp->hasFilename = PR_TRUE;
   }
   nsCString urlString(urlSpec);
 
   if (tmp->real_name && !tmp->isExternalAttachment)
   {
     urlString.Append("&filename=");
     nsCAutoString aResult;
     if (NS_SUCCEEDED(MsgEscapeString(nsDependentCString(tmp->real_name),
@@ -525,25 +530,19 @@ BuildAttachmentList(MimeObject *anObject
     else if (! ct)
       // no content type so can't be message body
       skip = PR_FALSE;
     else if (PL_strcasecmp (ct, TEXT_PLAIN) &&
              PL_strcasecmp (ct, TEXT_HTML) &&
              PL_strcasecmp (ct, TEXT_MDL))
       // not a type we recognize as a message body
       skip = PR_FALSE;
-    if (skip)
-    {
-      nsIPrefBranch *prefBranch = GetPrefBranch(child->options);
-      PRInt32 html_as = 0;
-      prefBranch->GetIntPref("mailnews.display.html_as", &html_as);
-      if (html_as == 4)
-        // we're displaying all body parts
+    // we're displaying all body parts
+    if (child->options->html_as_p == 4)
         skip = PR_FALSE;
-    }
     if (skip && child->headers)
     {
       char * disp = MimeHeaders_get (child->headers,
                                      HEADER_CONTENT_DISPOSITION,
                                      PR_TRUE, PR_FALSE);
       if (MimeHeaders_get_name(child->headers, nsnull) &&
           (!disp || PL_strcasecmp(disp, "attachment")))
         // it has a filename and isn't being displayed inline
@@ -677,17 +676,17 @@ NotifyEmittersOfAttachmentList(MimeDispl
   PRInt32     i = 0;
   struct      nsMsgAttachmentData  *tmp = data;
 
   if (!tmp)
     return;
 
   while (tmp->url)
   {
-    if (!tmp->real_name)
+    if (!tmp->real_name || (!tmp->hasFilename && (opt->html_as_p != 4 || opt->metadata_only)))
     {
       ++i;
       ++tmp;
       continue;
     }
 
     nsCAutoString spec;
     if ( tmp->url )
@@ -1671,16 +1670,17 @@ mime_bridge_create_display_stream(
       // We have a bool pref (mail.force_user_charset) to deal with attachments.
       // 1) If true - libmime does NO conversion and just passes it through to raptor
       // 2) If false, then we try to use the charset of the part and if not available,
       //    the charset of the root message
       //
     msd->options->m_prefBranch->GetBoolPref("mail.force_user_charset", &(msd->options->force_user_charset));
     msd->options->m_prefBranch->GetBoolPref("mail.inline_attachments", &(msd->options->show_attachment_inline_p));
     msd->options->m_prefBranch->GetBoolPref("mail.reply_quote_inline", &(msd->options->quote_attachment_inline_p));
+    msd->options->m_prefBranch->GetIntPref("mailnews.display.html_as", &(msd->options->html_as_p));
   }
   /* This pref is written down in with the
      opposite sense of what we like to use... */
   MIME_VariableWidthPlaintext = !MIME_VariableWidthPlaintext;
 
   msd->options->wrap_long_lines_p = MIME_WrapLongLines;
   msd->options->headers = MIME_HeaderType;
 
--- a/mailnews/mime/src/modlmime.h
+++ b/mailnews/mime/src/modlmime.h
@@ -388,16 +388,18 @@ public:
                  missing parts (from IMAP Mime Parts On Demand) */
 
   PRBool show_attachment_inline_p; /* Whether or not we should display attachment inline (whatever say
                          the content-disposition) */
 
   PRBool quote_attachment_inline_p; /* Whether or not we should include inlined attachments in
                          quotes of replies) */
 
+  PRInt32 html_as_p; /* How we should display HTML, which allows us to know if we should display all body parts */
+
   /**
    * Should StartBody/EndBody events be generated for nested MimeMessages.  If
    *  false (the default value), the events are only generated for the outermost
    *  MimeMessage.
    */
   PRBool notify_nested_bodies;
 
   /**