1136889 - TSan: data race xpcom/io/nsStreamUtils.cpp:415 Run / netwerk/base/nsInputStreamPump.cpp:362 AsyncRead

Status: RESOLVED INCOMPLETE

(Core :: Networking, defect)

Description

[Nathan Froyd [:froydnj]]

Attachment: netwerk-stream-race.txt

The attached logfile shows a thread/data race detected by TSan (ThreadSanitizer).

* Specific information about this bug

The stacks in the log are, unfortunately, a complete mess.

After poking around in the disassembly, I think the write that's being complained about is the assignment to mSink here:

http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsStreamUtils.cpp#361

But I don't see how ~nsCOMPtr_base can be called from a line making a virtual method call to OpenInputStream:

http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsInputStreamPump.cpp#362

Nor is it clear to me how mSink could be touched by another thread.

Patrick, any idea what's going on here?

* General information about TSan, data races, etc.

Typically, races reported by TSan are not false positives, but it is possible that the race is benign. Even in this case though, we should try to come up with a fix unless this would cause unacceptable performance issues. Also note that seemingly benign races can possibly be harmful (also depending on the compiler and the architecture) [1][2].

If the bug cannot be fixed, then this bug should be used to either make a compile-time annotation for blacklisting or add an entry to the runtime blacklist.

[1] http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong
[2] _How to miscompile programs with "benign" data races_: https://www.usenix.org/legacy/events/hotpar11/tech/final_files/Boehm.pdf

Comment 1

[Patrick McManus [:mcmanus]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #0)
> Created attachment 8569400 [details]
> netwerk-stream-race.txt
> 
> The attached logfile shows a thread/data race detected by TSan
> (ThreadSanitizer).
> 
> * Specific information about this bug
> 
> The stacks in the log are, unfortunately, a complete mess.
> 
> After poking around in the disassembly, I think the write that's being
> complained about is the assignment to mSink here:
> 
> http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsStreamUtils.cpp#361
> 
> But I don't see how ~nsCOMPtr_base can be called from a line making a
> virtual method call to OpenInputStream:
> 
> http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsInputStreamPump.
> cpp#362

it certainly isn't obvious. are you confident in the line numbers?


cc jduell - he's probably best atuned to the stream code.

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Patrick McManus [:mcmanus] from comment #1)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #0)
> > Created attachment 8569400 [details]
> > netwerk-stream-race.txt
> > 
> > The attached logfile shows a thread/data race detected by TSan
> > (ThreadSanitizer).
> > 
> > * Specific information about this bug
> > 
> > The stacks in the log are, unfortunately, a complete mess.
> > 
> > After poking around in the disassembly, I think the write that's being
> > complained about is the assignment to mSink here:
> > 
> > http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsStreamUtils.cpp#361
> > 
> > But I don't see how ~nsCOMPtr_base can be called from a line making a
> > virtual method call to OpenInputStream:
> > 
> > http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsInputStreamPump.
> > cpp#362
> 
> it certainly isn't obvious. are you confident in the line numbers?

As confident as the compiler is; I am reasonably confident that the write is to mSink, though I didn't do an exhaustive trace through the assembly.  I did realize that unified builds might be screwing things up, so I should probably retest with that.

Comment 3

[Jason Duell]

I wish I could say I have a clue here, but I don't.  Perhaps honza may have an idea...

Comment 4

[Honza Bambas (:mayhemer)]

Nathan, any news with the retest here?

Also, please - big PLEASE - when you are pasting links to sources PLEASE use version annotated links.  The sources are constantly changing and line numbers in the non-annotated references are soon going to be misplaced.  Thanks.

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Honza Bambas (:mayhemer) from comment #4)
> Nathan, any news with the retest here?

I asked Julian about this, who has been doing more work with TSan lately than I have.  I don't remember this being a terribly common race and Julian says he hasn't seen anything like it either.  It's possible the stacks came from bogus debug information anyway, so I'm going to close this bug.  We can open up another one if this comes back.