1335638 - Crash in [thunk]:nsIImapMailFolderSink::`vcall''{56, {flat}}'' }'' via `anonymous namespace'::SyncRunnable1

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Wayne Mery (:wsmwk)]

Like bug 1335578, perhaps fallout from an uplifted patch?

Note, checking ~10 users who report with an email address, none have crashed in the release prior. So this is a new crash.  Also, most only see this signature, but a few also crash with the signature of bug 1335578 [thunk]:mozilla::storage::Connection::`vcall''{100, {flat}}'' }''   (51.0b2 has some crashes ... I did not check for any correlation between those crashes and the other releases)
#2 crash for 45.7.0 ***
#3 crash for 52.0b1
#8 crash for 51.0b2

the most common comments are viewing, forwarding, deleting email

bp-c7d35878-640c-4ad8-a8e5-1d8c62170130
0 	xul.dll	[thunk]:nsIImapMailFolderSink::`vcall'{56, {flat}}' }'	
1 	xul.dll	`anonymous namespace'::SyncRunnable1<nsIImapServerSink, unsigned __int64>::Run()	c:/builds/moz2_slave/tb-rel-c-esr45-w32_bld-0000000/build/mailnews/imap/src/nsSyncRunnableHelpers.cpp:118
2 	xul.dll	nsThread::ProcessNextEvent(bool, bool*)	xpcom/threads/nsThread.cpp:972
3 	xul.dll	NS_ProcessNextEvent(nsIThread*, bool)	xpcom/glue/nsThreadUtils.cpp:297
4 	xul.dll	MessageLoop::RunHandler()	ipc/chromium/src/base/message_loop.cc:227
5 	xul.dll	nsBaseAppShell::Run()	widget/nsBaseAppShell.cpp:156
6 	xul.dll	nsAppShell::Run()	widget/windows/nsAppShell.cpp:257

Comment 1

[Wayne Mery (:wsmwk)]

51.0b1 had only a smallish number of crashes [1]. only a few per day - not sure what to make of that low rate in comparison to 51.0b2.

FWIW, earliest is bp-c20d7012-2c89-4d5c-b912-ab8482170113 dated 2017-01-13 which predates the release => some of our testers crashed https://www.mozilla.org/en-US/thunderbird/51.0beta/releasenotes/

[1] https://crash-stats.mozilla.com/signature/?version=51.0b1&signature=%5Bthunk%5D%3AnsIImapMailFolderSink%3A%3A%60vcall%27%27%7B56%2C%20%7Bflat%7D%7D%27%27%20%7D%27%27&date=%3E%3D2016-11-01T21%3A17%3A21.000Z&date=%3C2017-02-01T21%3A17%3A21.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_sort=-date&page=1#reports

Comment 3

[Razvan]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20170125094131

Steps to reproduce:

Tried to open an inline forwarded email. The forward was made from a Mac version of TB (User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:45.0) Gecko/20100101 Thunderbird/45.6.0) and originated from an Outlook client (<meta name="Generator" content="Microsoft Word 14 (filtered medium)>).

After deleting the src="imap://...." part of the embedded picture, the message was rendered successfully.

My TB client is: User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.0




Actual results:

TB crashed without displaying anything.


Expected results:

Render the message.

Comment 4

[Razvan]

Something happened between 45.6 and 45.7 because it works on 45.6 but 45.7 does crash.

Comment 5

[Jorg K (CEST = GMT+2)]

We made very few changes between 45.6 and 45.7:
https://hg.mozilla.org/releases/comm-esr45/pushloghtml?fromchange=2d3f730dbc9a&tochange=5b85a38d67c3

The only one related to IMAP is bug 1328814:
https://hg.mozilla.org/releases/comm-esr45/rev/ccb801cd360e

I suggested that Kent do some investigation, see bug 1332606 comment #5.

Comment 6

[Wayne Mery (:wsmwk)]

(In reply to Razvan from comment #4)
> Something happened between 45.6 and 45.7 because it works on 45.6 but 45.7
> does crash.

Razvan, please try  https://archive.mozilla.org/pub/thunderbird/nightly/2017/02/2017-02-04-03-02-04-comm-central/thunderbird-54.0a1.en-US.win32.installer.exe and post your results, and any crash IDs

Comment 7

[Razvan]

Unfortunately it still crashes.
Crash id: bp-6e4642c3-b3bc-441c-915d-941202170205

(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #6)
> (In reply to Razvan from comment #4)
> > Something happened between 45.6 and 45.7 because it works on 45.6 but 45.7
> > does crash.
> 
> Razvan, please try 
> https://archive.mozilla.org/pub/thunderbird/nightly/2017/02/2017-02-04-03-02-
> 04-comm-central/thunderbird-54.0a1.en-US.win32.installer.exe and post your
> results, and any crash IDs

Comment 8

[Kent James (:rkent)]

Razva, could you try the latest nightly https://archive.mozilla.org/pub/thunderbird/nightly/2017/02/2017-02-04-03-02-04-comm-central/thunderbird-54.0a1.en-US.win32.installer.exe 

Ideally this will also crash for you, but it will crash in a different place that will give us more idea of the root cause, and hos to prevent it. If it crashes make sure you report the crash ID.

Comment 9

[Razvan]

I think that this version is the same as the version suggested in comment #6. In comment #7 is the crash id for that. (bp-6e4642c3-b3bc-441c-915d-941202170205)

(In reply to Kent James (:rkent) from comment #8)
> Razva, could you try the latest nightly
> https://archive.mozilla.org/pub/thunderbird/nightly/2017/02/2017-02-04-03-02-
> 04-comm-central/thunderbird-54.0a1.en-US.win32.installer.exe 
> 
> Ideally this will also crash for you, but it will crash in a different place
> that will give us more idea of the root cause, and hos to prevent it. If it
> crashes make sure you report the crash ID.

Comment 10

[Jorg K (CEST = GMT+2)]

Razvan, many thanks!! Yes, comment #6 and comment #8 both asked you to use the same version of 2017-02-04 (yesterday). The new crash report is *extremely* helpful.

We see a very useful call stack and can now diagnose the problem much better. So thanks again.

In nsImapProtocol::SetupSinkProxy() we do this:
    if (!m_imapMessageSink)
    {
      nsCOMPtr<nsIImapMessageSink> aImapMessageSink;
      (void) m_runningUrl->GetImapMessageSink(getter_AddRefs(aImapMessageSink));
      m_imapMessageSink = new ImapMessageSinkProxy(aImapMessageSink); <=== line 641
we now crash when trying to instantiate ImapMessageSinkProxy(nullptr).

This does seem to be related to bug 1328814 since there we kick on with life despite not being able to find a now renamed folder. That seems to open up this code path here. Kent is the expert on this.

Comment 11

[Kent James (:rkent)]

Razvan: yeah sorry about that, I did not read Wayne's link very carefully, and somehow I assumed he directed you to the recent esr45 run instead (which tests a different theory of the same problem).

Comment 12

[Jorg K (CEST = GMT+2)]

Kent, do you have some tips about how to fix this?

Comment 13

[Kent James (:rkent)]

Hints or tips? Hmmm ... in spite of your comments about me as the Imap expert, I'm more the last man standing more than an expert.

But I could give some hints about how I would approach. The crash report showed a path coming from I believe an embedded imap: or imap-message: protocol in the image of am email. It would be good to know where those come from. I thought we got rid of those in Magnus' patches from our formerly favorite bug. Perhaps this current problem is a transition problem, with old non-data uris lieing around causing issues. In that case, merely preventing the crash and not displaying the embedded content is probably the best approach.

Still, an invalid folder reference in a URL should not lead to a crash. The patch to bug 1328814 removed one previous location where invalid folder references threw an error, apparently leading to this current crash. One possible compromise would be to understand how that happens (through the creation presumably of invalid proxy references in the imap protocol), prevent it from happening at that point by throwing errors, that would presumably allow us to continue responsibly. Not displaying an embedded HTML image would be preferable to either crashing or failing in the nature that bug 1328814 was meant to solve. But better still would be to correct the URL in known circumstances such as a folder rename. I doubt if that is worth the work though, particularly as imap protocol references are known to cause all sorts of issues, which is why we are moving to data urls in the first place.

Comment 14

[Razvan]

Attachment: This one crashes TB

This is an working example. It must be in an IMAP folder in TB, otherwise it doesn't work (TB doesn't crash if is opened from the local folder of TB or as eml from a PC local folder).

More than that, the host/address of the IMAP server must be valid otherwise the TB doesn't crash.

I hope this help.

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 8833869 [details]
This one crashes TB

Razvan, that you so much!! Having an example that reproduces the crash it a big step towards fixing it.

As Kent said, this is coming from from an embedded image with an IMAP URL, since imgLoader::LoadImage() is in the call stack. In the message source we see:

<img moz-do-not-send="true" id="Picture_x0020_1" src="imap://[e-mail]:993/....

Razvan, one question: Does viewing the message with an older version of TB that doesn't crash show any image? I don't think it would since this appears to be an old invalid reference. So not showing anything here would solve the problem.

Comment 16

[Razvan]

No, it doesn't.

The IMAP folder that the link is pointing to is particular to the user that has forwarded the message and only him can access it, so there is no picture to see for the receiver.  The no-picture-found image is displayed on the older versions. 

I don't know if this is important for this bug, but the IMAP server is Dovecot and is secured with self signed certificate.

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: 1335638-IMAP-crash-fix.patch (v1)

This fixes the crash. This is quite a pain, once an invalid IMAP URL is in the system, it's hard to get out.

Kent, please tell me what you think. As per comment #10, only checking of aImapMessageSink was necessary. So perhaps the checking is overzealous. I'm also not sure what the various sink (proxies) do.

Another option would be to check the URL before calling SetupSinkProxy().

With the patch I get two grumbling elsewhere in the bowels of IMAP:
###!!! ASSERTION: shouldn't get an error loading url: 'false', file c:/mozilla-source/comm-central/mailnews/imap/src/nsImapIncomingServer.cpp, line 455
Well, it can now if the URL is invalid.

More interestingly:
[4848] ###!!! ASSERTION: shouldn't already have a channel listener: '!m_channelListener', file c:/mozilla-source/comm-central/mailnews/imap/src/nsImapProtocol.cpp, line 790

The other option would be to back out the "folder move/rename" bug and try to solve it some other way. I still think you should be able to clone something invalid, sigh.

Comment 18

[Wayne Mery (:wsmwk)]

Attachment: gsmith-bug1335578-image-blocked-okafgmbeepcacbgp.png

jorgk, perhaps this will further inform your investigations...

gsmith, who is a prolific crasher with multiple signature - mostly bug 1335578 [thunk]:mozilla::storage::Connection::`vcall''{100, {flat}}'' }'' - has done a lot of testing for me with nightly builds and such.  He encounters "Daily has blocked a file from loading into this message" [and no crash] when composing an email with a signature containing a logo image and using https://ftp.mozilla.org/pub/thunderbird/nightly/2016/12/2016-12-10-03-02-04-comm-central/thunderbird-53.0a1.en-US.win32.installer.exe

