1256987 - TSan: data race db/sqlite3/src/sqlite3.c:72196:16 in sqlite3ExpirePreparedStatements

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Julian Seward [:jseward]]

Attachment: race-sqlite3ExpirePreparedStatements

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

* Specific information about this bug

Perhaps this is the same as, or related to bug 1153409 or bug 1153415 ?  I don't know.

* 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

Comment 1

[Julian Seward [:jseward]]

Once bug 1248915 lands, this will be the most frequently reported race
when running a TSan-ified build of Firefox on xpcshell tests:

45 modules/libpref/Preferences.cpp:1769:38 in mozilla::UintVarChanged
21 db/sqlite3/src/sqlite3.c:72196:16 in sqlite3ExpirePreparedStatemen <----
14 js/src/gc/Heap.h:550:26 in getAllocKind
11 toolkit/components/telemetry/Telemetry.cpp:3335:37 in CanRecordBase
11 toolkit/components/telemetry/Telemetry.cpp:3335:11 in CanRecordBase
10 tools/profiler/core/platform-linux.cc:250:46 in (anonymous namespac
10 tools/profiler/core/platform-linux.cc:249:46 in (anonymous namespac
10 tools/profiler/core/platform.h:350:34 in IsActive
10 signal handler spoils errno tools/profiler/core/platform-linux.cc:2
 7 tools/profiler/gecko/ThreadResponsiveness.cpp:114:21 in ThreadRespo
 7 netwerk/base/nsPACMan.cpp:344:13 in nsPACMan::Shutdown()
 6 nsprpub/pr/src/malloc/prmem.c:458:9 in PR_Free
 5 xpcom/glue/nsCOMPtr.h:331:13 in assign_assuming_AddRef
 4 nsprpub/pr/src/misc/prinit.c:791:33 in PR_CallOnce
 4 nsprpub/pr/src/misc/prinit.c:776:16 in PR_CallOnce
 4 nsprpub/pr/src/io/pripv6.c:326:12 in _pr_ipv6_is_present
 4 lock-order-inversion (potential deadlock) db/sqlite3/src/sqlite3.c:

Comment 2

[D. Richard Hipp]

One thread is reading a read-only bit in a bit vector while the other thread is changing another bit in that same bit vector.  So, yes, this is a thread-safety fault.  But I don't think it is one that could ever cause problems.  Nevertheless, I'll fix it in SQLite for the next release (due in a few weeks).  I don't think this is important enough to justify a patch release of SQLite, but please speak up if you think that assessment is wrong.

Comment 3

[D. Richard Hipp]

I believe the "Pre-release Snapshot" at https://www.sqlite.org/download.html will fix this problem.

Comment 4

[Julian Seward [:jseward]]

drh, thank you for your quick response.  And for fixing this.

I don't think I can easily test/verify a complete snapshot as integrated
into the Mozilla tree.  But I could try out a patch, if you have one that
is small and has a chance of being backportable.  Is that feasible?
I think what is in the tree is "3.11.0".

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Julian Seward [:jseward] from comment #4)
> I don't think I can easily test/verify a complete snapshot as integrated
> into the Mozilla tree.  But I could try out a patch, if you have one that
> is small and has a chance of being backportable.  Is that feasible?
> I think what is in the tree is "3.11.0".

Couldn't you just replace sqlite.c/h with the provided ones and run the build through TSan on Try?
Fwiw, we don't take patches to sqlite in our tree, only officially released versions, unless the reasons are REALLY compelling (I think it happened twice in the last 8 years). And according to comment 2, this is unlikely to cause problems.

Comment 6

[D. Richard Hipp]

The patch is here: https://www.sqlite.org/src/info/e0b116edd64a55c9

Sorry for the delay.  I thought I had sent the above to you yesterday, but apparently I did something wrong and the message didn't go through...

Comment 7

[Julian Seward [:jseward]]

Ach, I forgot the STR.  This is moderately difficult to reproduce
(is scheduling sensitive, maybe).  The best I have is

Do a tsan-enabled build, then:

TSAN_OPTIONS=suppressions=/home/sewardj/MOZ/SUPPS/tsan-empty.txt DISPLAY=:1.0 \
   ./mach xpcshell-test --sequential --log-mach - browser/components/places/tests/unit \
   2>&1 | grep SUMMARY

Comment 8

[Julian Seward [:jseward]]

(In reply to Marco Bonardo [::mak] from comment #5)
> Fwiw, we don't take patches to sqlite in our tree, only officially released
> versions, unless the reasons are REALLY compelling

There is no real intention here to patch the sqlite in our tree, given
that this is fairly harmless; waiting for the next official release is
fine.

To put it more in context though, this is part of a wider initiative
to de-race the tree as much as we can.  And so it is helpful for me to
have the patch itself, just temporarily, so that I don't have to see
this race whilst analysing others.

Comment 9

[Julian Seward [:jseward]]

(In reply to D. Richard Hipp from comment #6)
> The patch is here: https://www.sqlite.org/src/info/e0b116edd64a55c9

Yes, that does seem to stop TSan complaining.  Thank you.

Comment 10

[Julian Seward [:jseward]]

Attachment: bug1256987c10.cset

The patch from comment 6, as it applies to our tree.
DO NOT LAND.

Comment 11

[Julian Seward [:jseward]]

Closing as FIXED, because this is fixed in the sqlite upgrade
that has now landed in bug 1260553.