TSan: data race image/src/imgRequest.h:119 HadInsecureRedirect

RESOLVED FIXED in Firefox 39

Status

()

Core
ImageLib
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: froydnj, Unassigned)

Tracking

(Blocks: 1 bug)

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

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [tsan] gfx-noted)

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
Created attachment 8584496 [details]
imglib-redirect-race.txt

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

* Specific information about this bug

I'm not entirely clear on the race here, but Seth asked me to file this one.  The reading side is pretty obvious, in imgRequest::HadInsecureRace.  One interesting thing about this race is that the member in question, mHadInsecureRedirect, is a bitfield, so the "write" to the field doesn't necessarily have to be that member; it could be any of the members that share the same byte of storage in the imgRequest object.

That being said, the stack for the writing side of the race is not terribly helpful.  It is particularly odd to me that nsRefPtr::operator= is being fingered, as that should be copying pointers only, and not twiddling the bits of imgRequest objects itself.  I'm going to chalk that one up to bad debug information and/or bad propagation of debug information during inlining.

Still doesn't help pinpoint the location of the write, though.  I looked at all the places where mHadInsecureRedirect was written and--though I didn't trace call chains or anything like that--none of them looked like they would be called from OnDataAvailable.  I didn't perform exhaustive analysis of all the fields sharing byte storage with mHadInsecureRedirect, either.

All that is long-winded analysis for something that Seth made it sound like was added fairly recently, so perhaps he'll have a much better idea of where to look. :)  ni? Seth just to ensure this gets into his queue.

* 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
Flags: needinfo?(seth)
(Reporter)

Comment 1

3 years ago
Bugs are much more helpful when they have descriptive titles.
Summary: TSan: data race → TSan: data race image/src/imgRequest.h:119 HadInsecureRedirect
Whiteboard: [tsan] → [tsan] gfx-noted
Blocks: 1137002
This got introduced in bug 1082837, which just landed, and contained a patch that dated from before the recent TSan fixes in ImageLib. That patch broke the invariant that we need to take the imgRequest's lock when accessing the bitfield.

Unlike the previous issues, fixing this won't require a big rewrite or anything, so I'll just do it in this bug.
Created attachment 8584937 [details] [diff] [review]
Fix data race on imgRequest::mHadInsecureRedirect.

Here's the patch. We just need to take the lock when accessing
mHadInsecureRedirect.
Flags: needinfo?(seth)
Attachment #8584937 - Flags: review?(tanvi)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=91fc2ac380da

Comment 5

3 years ago
Comment on attachment 8584937 [details] [diff] [review]
Fix data race on imgRequest::mHadInsecureRedirect.

This looks okay to me, but I'm probably no the best reviewer for this.
Attachment #8584937 - Flags: review?(tanvi) → feedback+
Attachment #8584937 - Flags: review?(tnikkel)
Attachment #8584937 - Flags: review?(tnikkel) → review+
Thanks for the review!
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b72da096a3c
https://hg.mozilla.org/mozilla-central/rev/5b72da096a3c
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.