TSan: data race image/imgFrame.cpp on hasCheckedOptimize

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jseward, Unassigned)

Tracking

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 attachments)

TSan reports races on hasCheckedOptimised (and, probably, indirectly,
on gDisableOptimize).  These are reported very quickly when doing any
surfing with a TSan-enabled build, presumably after encountering
images for the first time.

The race is trivial and (probably!) harmless, but it is unnecessary
and causes noise which makes it harder to spot other races.
gDisableOptimize is only used in one place, imgFrame::Optimize(), and
only on the main thread, so we can simply move the check to there
instead.  Also, placing the test at the single point of use seems more
logical than having it in imgFrame::imgFrame for no obvious reason.
Posted patch Proposed fixSplinter Review
Posted file TSan complaint
Whiteboard: [gfx-noted]
Comment on attachment 8761256 [details] [diff] [review]
Proposed fix

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

Thanks for fixing this issue!

::: image/imgFrame.cpp
@@ +330,5 @@
>    MOZ_ASSERT(mLockCount == 1,
>               "Should only optimize when holding the lock exclusively");
>  
> +  // Check whether image optimization is disabled.  This isn't thread-safe,
> +  // but this function runs only on the main thread, so we're OK.

I'd just leave it at "Check whether image optimization is disabled. (Not thread safe.)" or something similar. This method asserts "NS_IsMainThread()" just a few lines up, and I think that's self-documenting enough.
Attachment #8761256 - Flags: review+
Pushed by jseward@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b64db3b38fd8
TSan: data race image/imgFrame.cpp on hasCheckedOptimize.  r=seth.
https://hg.mozilla.org/mozilla-central/rev/b64db3b38fd8
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.