Closed Bug 1278524 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Networking: Cache, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: jseward, Assigned: jseward)

Details

(Whiteboard: [necko-active])

Attachments

(2 files, 1 obsolete file)

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.
Attached file TSan complaints
Attached patch Proposed fix (obsolete) — Splinter Review
Assignee: nobody → jseward
Whiteboard: [necko-active]
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?
(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 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.
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.
Attachment #8760717 - Attachment is obsolete: true
Attachment #8765270 - Flags: review?(honzab.moz)
Attachment #8765270 - Flags: review?(honzab.moz) → review+
Pushed by jseward@mozilla.com:
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.
https://hg.mozilla.org/mozilla-central/rev/ff6f8a26f7b7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.