1940154 - Audit the thread protection for MboxMsgInputStream (and any other c-c nsIInputStream uses)

Status: RESOLVED DUPLICATE of bug 1931863

(MailNews Core :: General, task)

Description

[Ben Campbell]

In Bug 1931863, TSAN flagged up some potential issues in our mbox nsIInputStream implementation, MboxMsgInputStream.

Here's what I wrote about it on Magnus' initial patch in phab:

I vaguely thought that nsIInputStream, being an xpcom interface, had to be called from the main thread. But that's not the case:

https://searchfox.org/comm-central/source/mozilla/xpcom/io/nsIInputStream.idl#54

I'm using an nsInputStreamPump to use the stream in an async fashion, and I think it delegates reads to the STS thread to avoid blocking the main thread.

I don't think there's a data race, because the main thread is just waiting for an event saying the STS thread has fetched some data - there two threads are cooperating and there aren't any others involved.

Magnus worked on a patch over in that bug to satisfy TSAN.
But I can definitely still see some cracks in the thread safety (eg status var is not guarded). I'm not really worried that there's a practical issue here that'll bite us - the threading involved here is pretty minimal and cooperative.

But this is some core stuff, I think it's worth following up to real digging into this and really understand exactly what we need to protect and why. More of an education thing than anything!
Looking at comparable non-async nsIInputStream classes over in m-c would probably be a good start.

Comment 1

[Ben Campbell]

Update: I am worried that there's a practical issue here that'll bite us. Bug 1917447 seems to be it.

The patches in Bug 1931863 kind of render this bug moot - the only real solution seems to be to make the MboxMsgInputStream properly threadsafe, which involves gating all member accesses behind a lock. A bit tedious, but there you are. I was hopeful there was a more elegant solution.
If the Bug 1931863 patches work, I'll close this one.