1513826 - Crash in nsImapService::FetchMimePart with FiltaQuilla add-on

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Wayne Mery (:wsmwk)]

Axel, do you see this in any of the FiltaQuilla reports?

This crash first appears with 60.3.0

This bug was filed from the Socorro interface and is
report bp-7030048d-08d8-473f-b507-7c8c20181107.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll nsImapService::FetchMimePart comm/mailnews/imap/src/nsImapService.cpp:400
1 xul.dll nsMessenger::SaveAttachment comm/mailnews/base/src/nsMessenger.cpp:730
2 xul.dll nsMessenger::DetachAttachmentsWOPrompts comm/mailnews/base/src/nsMessenger.cpp:650
3 xul.dll NS_InvokeByIndex xpcom/reflect/xptcall/md/win32/xptcinvoke_asm_x86_msvc.asm:54
4 xul.dll XPCWrappedNative::CallMethod js/xpconnect/src/XPCWrappedNative.cpp:1234
5 xul.dll js::detail::CopyScript js/src/vm/JSScript.cpp:3661
6 xul.dll JS::GCVector<JS::Value, 8, js::TempAllocPolicy>::trace js/public/GCVector.h:136
7 xul.dll XPC_WN_CallMethod js/xpconnect/src/XPCWrappedNativeJSOps.cpp:913
8 xul.dll XPC_WN_CallMethod js/xpconnect/src/XPCWrappedNativeJSOps.cpp:913
9 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:468

=============================================================

bp-5803845-5979-4b24-9e08-f97c30181203
bp-201ca0b2-7fdf-412b-89b1-bcca20181127

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1513826-fix-crash.patch

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9084569 [details] [diff] [review]
1513826-fix-crash.patch

Review of attachment 9084569 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, r=mkmelin

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 9084569 [details] [diff] [review]
1513826-fix-crash.patch

Pretty rare crash, some add-on must be calling this with a "bad" URL.

Comment 4

[Axel Grude]

(In reply to Jorg K (GMT+2) from comment #3)

Comment on attachment 9084569 [details] [diff] [review]
1513826-fix-crash.patch

Pretty rare crash, some add-on must be calling this with a "bad" URL.

Interesting, I am getting quite a few reports on this, how would the following code generate bad URLs? Are you talking about bad folder paths?

{
this.msgURI = aMsgHdr.folder.generateMessageURI(aMsgHdr.messageKey);
this.attachments = aMimeMessage.allAttachments;
let messenger = Cc["@mozilla.org/messenger;1"].createInstance(Ci.nsIMessenger);

if (this.attachments.length > 0) {
  let msgURIs = [],
      contentTypes = [],
      urls = [],
      displayNames = [];
  for (let j = 0; j < this.attachments.length; j++) {
    let attachment = this.attachments[j];
    msgURIs.push(this.msgURI);
    contentTypes.push(attachment.contentType);
    urls.push(attachment.url);
    displayNames.push(attachment.name);
    util.logDebug("Detach attachment [" + j + "] to " + uniqueFile.path + 
        "...\n msgURI=" + this.msgURI + 
        "\n att.url=" + attachment.url +
        "\n att.ncontentType=" + attachment.contentType)
    
  }
  messenger.detachAttachmentsWOPrompts(this.directory, this.attachments.length,
                          contentTypes, urls, displayNames, msgURIs, null);
}

}

Comment 5

[Jorg K (CEST = GMT+2)]

I can't see how the code you pasted into comment #4 is relevant. It doesn't show a call to nsImapService::FetchMimePart() so I can't see which URL you're passing in.

In general, the nsIURI passed into the function is QI'ed to nsIMsgMailNewsUrl, so if it doesn't implement that interface, it will crash. With the patch, it will return an error. So if you call that function from your add-on(s), the least you can do is to dump out the .spec of the nsIURI before the call so we can see what gets passed in there. Surely something link file: or http(s): URLs are not nsIMsgMailNewsUrls.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/67b687a5aad6
Don't crash in nsImapService::FetchMimePart() if URL passed in can't be QI'ed. r=mkmelin

Comment 7

[Axel Grude]

(In reply to Jorg K (GMT+2) from comment #5)

I can't see how the code you pasted into comment #4 is relevant. It doesn't show a call to nsImapService::FetchMimePart() so I can't see which URL you're passing in.

In general, the nsIURI passed into the function is QI'ed to nsIMsgMailNewsUrl, so if it doesn't implement that interface, it will crash. With the patch, it will return an error. So if you call that function from your add-on(s), the least you can do is to dump out the .spec of the nsIURI before the call so we can see what gets passed in there. Surely something link file: or http(s): URLs are not nsIMsgMailNewsUrls.

It calls messenger.detachAttachmentsWOPrompts, which is part of the original stack trace:

0 xul.dll nsImapService::FetchMimePart comm/mailnews/imap/src/nsImapService.cpp:400
1 xul.dll nsMessenger::SaveAttachment comm/mailnews/base/src/nsMessenger.cpp:730
2 xul.dll nsMessenger::DetachAttachmentsWOPrompts comm/mailnews/base/src/nsMessenger.cpp:650

Comment 8

[Jorg K (CEST = GMT+2)]

Unrelated: The NNTP and mailbox equivalents QI to nsIMsgMailNewsUrl straight away and return an error if unsuccessful:
https://searchfox.org/comm-central/rev/99e635c4517ff1689d25f01b41f0753160abf7ac/mailnews/local/src/nsMailboxService.cpp#235
https://searchfox.org/comm-central/rev/99e635c4517ff1689d25f01b41f0753160abf7ac/mailnews/news/src/nsNntpService.cpp#378
So this is the right fix.

Looking at your call stack, the aURI parameter of nsImapService::FetchMimePart() is a URI produced from the const nsACString &aURL, parameter passed into nsMessenger::SaveAttachment() (nsMessenger.cpp:730) and that comes from the first of the URL strings passed into nsMessenger::DetachAttachmentsWOPrompts() (nsMessenger.cpp:650).

So in your code that would be the urls variable which comes from attachment.url. So I guess that will fail big time if the attachment was detached, since than this will be a file: URL or it was a so-called "external" attachment (mainly used in feeds) which is a http(s): URL.

Dump out your urls variable.

Comment 9

[Alex Ihrig [:Thunderbird_Mail_DE]]

I'm wondering, if this crash could be related to my still existing problems and trials in upgrading the AttachmentsExtractor extension to "AttachmentExtractor Continued". In my tests Thunderbird crashes dependent on my try and error trials to fix a function to fetchMimeParts.

lines 433 and following:
https://gitlab.com/ThunderbirdMailDE/attachment-extractor-continued/blob/2.0a1-20190810/chrome/content/aec_js_messenger.js#L433

Comment 10

[Jorg K (CEST = GMT+2)]

TB 69 beta 3:
https://hg.mozilla.org/releases/comm-beta/rev/4081e478928cf08b3eecda59423387a6eefd86eb

Comment 11

[Jorg K (CEST = GMT+2)]

TB 68.0 ESR:
https://hg.mozilla.org/releases/comm-esr68/rev/823285a5b6f96b40545926b9da999dc5b45f22af

Comment 12

[Jorg K (CEST = GMT+2)]

TB 60.9 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/448d166534091217fbc9e887025eceec12eefee4

Comment 13

[Axel Grude]

TB 60.9 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/448d166534091217fbc9e887025eceec12eefee4
great stuff - this will throw for a proper stack trace? I will add the URL parameter to my catch.

Comment 14

[Jorg K (CEST = GMT+2)]

This will stop the crash. The function will return an error and it's likely that you'll see that as a throw in JS code.