diff --git a/mailnews/base/public/nsIMsgFolder.idl b/mailnews/base/public/nsIMsgFolder.idl index 679a063766..c1fd658fd6 100644 --- a/mailnews/base/public/nsIMsgFolder.idl +++ b/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. diff --git a/mailnews/base/src/nsMsgDBFolder.cpp b/mailnews/base/src/nsMsgDBFolder.cpp index 3c3c5e3528..6c8dab28b9 100644 --- a/mailnews/base/src/nsMsgDBFolder.cpp +++ b/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 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 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; diff --git a/mailnews/imap/src/ImapChannel.sys.mjs b/mailnews/imap/src/ImapChannel.sys.mjs index 775f29e2f4..06928cc0b2 100644 --- a/mailnews/imap/src/ImapChannel.sys.mjs +++ b/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) => { diff --git a/mailnews/imap/src/nsImapProtocol.cpp b/mailnews/imap/src/nsImapProtocol.cpp index aa18929b78..f3a16f7de9 100644 --- a/mailnews/imap/src/nsImapProtocol.cpp +++ b/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 mFolder; + nsMsgKey mMsgKey; + nsCOMPtr mListener; // The listener we're wrapping. + nsCOMPtr 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 imapUrl = do_QueryInterface(m_url); @@ -9432,20 +9513,34 @@ bool nsImapMockChannel::ReadFromLocalCache() { nsCOMPtr 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 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 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 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 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 listener = aListener; nsresult rv = nsContentSecurityManager::doContentSecurityCheck(this, listener); diff --git a/mailnews/imap/test/unit/test_downloadOffline.js b/mailnews/imap/test/unit/test_downloadOffline.js index ad8657bef9..57d084a09b 100644 --- a/mailnews/imap/test/unit/test_downloadOffline.js +++ b/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; + } } );