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

RESOLVED FIXED in Firefox 38

Status

()

Core
ImageLib
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: froydnj, Assigned: milan)

Tracking

(Blocks: 1 bug)

unspecified
mozilla38
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed)

Details

(Whiteboard: [tsan][gfx-noted])

Attachments

(2 attachments)

[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
Created attachment 8547774 [details]
tsan-gfx-race.txt

Ah, the log would be helpful.
Duplicate of this bug: 1120610
Looks like two image decoder threads racing.
Component: Graphics → ImageLib
Specifically, on gCMSIntent.
It looks like this was added in bug 910860.
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.
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.
Blocks: 910860
Whiteboard: [tsan] → [tsan][gfx-noted]
(Assignee)

Comment 8

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

Updated

3 years ago
Assignee: nobody → milan
(Assignee)

Comment 9

3 years ago
Created attachment 8548466 [details] [diff] [review]
Don't cache the cms intent, it's already cached in the prefs. r=jrmuizelaar
Attachment #8548466 - Flags: review?(jmuizelaar)
(Assignee)

Comment 10

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d54775f6e6dd
(Assignee)

Comment 11

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

Looks green.
(Assignee)

Comment 12

3 years ago
Review ping.
Flags: needinfo?(jmuizelaar)
Attachment #8548466 - Flags: review?(jmuizelaar) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b9ef69e9cfd
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8b9ef69e9cfd
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(Assignee)

Updated

3 years ago
Flags: needinfo?(jmuizelaar)
You need to log in before you can comment on or make changes to this bug.