Bug 1579341 - Don't fetch if parts_on_demand or URL with credentials. r=mkmelin draft
authoralta88@fixall.com
Wed, 18 Sep 2019 19:58:17 +0200
changeset 78974 b338718abf278ada3794d19010dffb04ab765909
parent 78956 d109b7903efb4637d877845579fc8501b132e3d3
child 78975 3ec81acc9d2ab321cd28a8a5775b289971b46a6e
push id9325
push usermozilla@jorgk.com
push dateWed, 18 Sep 2019 17:58:50 +0000
treeherdertry-comm-central@3ec81acc9d2a [default view] [failures only]
reviewersmkmelin
bugs1579341
Bug 1579341 - Don't fetch if parts_on_demand or URL with credentials. r=mkmelin
mail/base/content/msgHdrView.js
--- a/mail/base/content/msgHdrView.js
+++ b/mail/base/content/msgHdrView.js
@@ -686,32 +686,31 @@ var messageHeaderSink = {
         // remains false.
         if (isNaN(size) || size.toString().length != value.length || size < 2) {
           last.size = -1;
         } else if (size > Number.MAX_SAFE_INTEGER) {
           last.size = Number.MAX_SAFE_INTEGER;
         } else {
           last.size = size;
         }
-      } else if (size == -1) {
-        // libmime returns -1 if it never managed to figure out the size. We
-        // want fetch() to check again for internal attachments in this case,
-        // otherwise we accept libmime's value and set |sizeResolved|.
-        // A fetch() for a resolved size can still be requested if
-        // |ALWAYSFETCHSIZE| is true.
-        last.size = -1;
       } else {
+        // For internal or file (detached) attachments, save the size.
         last.size = size;
-        last.sizeResolved = true;
+        // For external file attachments, we won't have a valid size.
+        if (!last.isFileAttachment && size > -1) {
+          last.sizeResolved = true;
+        }
       }
     } else if (field == "X-Mozilla-PartDownloaded" && value == "0") {
       // We haven't downloaded the attachment, so any size we get from
       // libmime is almost certainly inaccurate. Just get rid of it. (Note:
       // this relies on the fact that PartDownloaded comes after PartSize from
       // the MIME emitter.)
+      // Note: for imap parts_on_demand, a small size consisting of the part
+      // headers would have been returned above.
       last.size = -1;
       last.sizeResolved = false;
     }
   },
 
   onEndAllAttachments() {
     displayAttachmentsForExpandedView();
 
@@ -1907,26 +1906,23 @@ function CopyNewsgroupURL(newsgroupNode)
 function AttachmentInfo(contentType, url, name, uri, isExternalAttachment) {
   this.message = gFolderDisplay.selectedMessage;
   this.contentType = contentType;
   this.name = name;
   this.url = url;
   this.uri = uri;
   this.isExternalAttachment = isExternalAttachment;
   // A |size| value of -1 means we don't have a valid size. Check again if
-  // |sizeResolved| is false or ALWAYSFETCHSIZE is true. For internal attachments
-  // and link attachments with a reported size, libmime streams values to
-  // addAttachmentField() which updates this object. For external file
-  // attachments and unresolved -1 size internal attachments, |size| is updated
+  // |sizeResolved| is false. For internal attachments and link attachments
+  // with a reported size, libmime streams values to addAttachmentField()
+  // which updates this object. For external file attachments, |size| is updated
   // in the isEmpty() function when the list is built. Deleted attachments
   // are resolved to -1.
   this.size = -1;
   this.sizeResolved = this.isDeleted;
-  // Fetch the size for internal attachments even though libmime gave a size.
-  this.ALWAYSFETCHSIZE = false;
 
   // Remove [?&]part= from remote urls, after getting the partID.
   // Remote urls, unlike non external mail part urls, may also contain query
   // strings starting with ?; PART_RE does not handle this.
   if (this.isLinkAttachment || this.isFileAttachment) {
     let match = url.match(/[?&]part=[^&]+$/);
     match = match && match[0];
     this.partID = match && match.split("part=")[1];
@@ -2069,49 +2065,37 @@ AttachmentInfo.prototype = {
    *
    * @returns true if the attachment is empty or error, false otherwise.
    */
   async isEmpty() {
     if (this.isDeleted) {
       return true;
     }
 
+    const isFetchable = url => {
+      let uri = makeURI(url);
+      return !(uri.username || uri.userPass);
+    };
+
     // We have a resolved size.
-    if (this.sizeResolved && !this.ALWAYSFETCHSIZE) {
-      if (this.size > 0) {
-        return false;
-      }
-      return true;
+    if (this.sizeResolved) {
+      return this.size < 1;
     }
 
-    let url = this.url;
+    if (!isFetchable(this.url)) {
+      return false;
+    }
+
     let empty = true;
     let size = 0;
-    let options = { method: "HEAD" };
-
-    // NOTE: For internal mailbox, imap, news urls the response body must get
-    // the content length with getReader().read() but we don't need to do this
-    // here as libmime streams it already in addAttachmentField(). For imap or
-    // news urls with credentials (username, userPass), we must remove them
-    // as Request fails such urls with a MSG_URL_HAS_CREDENTIALS error.
-    if (url.startsWith("imap://") || url.startsWith("news://")) {
-      url = makeURI(url)
-        .mutate()
-        .setUsername("")
-        .setUserPass("")
-        .finalize().spec;
-    }
-
-    let request = new Request(url, options);
-
-    if (
-      this.isExternalAttachment ||
-      !this.sizeResolved ||
-      this.ALWAYSFETCHSIZE
-    ) {
+    let options = { method: "GET" };
+
+    let request = new Request(this.url, options);
+
+    if (this.isExternalAttachment) {
       updateAttachmentsDisplay(this, true);
     }
 
     await fetch(request)
       .then(response => {
         if (!response.ok) {
           console.warn(
             "AttachmentInfo.isEmpty: fetch response error - " +
@@ -2133,50 +2117,49 @@ AttachmentInfo.prototype = {
             return null;
           }
         }
 
         return response;
       })
       .then(async response => {
         if (this.isExternalAttachment) {
-          size = response ? response.headers.get("content-length") : 0;
-        } else if (!this.sizeResolved || this.ALWAYSFETCHSIZE) {
+          size = response ? response.headers.get("content-length") : -1;
+        } else {
           // Check the attachment again if addAttachmentField() sets a
           // libmime -1 return value for size in this object.
-          let data = await response.body.getReader().read();
-          size = data.value.length;
-        } else {
-          // Accept the libmime streamed size for internal attachments.
-          size = this.size;
+          // Note: just test for a non zero size, don't need to drain the
+          // stream. We only get here if the url is fetchable.
+          let reader = response.body.getReader();
+          let { value: chunk } = await reader.read();
+          reader.cancel();
+          size = chunk && chunk.value ? chunk.value.length : -1;
         }
 
         if (size > 0) {
           empty = false;
         }
       })
       .catch(error => {
-        console.warn("AttachmentInfo.isEmpty: error - " + error.message);
+        console.warn(
+          `AttachmentInfo.isEmpty: ${error.message} url - ${this.url}`
+        );
+        size = -1;
       });
 
-    if (
-      this.isExternalAttachment ||
-      !this.sizeResolved ||
-      this.ALWAYSFETCHSIZE
-    ) {
+    if (this.isExternalAttachment) {
       // For link attachments, we may have had a published value or -1
       // indicating unknown value. We now know the real size, so set it and
       // update the ui. For detached file attachments, get the size here
-      // instead of the old xpcom way. Finally, also update libmime unknown
-      // internal attachment size.
-      this.size = empty ? -1 : size;
-      this.sizeResolved = true;
+      // instead of the old xpcom way.
+      this.size = size;
       updateAttachmentsDisplay(this, false);
     }
 
+    this.sizeResolved = true;
     return empty;
   },
 
   /**
    * Open a file attachment's containing folder.
    */
   openFolder() {
     if (!this.isFileAttachment || !this.hasFile) {
@@ -2540,23 +2523,19 @@ async function displayAttachmentsForExpa
 
     for (let attachment of currentAttachments) {
       // Create a new attachment widget
       var displayName = SanitizeAttachmentDisplayName(attachment);
       var item = attachmentList.appendItem(attachment, displayName);
       item.setAttribute("tooltiptext", attachment.name);
       item.addEventListener("command", attachmentItemCommand);
 
-      // Get a detached file's size or an internal attachment size that is not
-      // yet resolved, or if |ALWAYSFETCHSIZE| is true. For link attachments,
-      // the user must always initiate the fetch for privacy reasons.
-      if (
-        !attachment.isLinkAttachment &&
-        (!attachment.sizeResolved || attachment.ALWAYSFETCHSIZE)
-      ) {
+      // Get a detached file's size. For link attachments, the user must always
+      // initiate the fetch for privacy reasons.
+      if (attachment.isFileAttachment) {
         await attachment.isEmpty();
       }
     }
 
     if (
       Services.prefs.getBoolPref("mailnews.attachments.display.start_expanded")
     ) {
       toggleAttachmentList(true);
@@ -2725,17 +2704,17 @@ function updateSaveAllAttachmentsButton(
 /**
  * Update the attachments display info after a particular attachment's
  * existence has been verified.
  *
  * @param {AttachmentInfo} attachmentInfo
  * @param {Boolean} isFetching
  */
 function updateAttachmentsDisplay(attachmentInfo, isFetching) {
-  if (attachmentInfo.isExternalAttachment || attachmentInfo.ALWAYSFETCHSIZE) {
+  if (attachmentInfo.isExternalAttachment) {
     let attachmentList = document.getElementById("attachmentList");
     let attachmentIcon = document.getElementById("attachmentIcon");
     let attachmentName = document.getElementById("attachmentName");
     let attachmentSize = document.getElementById("attachmentSize");
     let attachmentItem = attachmentList.findItemForAttachment(attachmentInfo);
     let index = attachmentList.getIndexOfItem(attachmentItem);
 
     if (isFetching) {