1120611 - TSan: data race gfx/thebes/gfxPlatform.cpp:1767 GetRenderingIntent

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Nathan Froyd [:froydnj]]

[cribbing from decoder's script, hopefully he won't mind]

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

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].

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.

I don't think this particular race is complicated, TSan is just not happy that multiple threads are reading and writing to gCMSIntent{,Initialized}.  Blacklisting them or turning them into lightweight Atomic<> variables is probably sufficient.

[1] http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: tsan-gfx-race.txt

Ah, the log would be helpful.

Comment 3

[Andrew McCreight [:mccr8]]

Looks like two image decoder threads racing.

Comment 4

[Andrew McCreight [:mccr8]]

Specifically, on gCMSIntent.

Comment 5

[Andrew McCreight [:mccr8]]

It looks like this was added in bug 910860.

Comment 6

[Andrew McCreight [:mccr8]]

Jeff had a comment on this in the duped bug:

(In reply to Jeff Muizelaar [:jrmuizel] from comment #1)
> I expect this is mostly benign. The global that's being shared is basically
> just an optimization. Having the function run multiple times shouldn't
> really be a big deal.
> 
> That being said, the right way to fix this is probably to always call
> GetRenderingIntent from the main thread and pass the information through.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

If all that's needed is an assignee then as the author of bug 910860 Milan should be it by default, or he can pass it on to somebody else.

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

I like Jeff's idea, just want to make sure we don't have a dependency on bug 971126 or worse, bug 972099.

Bug 910860 didn't introduce the race, but made the consequences worse, in that we went from:

if (gCMSIntent == -2) {
   ... bunch of stuff
   gCMSIntent = something not -2;
}

to:

if (!gCMSIntentIntialized) {
  gCMSIntentInitialized = true;
   ... bunch of stuff
}

so we would set the flat to "initialized" before we actually had the value set, so instead of doing the work twice, there is a better chance of returning the wrong (e.g., -2) value.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Don't cache the cms intent, it's already cached in the prefs. r=jrmuizelaar

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d54775f6e6dd

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Milan Sreckovic [:milan] from comment #10)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=d54775f6e6dd

Looks green.

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

Review ping.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b9ef69e9cfd

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8b9ef69e9cfd