1219405 - TSan: data race image/decoders/nsPNGDecoder.cpp:41 GetPNG{DecoderAccounting,}Log

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Nathan Froyd [:froydnj]]

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

* Specific information about this bug

In this race, we're calling into GetPNGDecoderAccountingLog from multiple threads, which means we're touching the static variable in GetPNGDecoderAccountingLog from multiple threads without any locking.  If we had written it:

{
  static PRLogModuleInfo* sPNGDALog = PR_NewLogModule("PNGDA");
  return sPNGDALog;
}

that would have been OK.  Fortunately, we recently landed some xpcom logging things that cleanup the boilerplate associated with this pattern and should also fix the thread-safety issues as well.
* 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

[Nathan Froyd [:froydnj]]

Attachment: TSan stack trace

Comment 2

[Seth Fowler [:seth] [:s2h]]

(In reply to Nathan Froyd [:froydnj] from comment #0)
> that would have been OK.  Fortunately, we recently landed some xpcom logging
> things that cleanup the boilerplate associated with this pattern and should
> also fix the thread-safety issues as well.

What are these XPCOM logging things? I'm guessing some changes are still necessary to the ImageLib code?

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: use LazyLogModule for PNG decoder logging

Here's the example that you might have been looking for.  I can convert the
other logs in image/ in this bug or in a followup bug, your call.

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Seth Fowler [:seth] [:s2h] from comment #2)
> (In reply to Nathan Froyd [:froydnj] from comment #0)
> > that would have been OK.  Fortunately, we recently landed some xpcom logging
> > things that cleanup the boilerplate associated with this pattern and should
> > also fix the thread-safety issues as well.
> 
> What are these XPCOM logging things? I'm guessing some changes are still
> necessary to the ImageLib code?

Ideally the just-posted patch should provide all the information you need here.  Please ni? me if it doesn't.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8680213 [details] [diff] [review]
use LazyLogModule for PNG decoder logging

Review of attachment 8680213 [details] [diff] [review]:
-----------------------------------------------------------------

Much better than the old approach, yeah.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/765d51aa96c7

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/765d51aa96c7

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/765d51aa96c7

Comment 9

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion