New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 828897 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Fuchsia
Pri: 3
Type: Bug



Sign in to add a comment

New ThreadLocalStorageTest.UseTLSDuringDestruction fails under Fuchsia

Project Member Reported by w...@chromium.org, Apr 4 2018

Issue description

The 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.
 

Comment 1 by w...@chromium.org, Apr 4 2018

Status: Started (was: Assigned)
Project Member

Comment 2 by bugdroid1@chromium.org, 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

Comment 3 by w...@chromium.org, Apr 4 2018

Cc: kulakowski@chromium.org
Owner: erikc...@chromium.org
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?
Owner: ----
Status: Available (was: Started)
> 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.
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 :)
> 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.
Sorry, that last statement was unclear:

Switching from SimpleThread to instead directly use pthread_create should fix the issue.

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].



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.
Owner: erikc...@chromium.org
Status: Started (was: Available)
Since this is probably a bug in my original implementation, I'll take a stab at fixing.
Project Member

Comment 11 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)

Sign in to add a comment