1278927 - TSan: data race image/imgFrame.cpp on hasCheckedOptimize

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Julian Seward [:jseward]]

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.

Comment 1

[Julian Seward [:jseward]]

Attachment: Proposed fix

Comment 2

[Julian Seward [:jseward]]

Attachment: TSan complaint

Comment 3

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

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.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b64db3b38fd8
TSan: data race image/imgFrame.cpp on hasCheckedOptimize.  r=seth.

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b64db3b38fd8