Bug 1929105 - Respond to IMAP offline message read errors by discarding local copy. r=kaie

Differential Revision: https://phabricator.services.mozilla.com/D228621

--HG--
extra : amend_source : 831f998690216a1026a56ccbaca09e62c1dd66d1

mailnews/base/public/nsIMsgFolder.idl

@@ -623,6 +623,11 @@ interface nsIMsgFolder : nsISupports {
   readonly attribute boolean supportsOffline;
   boolean shouldStoreMsgOffline(in nsMsgKey msgKey);
   boolean hasMsgOffline(in nsMsgKey msgKey);
+  /**
+   * Discard the msgStore offline copy of this message (if there is one).
+   * If there is no offline copy, this has no effect.
+   */
+  void discardOfflineMsg(in nsMsgKey msgKey);
 
   /**
    * Get an offline store output stream for the passed message header.

mailnews/base/src/nsMsgDBFolder.cpp

@@ -1082,6 +1082,57 @@ NS_IMETHODIMP nsMsgDBFolder::HasMsgOffline(nsMsgKey msgKey, bool* result) {
   return NS_OK;
 }
 
+NS_IMETHODIMP nsMsgDBFolder::DiscardOfflineMsg(nsMsgKey msgKey) {
+  GetDatabase();
+  if (!mDatabase) return NS_ERROR_FAILURE;
+
+  nsresult rv;
+  RefPtr<nsIMsgDBHdr> hdr;
+  rv = mDatabase->GetMsgHdrForKey(msgKey, getter_AddRefs(hdr));
+  if (NS_FAILED(rv)) return rv;
+  if (!hdr) {
+    return NS_ERROR_FAILURE;
+  }
+
+  // Tell the msgStore to ditch its local copy of the message.
+  // For maildir this is easy (just delete the file).
+  // But mbox doesn't delete anything, it just attempts to set the
+  // `Expunged` flag by rewriting X-Mozilla-* headers in-place,
+  // relying on a compaction later to actually remove the message.
+  //
+  // But it's likely the reason we're calling DiscardOfflineMsg() is because
+  // we suspect the storeToken is wrong, or the message is corrupt in some
+  // other way.
+  //
+  // In that case, attempting to edit the message headers is just
+  // going to be a world of pain.
+  //
+  // Maybe one day we can get rid of the X-Mozilla-* rewriting,
+  // and then the mbox DeleteMessages() can just be a nice clean
+  // no-op... but for now we're just going to bodge it and skip this step
+  // for mbox.
+  nsCOMPtr<nsIMsgPluggableStore> msgStore;
+  rv = GetMsgStore(getter_AddRefs(msgStore));
+  if (NS_SUCCEEDED(rv)) {
+    nsAutoCString t;
+    msgStore->GetStoreType(t);
+    if (!t.EqualsLiteral("mbox")) {
+      // Ignore failure - no useful recovery and better to keep going.
+      msgStore->DeleteMessages({hdr});
+    }
+  }
+
+  // Detach the database entry from the offline message.
+  // mDatabase->markOffline() should really also clear storeToken and
+  // size, but see Bug 1931217.
+  hdr->SetStoreToken(EmptyCString());
+  hdr->SetOfflineMessageSize(0);
+  mDatabase->MarkOffline(msgKey, false, this);
+
+  mDatabase->Commit(nsMsgDBCommitType::kLargeCommit);
+  return NS_OK;
+}
+
 NS_IMETHODIMP nsMsgDBFolder::GetFlags(uint32_t* _retval) {
   ReadDBFolderInfo(false);
   *_retval = mFlags;

mailnews/imap/src/ImapChannel.sys.mjs

@@ -222,7 +222,7 @@ export class ImapChannel extends MailChannel {
 
     const hdr = this.URI.folder.GetMessageHeader(this._msgKey);
     const stream = this.URI.folder.getLocalMsgStream(hdr);
-    this._readFromCacheStream(stream);
+    this._readFromCacheStream(stream, hdr);
     return true;
   }
 
@@ -230,8 +230,10 @@ export class ImapChannel extends MailChannel {
    * Read the message from the a stream.
    *
    * @param {nsIInputStream} cacheStream - The input stream to read.
+   * @param {nsIMsgDBHdr} offlineHdr     - If streaming a message from
+   *                                       msgStore, this is its header.
    */
-  _readFromCacheStream(cacheStream) {
+  _readFromCacheStream(cacheStream, offlineHdr) {
     const pump = Cc["@mozilla.org/network/input-stream-pump;1"].createInstance(
       Ci.nsIInputStreamPump
     );
@@ -249,6 +251,13 @@ export class ImapChannel extends MailChannel {
         try {
           this.loadGroup?.removeRequest(this, null, Cr.NS_OK);
         } catch (e) {}
+        if (status != Cr.NS_OK) {
+          // If we're streaming an offline message, and it failed, discard
+          // the local copy on grounds that it's probably damaged.
+          if (offlineHdr) {
+            offlineHdr.folder.discardOfflineMsg(offlineHdr.messageKey);
+          }
+        }
         this._pending = false;
       },
       onDataAvailable: (request, stream, offset, count) => {

mailnews/imap/src/nsImapProtocol.cpp

@@ -8694,6 +8694,7 @@ nsresult nsImapCacheStreamListener::Init(nsIStreamListener* aStreamListener,
   NS_ENSURE_ARG(aStreamListener);
   NS_ENSURE_ARG(aMockChannelToUse);
 
+  MOZ_ASSERT(NS_IsMainThread());
   mChannelToUse = aMockChannelToUse;
   mListener = aStreamListener;
   mCache2 = aCache2;
@@ -8704,6 +8705,7 @@ nsresult nsImapCacheStreamListener::Init(nsIStreamListener* aStreamListener,
 
 NS_IMETHODIMP
 nsImapCacheStreamListener::OnStartRequest(nsIRequest* request) {
+  MOZ_ASSERT(NS_IsMainThread());
   if (!mChannelToUse) {
     NS_ERROR("OnStartRequest called after OnStopRequest");
     return NS_ERROR_NULL_POINTER;
@@ -8717,6 +8719,7 @@ nsImapCacheStreamListener::OnStartRequest(nsIRequest* request) {
 NS_IMETHODIMP
 nsImapCacheStreamListener::OnStopRequest(nsIRequest* request,
                                          nsresult aStatus) {
+  MOZ_ASSERT(NS_IsMainThread());
   if (!mListener) {
     NS_ERROR("OnStopRequest called twice");
     return NS_ERROR_NULL_POINTER;
@@ -8763,6 +8766,7 @@ nsImapCacheStreamListener::OnDataAvailable(nsIRequest* request,
                                            nsIInputStream* aInStream,
                                            uint64_t aSourceOffset,
                                            uint32_t aCount) {
+  MOZ_ASSERT(NS_IsMainThread());
   if (mCache2 && mStarting) {
     // Peeker() does check of leading bytes and sets mGoodCache2.
     uint32_t numRead;
@@ -8787,6 +8791,82 @@ nsImapCacheStreamListener::OnDataAvailable(nsIRequest* request,
                                     aCount);
 }
 
+//
+// ImapOfflineMsgStreamListener
+//
+// Listener wrapper, helper for ReadFromLocalCache().
+// It knows which offline message it's streaming out, and if the operation
+// fails it'll cause the local copy to be discarded (on the grounds that it's
+// likely damaged) before letting the underlying listener proceed with it's
+// own error handling.
+// Works on the Main thread.
+//
+// ***NOTE*** (BenC 2024-11-12):
+// We pass in the underlying channel to use as the request param to the
+// underlying listener callbacks. I'm not totally sure this is required.
+// It'd be nicer to just pass along whatever request that we're called
+// with.
+// But nsImapCacheStreamListener (which this is based upon) did it, so I'm
+// cargo-culting it. For now.
+//
+class ImapOfflineMsgStreamListener : public nsIStreamListener {
+ public:
+  NS_DECL_ISUPPORTS
+
+  ImapOfflineMsgStreamListener() = delete;
+  ImapOfflineMsgStreamListener(nsIMsgFolder* folder, nsMsgKey msgKey,
+                               nsIStreamListener* listener,
+                               nsIImapMockChannel* channel)
+      : mFolder(folder),
+        mMsgKey(msgKey),
+        mListener(listener),
+        mChannel(channel) {
+    MOZ_ASSERT(mFolder);
+    MOZ_ASSERT(mListener);
+    MOZ_ASSERT(mChannel);
+  }
+
+  NS_IMETHOD OnStartRequest(nsIRequest* request) override {
+    MOZ_ASSERT(NS_IsMainThread());
+    return mListener->OnStartRequest(mChannel);
+  }
+
+  NS_IMETHOD OnDataAvailable(nsIRequest* request, nsIInputStream* stream,
+                             uint64_t offset, uint32_t count) override {
+    MOZ_ASSERT(NS_IsMainThread());
+    return mListener->OnDataAvailable(mChannel, stream, offset, count);
+  }
+
+  NS_IMETHOD OnStopRequest(nsIRequest* request, nsresult status) override {
+    MOZ_ASSERT(NS_IsMainThread());
+    nsresult rv = mListener->OnStopRequest(mChannel, status);
+    mListener = nullptr;
+    mChannel->Close();
+    mChannel = nullptr;
+    if (NS_FAILED(status)) {
+      // The streaming failed, discard the offline copy of the message.
+      mFolder->DiscardOfflineMsg(mMsgKey);
+    }
+    return rv;
+  }
+
+ protected:
+  virtual ~ImapOfflineMsgStreamListener() {}
+  // Remember the folder and key of the offline message we're streaming out,
+  // so if anything goes wrong we can discard it on the grounds that it's
+  // damaged.
+  nsCOMPtr<nsIMsgFolder> mFolder;
+  nsMsgKey mMsgKey;
+  nsCOMPtr<nsIStreamListener> mListener;  // The listener we're wrapping.
+  nsCOMPtr<nsIImapMockChannel> mChannel;
+};
+
+NS_IMPL_ISUPPORTS(ImapOfflineMsgStreamListener, nsIStreamListener);
+
+//
+// nsImapMockChannel implementation
+//
+
 NS_IMPL_ISUPPORTS_INHERITED(nsImapMockChannel, nsHashPropertyBag,
                             nsIImapMockChannel, nsIMailChannel, nsIChannel,
                             nsIRequest, nsICacheEntryOpenCallback,
@@ -9405,6 +9485,7 @@ NS_IMETHODIMP nsImapMockChannel::ReadFromImapConnection() {
 // that... If it's in the local cache, we return true and we can abort the
 // download because this method does the rest of the work.
 bool nsImapMockChannel::ReadFromLocalCache() {
+  MOZ_ASSERT(NS_IsMainThread());
   nsresult rv = NS_OK;
 
   nsCOMPtr<nsIImapUrl> imapUrl = do_QueryInterface(m_url);
@@ -9432,20 +9513,34 @@ bool nsImapMockChannel::ReadFromLocalCache() {
   nsCOMPtr<nsIMsgDBHdr> hdr;
   rv = folder->GetMessageHeader(msgKey, getter_AddRefs(hdr));
   NS_ENSURE_SUCCESS(rv, false);
+
+  // Attempt to open the local message and pump it out asynchronously.
+  // If any of this fails we assume the local message is damaged.
+  // In that case we'll discard it and tell the caller there is no local
+  // copy.
   nsCOMPtr<nsIInputStream> msgStream;
   rv = folder->GetLocalMsgStream(hdr, getter_AddRefs(msgStream));
   NS_ENSURE_SUCCESS(rv, false);
-  // dougt - This may break the ablity to "cancel" a read from offline
-  // mail reading. fileChannel->SetLoadGroup(m_loadGroup);
-  RefPtr<nsImapCacheStreamListener> cacheListener =
-      new nsImapCacheStreamListener();
-  cacheListener->Init(m_channelListener, this);
 
-  // create a stream pump that will async read the message.
+  // Create a stream pump that will async read the message.
   nsCOMPtr<nsIInputStreamPump> pump;
   rv = NS_NewInputStreamPump(getter_AddRefs(pump), msgStream.forget());
   NS_ENSURE_SUCCESS(rv, false);
-  rv = pump->AsyncRead(cacheListener);
+
+  // Wrap the listener with another one which knows which message offline
+  // message we're reading from. If the read fails, our wrapper will discard
+  // the offline copy as damaged.
+  // We use a wrapper around the real listener because we don't know who is
+  // consuming this data (A docshell, gloda indexing, calendar, whatever), so
+  // we don't have any control over how read errors are handled. This lets
+  // us intercept errors in OnStopRequest() and respond by discarding the
+  // offline copy on the grounds that it's likely damaged.
+  // Then the underlying listener can proceed with it's own OnStopRequest()
+  // handling.
+  RefPtr<ImapOfflineMsgStreamListener> offlineMsgListener =
+      new ImapOfflineMsgStreamListener(folder, msgKey, m_channelListener, this);
+
+  rv = pump->AsyncRead(offlineMsgListener);
   NS_ENSURE_SUCCESS(rv, false);
 
   // if the msg is unread, we should mark it read on the server. This lets
@@ -9455,6 +9550,8 @@ bool nsImapMockChannel::ReadFromLocalCache() {
 }
 
 NS_IMETHODIMP nsImapMockChannel::AsyncOpen(nsIStreamListener* aListener) {
+  MOZ_ASSERT(NS_IsMainThread(),
+             "nsIChannel methods must be called from main thread");
   nsCOMPtr<nsIStreamListener> listener = aListener;
   nsresult rv =
       nsContentSecurityManager::doContentSecurityCheck(this, listener);

mailnews/imap/test/unit/test_downloadOffline.js

@@ -73,6 +73,11 @@ add_task(
 
     // Corrupt the storeTokens by adding 3.
     for (const msg of inbox.messages) {
+      Assert.equal(
+        true,
+        inbox.hasMsgOffline(msg.messageKey),
+        "Messages should be marked Offline."
+      );
       const offset = Number(msg.storeToken) + 3;
       msg.storeToken = offset.toString();
     }
@@ -84,7 +89,15 @@ add_task(
       const uri = inbox.getUriForMsg(msg);
       const service = MailServices.messageServiceFromURI(uri);
       try {
-        service.streamMessage(uri, streamListener, null, null, false, "", true);
+        service.streamMessage(
+          uri,
+          streamListener,
+          null,
+          null,
+          false,
+          "",
+          false // localOnly
+        );
         await streamListener.promise;
         Assert.ok(false, "Bad storeToken should cause error.");
       } catch (e) {
@@ -95,5 +108,53 @@ add_task(
         );
       }
     }
+
+    // Make sure that the offline messages were discarded.
+    for (const msg of inbox.messages) {
+      Assert.equal(
+        false,
+        inbox.hasMsgOffline(msg.messageKey),
+        "Bad message should not be marked Offline."
+      );
+      Assert.equal(
+        msg.storeToken,
+        "",
+        "Bad message should have had their storeToken cleared."
+      );
+      Assert.equal(
+        msg.flags & Ci.nsMsgMessageFlags.Offline,
+        0,
+        "Bad message should have had their Offline flag cleared."
+      );
+      Assert.equal(
+        msg.offlineMessageSize,
+        0,
+        "Bad message should have had their .offlineMessageSize zeroed."
+      );
+    }
+
+    // Stream them again from the server.
+    for (const msg of inbox.messages) {
+      const streamListener = new PromiseTestUtils.PromiseStreamListener();
+      const uri = inbox.getUriForMsg(msg);
+      const service = MailServices.messageServiceFromURI(uri);
+      service.streamMessage(uri, streamListener, null, null, false, "", false);
+      await streamListener.promise;
+    }
+
+    // The offline copies should have been re-downloaded.
+    for (const msg of inbox.messages) {
+      Assert.equal(
+        true,
+        inbox.hasMsgOffline(msg.messageKey),
+        "Message should again be available Offline."
+      );
+      const streamListener = new PromiseTestUtils.PromiseStreamListener();
+      const uri = inbox.getUriForMsg(msg);
+      const service = MailServices.messageServiceFromURI(uri);
+      // One more time, but with localOnly set.
+      service.streamMessage(uri, streamListener, null, null, false, "", true);
+      await streamListener.promise;
+    }
   }
 );