1278524 - TSan: data races netwerk/cache2/CacheEntry.{h,cpp} on CacheEntry::{mUseDisk, mSecurityInfoLoaded, mPreventCallbacks}

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Julian Seward [:jseward]]

TSan reports data races on CacheEntry::{mUseDisk, mSecurityInfoLoaded, 
mPreventCallbacks} for more or less any surfing performed with a TSan-enabled
Firefox build, and certainly when loading techcrunch.com.  It is easy
to reproduce.

I assume that all 3 fields are properly synchronised from a logical point
of view.  However, because mUseDisk and the 6 "bool:1" fields that follow
it live in the same byte, and are used by different threads, the hardware
cannot update them atomically, so TSan correctly reports races on the
containing byte.

Comment 1

[Julian Seward [:jseward]]

Attachment: TSan complaints

Comment 2

[Julian Seward [:jseward]]

Attachment: Proposed fix

Comment 3

[Honza Bambas (:mayhemer)]

This is a memory footprint sensitive class (heavily used), we should try save even bytes here.

Can we move the bool const set somewhere else?

Comment 4

[Nicholas Nethercote [inactive]]

(In reply to Honza Bambas (:mayhemer) from comment #3)
> This is a memory footprint sensitive class (heavily used), we should try
> save even bytes here.

I just tested on 64-bit Linux. This patch doesn't change the class size, which is 256 bytes, because the class's fields are not carefully packed. E.g. with the patch applied I was able to get it down to 248 by changing EState and ERegistration to uint8_t enums and Ops::mValue to uint8_t. With care some more savings could probably be found.

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 8760717 [details] [diff] [review]
Proposed fix

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

::: netwerk/cache2/CacheEntry.h
@@ +293,5 @@
>    nsCString mStorageID;
>  
> +  // mUseDisk and mSkipSizeCheck are plain "bool", not "bool:1", so as to
> +  // avoid bitfield races with the byte containing mSecurityInfoLoaded et al.
> +  // See bug 1278524.

It looks like mIsDoomed is a non-bitfield field for the same reason. Perhaps it should be moved up with these ones and the comments combined.

Comment 6

[Julian Seward [:jseward]]

Attachment: bug1278524-fix-CacheEntry-bitfield-races-2.cset

Revised patch, that takes into account comments in comment 4 and comment 5.
I re-verified that the new layout still only takes 256 bytes on 64-bit Linux.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff6f8a26f7b7
TSan: data races netwerk/cache2/CacheEntry.{h,cpp} on CacheEntry::{mUseDisk, mSecurityInfoLoaded, mPreventCallbacks}.  r=honzab.moz.

Comment 8

[Carsten Book [:Tomcat]]

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