New ThreadLocalStorageTest.UseTLSDuringDestruction fails under Fuchsia |
|||||
Issue descriptionThe new ThreadLocalStorageTest.UseTLSDuringDestruction test fails under Fuchsia. We should verify whether that is due to use of not-implemented pthread features, or reveals bugs in the actual TLS implementation.
,
Apr 4 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/373ee93daffb03a1bb738c49a50e198e5c366f18 commit 373ee93daffb03a1bb738c49a50e198e5c366f18 Author: Wez <wez@chromium.org> Date: Wed Apr 04 15:38:58 2018 Skip ThreadLocalStorageTest.UseTLSDuringDestruction under Fuchsia. This test fails under Fuchsia's implementation of the pthreads API. Bug: 825218, 828897 Change-Id: I33df13b18e295a05acb000db7ebbcc7ea02b1728 Reviewed-on: https://chromium-review.googlesource.com/994341 Commit-Queue: Wez <wez@chromium.org> Reviewed-by: Erik Chen <erikchen@chromium.org> Reviewed-by: Nico Weber <thakis@chromium.org> Cr-Commit-Position: refs/heads/master@{#548072} [modify] https://crrev.com/373ee93daffb03a1bb738c49a50e198e5c366f18/base/threading/thread_local_storage_unittest.cc
,
Apr 4 2018
erikchen: The difference between Linux and Fuchsia seems to be that: 1. Under Linux, all the pthread key destructors are called _after_ Chrome TLS teardown. 2. Under Fuchsia, all the pthread key destructors are called _before_ Chrome TLS teardown. In #2 we therefore never set the |teardown_works_correctly_| bool. I'm not sure what this test is trying to verify, nor how it is intended to work; it seems that you're verifying that the pthread TLS implementation correctly implements destructor iteration, but in that case the test will flake dependent on TLS destructor ordering (of Chrome TLS vs the other slots). Can you advise?
,
Apr 4 2018
> Under Fuchsia, all the pthread key destructors are called _before_ Chrome TLS teardown. Fascinating. In order to fix the test for Fuschia, we'd probably have to modify the test to not use SimpleThread, as I bet something earlier in the thread startup implicitly calls Chrome TLS, and Fuschia appears to behave differently from Linux/macOS/Android in terms of TLS key destructor ordering. If I had to guess, I'd say that Fuschia probably destroys keys in first created first destroyed order, whereas the other OSes probably do last created first destroyed. Given that this only affects Fuschia, and the test doesn't run on the CQ, I'm going to mark as available, for someone on the Fuschia team to deal with.
,
Apr 4 2018
Fuchsia pthread_key_t destructors are run from first-created to last-created, with up to PTHREAD_DESTRUCTOR_ITERATIONS (iirc 4) attempts across the array. I wouldn't know when base::ThreadLocalStorage is created on SimpleThread on Fuchsia, but it's probably not hard to find out :)
,
Apr 4 2018
> Fuchsia pthread_key_t destructors are run from first-created to last-created, with up to PTHREAD_DESTRUCTOR_ITERATIONS (iirc 4) attempts across the array. Ah okay, that confirms my suspicions. Switching from SimpleThread to an API that directly uses pthread_create should fix the issue then.
,
Apr 4 2018
Sorry, that last statement was unclear: Switching from SimpleThread to instead directly use pthread_create should fix the issue.
,
Apr 4 2018
More clarification. The test is associated with a fix to Chrome TLS that adds a DCHECK when Chrome code calls Slot::Get() after Chrome TLS has been destroyed. The test is attempting to do the following: (1) Add some pthread keys (2) Initialize Chrome TLS (3) Add some pthread keys. The test is confirming two things: (a) If a pthread key destructor is called before Chrome TLS has been destroyed, everything should still work (b) Confirm that at least one pthread key destructor is called after Chrome TLS has been destroyed, and confirm that internal::ThreadLocalStorageTestInternal::HasBeenDestroyed() returns true in this case. Here is what I think is happening: The implementation of SimpleThread is secretly calling (0) Initialize Chrome TLS. before any of the actual test code is run. The test happens to work on all other POSIX systems because they run pthread key destructors in last-created-first-destroyed order, and thus (b) is confirmed. This breaks Fuschia because it runs pthread key destructors in first-created-first-destroyed order, and thus (b) is never confirmed. Note that there is no check to confirm that (a) has ever been called [as that was never the point of the test].
,
Apr 4 2018
There are some other theories as well [haven't confirmed]. It's also possible that the ipmlementation of SimpleThread is different on Fuschia, and causes (0) to be invoked, whereas that doesn't happen on other implementations of SimpleThread.
,
Apr 4 2018
Since this is probably a bug in my original implementation, I'll take a stab at fixing.
,
Apr 4 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/236ec81120c5e950fa80ec50e428aeb787af803d commit 236ec81120c5e950fa80ec50e428aeb787af803d Author: erikchen <erikchen@chromium.org> Date: Wed Apr 04 21:29:52 2018 Fix ThreadLocalStorageTest,UseTLSDuringDestruction to work on Fuchsia. The original version of the test used base::SimpleThread, which uses Chrome TLS behind the scenes. This ends up tickling a difference between Fuchsia and other POSIX implementations with respect to the order in which pthread keys are destroyed. The new version of the test uses pthread APIs to avoid this, which fixes the bug. Change-Id: I8eaf3230a31f96f9ee03afa88a1b7ee8b770bb42 BUG: 828897 , 825218 Reviewed-on: https://chromium-review.googlesource.com/996038 Reviewed-by: Erik Chen <erikchen@chromium.org> Reviewed-by: Nico Weber <thakis@chromium.org> Reviewed-by: Wez <wez@chromium.org> Commit-Queue: Erik Chen <erikchen@chromium.org> Cr-Commit-Position: refs/heads/master@{#548204} [modify] https://crrev.com/236ec81120c5e950fa80ec50e428aeb787af803d/base/threading/thread_local_storage_unittest.cc
,
Apr 9 2018
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by w...@chromium.org
, Apr 4 2018