Clean up usage of nsImapProtocol::m_imapThreadIsRunning

RESOLVED FIXED

Status

MailNews Core
Backend
RESOLVED FIXED
15 years ago
10 years ago

People

(Reporter: Ere Maijala (slow), Assigned: Ere Maijala (slow))

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

15 years ago
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.
(Assignee)

Comment 1

15 years ago
Created attachment 119093 [details] [diff] [review]
Patch to add the enter/exit monitor to protect from access from multiple threads
(Assignee)

Updated

15 years ago
Attachment #119093 - Flags: superreview?(bienvenu)

Comment 2

15 years ago
is this variable ever accessed from a thread other than the imap thread?
(Assignee)

Comment 3

15 years ago
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?
Attachment #119093 - Attachment is obsolete: true
Attachment #119093 - Flags: superreview?(bienvenu)
(Assignee)

Comment 4

15 years ago
Created attachment 119099 [details] [diff] [review]
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.
(Assignee)

Comment 5

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

David, what do you think?
Attachment #119099 - Flags: superreview?(bienvenu)

Comment 6

15 years ago
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.
(Assignee)

Comment 7

15 years ago
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.
Attachment #119099 - Attachment is obsolete: true
Attachment #119099 - Flags: superreview?(bienvenu)
(Assignee)

Updated

15 years ago
Summary: Should protect nsImapProtocol::ImapThreadIsRunning() with a monitor → Clean up usage of nsImapProtocol::m_imapThreadIsRunning
(Assignee)

Comment 8

15 years ago
Created attachment 119108 [details] [diff] [review]
3rd try, clean up the usage of m_imapThreadIsRunning
(Assignee)

Comment 9

15 years ago
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?
Attachment #119108 - Flags: superreview?(bienvenu)

Comment 10

15 years ago
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.
Attachment #119108 - Flags: superreview?(bienvenu) → superreview+
(Assignee)

Comment 11

15 years ago
Yes, it works. I've ran it in debugger to check that it goes as expected. 
(Assignee)

Updated

15 years ago
Attachment #119108 - Flags: review?(sspitzer)
(Assignee)

Updated

15 years ago
Attachment #119108 - Flags: review?(sspitzer) → review?(mscott)
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.
Attachment #119108 - Flags: review?(mscott) → review?(mscott)

Updated

15 years ago
Attachment #119108 - Flags: review?(mscott) → review+
(Assignee)

Comment 13

15 years ago
Thanks a bunch! Fix checked in.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.