https://ftp.mozilla.org/pub/thunderbird/nightly/2016/12/2016-12-09-03-02-10-comm-central/thunderbird-53.0a1.en-US.win32.installer.exe handles the logo correctly

checkins: https://hg.mozilla.org/comm-central/pushloghtml?startdate=2016-12-09+03%3A05%3A00&enddate=2016-12-10+03%3A05%3A00

Comment 19

[Jorg K (CEST = GMT+2)]

(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #18)
> jorgk, perhaps this will further inform your investigations...
Wayne, no it doesn't. "Daily has blocked a file from loading into this message". Please refer to bug 1332606 comment #11. This is Magnus' new compose Windows magic of blocking file-based images and others.

Let's concentrate on the crash and nothing else.

Comment 20

[Jorg K (CEST = GMT+2)]

Kent, please take a look at crash ID
https://crash-stats.mozilla.com/report/index/ef6fbbe4-9790-4c4a-8b78-b2e932170206
posted in bug 1335345 comment #11

This comes from nsSyncRunnableHelpers.h:69:
  ImapServerSinkProxy(nsIImapServerSink* receiver)

So this code:
       res = m_runningUrl->GetImapServerSink(getter_AddRefs(aImapServerSink));
       m_imapServerSink = new ImapServerSinkProxy(aImapServerSink);
can also create null receivers at nsImapProtocol.cpp:647, so not just like 641 needs extra checking. So perhaps the checking I added isn't overzealous after all.

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: 1335638-IMAP-crash-fix.patch (v2)

This takes care of the crashes and tones down some NS_ASSERTS().

The message that used to cause the crash loads, but the status indicator keeps showing "Loading Message..." like it never finishes, somehow an indication of the fact that it tries accessing something without success.

Kent, let me know what you think of the new checks. As I said: We've seen crashes due to aImapMessageSink or aImapServerSink being null.

Comment 22

[Jorg K (CEST = GMT+2)]

Comment on attachment 8833903 [details]
gsmith-bug1335578-image-blocked-okafgmbeepcacbgp.png

Sorry, this is not useful, I know how the message looks like since I reviewed the bug where this was implemented.

Comment 26

[Jorg K (CEST = GMT+2)]

Comment on attachment 8833950 [details] [diff] [review]
1335638-IMAP-crash-fix.patch (v2)

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

I did some further digging in nsImapUrl::GetImapMailFolderSink() and friends.
https://dxr.mozilla.org/comm-central/rev/756fa9e390ab08f995382c9a9dfc6bf4b72421ca/mailnews/imap/src/nsImapUrl.cpp#147
See comments below.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +632,3 @@
>          m_imapMailFolderSink = new ImapMailFolderSinkProxy(aImapMailFolderSink);
> +      } else {
> +        return NS_ERROR_ILLEGAL_VALUE;

Maybe this is overzealous since nsImapUrl::GetImapMailFolderSink() sort of seems to expect a nullptr in m_imapMailFolderSink.

@@ +641,5 @@
>        (void) m_runningUrl->GetImapMessageSink(getter_AddRefs(aImapMessageSink));
> +      if (aImapMessageSink) {
> +        m_imapMessageSink = new ImapMessageSinkProxy(aImapMessageSink);
> +      } else {
> +        return NS_ERROR_ILLEGAL_VALUE;

I think this is necessary since we crash later otherwise.

@@ +651,5 @@
> +      res = m_runningUrl->GetImapServerSink(getter_AddRefs(aImapServerSink));
> +      if (aImapServerSink) {
> +        m_imapServerSink = new ImapServerSinkProxy(aImapServerSink);
> +      } else {
> +        return NS_ERROR_ILLEGAL_VALUE;

Same here. See comment #20.

@@ +660,5 @@
>        nsCOMPtr<nsIImapProtocolSink> anImapProxyHelper(do_QueryInterface(NS_ISUPPORTS_CAST(nsIImapProtocolSink*, this), &res));
> +      if (anImapProxyHelper) {
> +        m_imapProtocolSink = new ImapProtocolSinkProxy(anImapProxyHelper);
> +      } else {
> +        return NS_ERROR_ILLEGAL_VALUE;

This is most likely nonsense since the cast won't fail.

Comment 27

[Wayne Mery (:wsmwk)]

jph who crashes with vcall''{56 in 45.7.0, with nightly build all crash are ImapMessageSinkProxy::ImapMessageSinkProxy bp-58dda482-6b93-4966-a797-9485f2170206

Comment 28

[Jorg K (CEST = GMT+2)]

We have two crashes here:
ImapMessageSinkProxy - bp-58dda482-6b93-4966-a797-9485f2170206 - nsSyncRunnableHelpers.h:89 
ImapServerSinkProxy  - bp-ef6fbbe4-9790-4c4a-8b78-b2e932170206 - nsSyncRunnableHelpers.h:69
Both are addressed in my patch.

Comment 29

[Kent James (:rkent)]

Comment on attachment 8833950 [details] [diff] [review]
1335638-IMAP-crash-fix.patch (v2)

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

r+ with my one comment fixed.

These sync proxies look like they will crash if the conditions checked by this bug are violated, so this seems pretty safe. But there are lots of dark corners lurking in this code, and unknown behavior when these failures are flagged. So I would not be too quick to uplift this.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +672,5 @@
>  {
>    nsCOMPtr<nsIInterfaceRequestor> callbacks;
>    aChannel->GetNotificationCallbacks(getter_AddRefs(callbacks));
>  
>    nsCOMPtr<nsILoadGroup> loadGroup;

Later (line 1173) there is another call to SetupSinkProxy in SetupMainThreadProxies() that should also propagate the error.

Comment 30

[Jorg K (CEST = GMT+2)]

Can you please clarify. Please refer to my comment #26. There I suggested to only return an error on the 2nd and 3rd case since the 1st case is already handled specially and the 4th can't fail.

Comment 31

[Jorg K (CEST = GMT+2)]

Attachment: 1335638-IMAP-crash-fix.patch (v3).

This is what I discussed with Kent on IRC, although we're not sure how to react to an error returned by SetupSinkProxy()/SetupMainThreadProxies() in ProcessCurrentURL(). Kent said on IRC that if we get it wrong, the session will hang. As an example he mentioned to look at HandleCurrentUrlError().

Comment 32

[Jorg K (CEST = GMT+2)]

Comment on attachment 8835905 [details] [diff] [review]
1335638-IMAP-crash-fix.patch (v3).

Carrying forward Kent's r+ with the issues addressed.
I looked into improving error handling further but couldn't come up with anything better. So I'll land this now and get Wayne for find many testers.

Comment 33

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/cb631957f0df8544737186e625f8ea9794e38cb0

This should fix the crashes. Wayne, can you please get test coverage here. I will also uplift this to Aurora so it goes onto the Earlybird Channel and those poor people stop crashing.

We need Razvan and Andrea from bug 1335345 comment #9.

After two weeks we can uplift to beta if all goes well ;-)

Comment 34

[Razvan]

The current Daily still crashes, but I don't know if it is the patched one.

Comment 35

[Jorg K (CEST = GMT+2)]

Sure, I landed the patch at 2017-02-10 16:14:06 CET. Please try tomorrow's build, or if you let me know your platform (and x86 or x64) you can use a build when the compile is ready, I can provide the link in two hours. I tested with your sample e-mail, so that won't crash any more.

Comment 36

[Jorg K (CEST = GMT+2)]

You can try these builds:
Windows x86: https://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-win32/1486739243/
Windows x64: https://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-win64/1486739243/
Linux64:   https://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-linux64/1486739243/

Andrea, we know you from bug 1335345 comment #9. Can you check these builds, please.

Comment 37

[Jorg K (CEST = GMT+2)]

Comment on attachment 8835905 [details] [diff] [review]
1335638-IMAP-crash-fix.patch (v3).

[Approval Request Comment]
Regression caused by (bug #): Bug 1328814.
User impact if declined: Crash!!
Testing completed (on c-c, etc.): Manual.
Risk to taking this patch (and alternatives if risky): Can't get any worse, or maybe it can ;-)

Comment 38

[Andrea]

(In reply to Jorg K (GMT+1) from comment #36)
> You can try these builds:
> Windows x86:
> https://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-
> win32/1486739243/
> Windows x64:
> https://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-
> win64/1486739243/
> Linux64:  
> https://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-
> linux64/1486739243/
> 
> Andrea, we know you from bug 1335345 comment #9. Can you check these builds,
> please.

Hi Jorg,
I've tried Linux64 version and works fine for me.

Comment 39

[Jorg K (CEST = GMT+2)]

Aurora (TB 53):
https://hg.mozilla.org/releases/comm-aurora/rev/67909d34664406b0153179342e35716dae965688

Comment 40

[Razvan]

Sorry for the late response. From my point of view, the bug was fixed in the Daily.

Comment 41

[Jorg K (CEST = GMT+2)]

Comment on attachment 8835905 [details] [diff] [review]
1335638-IMAP-crash-fix.patch (v3).

This adds stability to TB, even it we don't land bug 1328814 back on TB 52 beta 3

Comment 42

[Jorg K (CEST = GMT+2)]

Beta (TB 52):
https://hg.mozilla.org/releases/comm-beta/rev/9807cf77ed6948b29c4750a3351b8b2484388a88

Comment 43

[Jorg K (CEST = GMT+2)]

Razvan, thank you so much for your help. You contribution was absolutely essential to getting this fixed quickly! We are very grateful. And thank you for being on the Daily release channel and helping us. You know that this can be a bumpy ride sometimes. Currently we have one loss of functionality: Proxies don't work for IMAP. We're working on it.

Comment 44

[Razvan]

I am sorry to here that (that the proxy isn't working) but unfortunately I don't have any experience with any type of proxy internals so I can't help with this problem.