Bug 1801067 Comment 55 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

Well, the ideal solution from comment 54 seems very difficult to do. In my experiment I immediately run into problems.
Some code must run on the main thread, and we're reaching assertions when running it from either the IMAP thread or the socket thread.
The IMAP code has a lot of assumptions baked in about the thread it is running on...

So, let's try harder with the pragmatic approach (minimal changes).
I've made more experiments.

Both Emilio and Magnus had suggested to skip the call to IsAlive.

I agree we could remove that from nsImapProtocol::TellThreadToDie()

In nsImapProtocol::ImapThreadMainLoop() the result of the call is used to decide about retrying. I don't have a great idea how to change that logic. However, that code runs on the IMAP thread, and the call to IsAlive should be simple and quick. I'm hoping it's safe to dispatch IsAlive to the socket thread in that place.

Then we have the calls to GetSecurityInfo. When Emilio hit the deadlock in bug 1816633 comment 1, it was inside nsImapProtocol::LoadImapUrlInternal(), running on the main thread, dispatching GetSecurityInfo() to the socket thread.

As a wild experiment I simply disabled that code, which skips adding that info to the mock channel.
In my manual testing so far I didn't find a negative side effect yet.
Let me allow to cross fingers and hope that we indeed could disable it, and maybe follow up later with a solution, should it be necessary.

Gene pointed out another place in comment 53, which we weren't yet handling. That's true, I tested a connection that requires a certificate exception, and Gene's addition is necessary in that place. Luckily, that place runs on the IMAP thread, so again (hopefully) it's safe to proxy to the socket thread from there. In my manual testing, adding the exception worked.

Let's see if that combination of changes survives a try build.
Well, the ideal solution from comment 54 seems very difficult to do. In my experiment I immediately run into problems.
Some code must run on the main thread, and we're reaching assertions when running it from either the IMAP thread or the socket thread.
The IMAP code has a lot of assumptions baked in about the thread it is running on...

So, let's try harder with the pragmatic approach (minimal changes).
I've made more experiments.

Both Emilio and Magnus had suggested to skip the call to IsAlive.

I agree we could remove that from nsImapProtocol::TellThreadToDie()

In nsImapProtocol::ImapThreadMainLoop() the result of the call is used to decide about retrying. I don't have a great idea how to change that logic. However, that code runs on the IMAP thread, and the call to IsAlive should be simple and quick. I'm hoping it's safe to dispatch IsAlive to the socket thread in that place.

Then we have the calls to GetSecurityInfo. When Emilio hit the deadlock in bug 1816633 comment 1, it was inside nsImapProtocol::LoadImapUrlInternal(), running on the main thread, dispatching GetSecurityInfo() to the socket thread.

As a wild experiment I simply disabled that code, which skips adding that info to the mock channel.
In my manual testing so far I didn't find a negative side effect yet.
Let me allow to cross fingers and hope that we indeed could disable it, and maybe follow up later with a solution, should it be necessary.

Gene pointed out another place in comment 53, which we weren't yet handling. That's true, I tested a connection that requires a certificate exception, and Gene's addition is necessary in that place. Luckily, that place runs on the IMAP thread, so again (hopefully) it's safe to proxy to the socket thread from there. In my manual testing, adding the exception worked.

Let's see if that combination of changes survives a try build.
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=41769cab39516687f216bd8a537ffa6aede17b24

Back to Bug 1801067 Comment 55