200152 - Clean up usage of nsImapProtocol::m_imapThreadIsRunning

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Ere Maijala (slow)]

I suspect enter/exit monitor is missing from
nsImapProtocol::ImapThreadIsRunning() making it possible to read it while it's
being assigned in the thread. Patch coming up.

Comment 1

[Ere Maijala (slow)]

Attachment: Patch to add the enter/exit monitor to protect from access from multiple threads

Comment 2

[David :Bienvenu]

is this variable ever accessed from a thread other than the imap thread?

Comment 3

[Ere Maijala (slow)]

Comment on attachment 119093 [details] [diff] [review]
Patch to add the enter/exit monitor to protect from access from multiple threads

Right, it isn't. Then I'd say this little function is quite useless. It could
only protect from starting multiple threads which shouldn't happen anyway,
right?

Comment 4

[Ere Maijala (slow)]

Attachment: Second try, now protects the setting in TellThreadToDie() too

Okay, it _is_ actually set outside the thread in TellThreadToDie(), where it
isn't protected by the same monitor as elsewhere. This fixes both.

Comment 5

[Ere Maijala (slow)]

Comment on attachment 119099 [details] [diff] [review]
Second try, now protects the setting in TellThreadToDie() too

David, what do you think?

Comment 6

[David :Bienvenu]

I'm not convinced we need to set isthreadrunning to false in TellThreadToDie -
it already sets m_threadShouldDie, which is checked in DeathSignalReceived. It
would be nice to remove the monitor calls instead of adding them, if we can
convince ourselves/make it so it's only accessed from one thread.

Comment 7

[Ere Maijala (slow)]

Comment on attachment 119099 [details] [diff] [review]
Second try, now protects the setting in TellThreadToDie() too

I wonder if we really need the flag at all, but I'll make it so it's not used
outside the thread.

Comment 8

[Ere Maijala (slow)]

Attachment: 3rd try, clean up the usage of m_imapThreadIsRunning

Comment 9

[Ere Maijala (slow)]

Comment on attachment 119108 [details] [diff] [review]
3rd try, clean up the usage of m_imapThreadIsRunning

Ok, could this finally be the right way to do it?

Comment 10

[David :Bienvenu]

Comment on attachment 119108 [details] [diff] [review]
3rd try, clean up the usage of m_imapThreadIsRunning

it looks ok to me - does it work? In order to hit the tellthreadtodie code, I
think you need to have an imap connection timeout, or just shutdown.

Comment 11

[Ere Maijala (slow)]

Yes, it works. I've ran it in debugger to check that it goes as expected. 

Comment 12

[Simon Paquet [:sipaq]]

Comment on attachment 119108 [details] [diff] [review]
3rd try, clean up the usage of m_imapThreadIsRunning

Moving over to Scott's new adress. Scott, this patch already has superreview
from David Bienvenu. Can you please review?
The patch is a little bit bitrotted (the line numbers no longer apply), but in
itself it is still valid.

Comment 13

[Ere Maijala (slow)]

Thanks a bunch! Fix checked in.