Fix for issue 25236

Fix for issue 25236
- Ensure that no new OSThreads are created once the VM is in shutdown mode
- Do not query thread_list_head_ without a lock (this leads to the race which causes two threads to delete the lock and TLS at the same time)

BUG=25236
R=zra@google.com

Committed: https://github.com/dart-lang/sdk/commit/b40775549f59740c7fb874da734417b9b56cd758

[siva, 2015-12-17 00:45:34]

asiva@google.com changed reviewers:
+ zra@google.com

[Florian Schneider, 2015-12-17 13:33:08]

fschneider@google.com changed reviewers:
+ fschneider@google.com

[Florian Schneider, 2015-12-17 13:33:08]

https://codereview.chromium.org/1537543002/diff/1/runtime/vm/os_thread.cc
File runtime/vm/os_thread.cc (right):

https://codereview.chromium.org/1537543002/diff/1/runtime/vm/os_thread.cc#new...
runtime/vm/os_thread.cc:223: final_thread = !creation_enabled_  &&
(thread_list_head_ == NULL);
It seems DisableOSThreadCreation is never called in the shutdown procedure, so
as a result Cleanup would never be called below.

[zra, 2015-12-17 16:37:59]

https://codereview.chromium.org/1537543002/diff/1/runtime/vm/os_thread.cc
File runtime/vm/os_thread.cc (right):

https://codereview.chromium.org/1537543002/diff/1/runtime/vm/os_thread.cc#new...
runtime/vm/os_thread.cc:162: void OSThread::DisableOSThreadCreation() {
Where is this called?

https://codereview.chromium.org/1537543002/diff/1/runtime/vm/os_thread.cc#new...
runtime/vm/os_thread.cc:180: void OSThread::AddThreadToList(OSThread* thread) {
AddThreadToListLocked?

https://codereview.chromium.org/1537543002/diff/1/runtime/vm/os_thread.h
File runtime/vm/os_thread.h (right):

https://codereview.chromium.org/1537543002/diff/1/runtime/vm/os_thread.h#newc...
runtime/vm/os_thread.h:172: static OSThread* CreateOSThread();
Maybe this should go up at the top of the class where the constructor would
normally go to make it easier to find. Add a comment explaining when NULL can be
returned.

[siva, 2015-12-17 18:19:13]

Addressed comments PTAL.

https://codereview.chromium.org/1537543002/diff/1/runtime/vm/os_thread.cc
File runtime/vm/os_thread.cc (right):

https://codereview.chromium.org/1537543002/diff/1/runtime/vm/os_thread.cc#new...
runtime/vm/os_thread.cc:162: void OSThread::DisableOSThreadCreation() {
On 2015/12/17 16:37:59, zra wrote:
> Where is this called?

Good point, forgot to add the call in Dart::Cleanup.

https://codereview.chromium.org/1537543002/diff/1/runtime/vm/os_thread.cc#new...
runtime/vm/os_thread.cc:180: void OSThread::AddThreadToList(OSThread* thread) {
On 2015/12/17 16:37:59, zra wrote:
> AddThreadToListLocked?

Done.

https://codereview.chromium.org/1537543002/diff/1/runtime/vm/os_thread.cc#new...
runtime/vm/os_thread.cc:223: final_thread = !creation_enabled_  &&
(thread_list_head_ == NULL);
On 2015/12/17 13:33:08, Florian Schneider wrote:
> It seems DisableOSThreadCreation is never called in the shutdown procedure, so
> as a result Cleanup would never be called below.

Good catch, forgot to add the call in Dart::Cleanup.

https://codereview.chromium.org/1537543002/diff/1/runtime/vm/os_thread.h
File runtime/vm/os_thread.h (right):

https://codereview.chromium.org/1537543002/diff/1/runtime/vm/os_thread.h#newc...
runtime/vm/os_thread.h:172: static OSThread* CreateOSThread();
On 2015/12/17 16:37:59, zra wrote:
> Maybe this should go up at the top of the class where the constructor would
> normally go to make it easier to find. Add a comment explaining when NULL can
be
> returned.

Done.

[zra, 2015-12-17 19:01:43]

https://codereview.chromium.org/1537543002/diff/20001/runtime/vm/dart.cc
File runtime/vm/dart.cc (right):

https://codereview.chromium.org/1537543002/diff/20001/runtime/vm/dart.cc#newc...
runtime/vm/dart.cc:227: OSThread::DisableOSThreadCreation();
Should we defend against concurrent/multiple calls to Dart_Cleanup()? We could
catch that here if OSThread creation is already disabled.

https://codereview.chromium.org/1537543002/diff/20001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1537543002/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:880: ASSERT(retval);
I think this should be a FATAL(). Otherwise if Dart_CreateIsolate() on a new
thread races with Dart_Cleanup(), it won't be caught in a Release build.

[zra, 2015-12-17 22:04:15]

https://codereview.chromium.org/1537543002/diff/20001/runtime/vm/os_thread.cc
File runtime/vm/os_thread.cc (right):

https://codereview.chromium.org/1537543002/diff/20001/runtime/vm/os_thread.cc...
runtime/vm/os_thread.cc:134: ASSERT(OSThread::Current() == NULL);
Just noticed this: OSThread::Current() can call CreateAndSetUnknownThread() so
this ASSERT should probably directly call GetCurrentTLS instead.

[siva, 2015-12-17 23:21:14]

https://codereview.chromium.org/1537543002/diff/20001/runtime/vm/dart.cc
File runtime/vm/dart.cc (right):

https://codereview.chromium.org/1537543002/diff/20001/runtime/vm/dart.cc#newc...
runtime/vm/dart.cc:227: OSThread::DisableOSThreadCreation();
On 2015/12/17 19:01:43, zra wrote:
> Should we defend against concurrent/multiple calls to Dart_Cleanup()? We could
> catch that here if OSThread creation is already disabled.

Will probably do that in a different CL, I don't think checking for OSThread
creation is foolproof we can have a racing Dart_Cleanup call before
OSThread::InitOnce is called and we should be able to recognize that as a
preemptive call (not sure what to do here maybe a different error) Vs multiple
calls to Dart_Cleanup.

https://codereview.chromium.org/1537543002/diff/20001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1537543002/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:880: ASSERT(retval);
On 2015/12/17 19:01:43, zra wrote:
> I think this should be a FATAL(). Otherwise if Dart_CreateIsolate() on a new
> thread races with Dart_Cleanup(), it won't be caught in a Release build.

Done.

https://codereview.chromium.org/1537543002/diff/20001/runtime/vm/os_thread.cc
File runtime/vm/os_thread.cc (right):

https://codereview.chromium.org/1537543002/diff/20001/runtime/vm/os_thread.cc...
runtime/vm/os_thread.cc:134: ASSERT(OSThread::Current() == NULL);
On 2015/12/17 22:04:14, zra wrote:
> Just noticed this: OSThread::Current() can call CreateAndSetUnknownThread() so
> this ASSERT should probably directly call GetCurrentTLS instead.

Done.

[zra, 2015-12-17 23:26:10]

lgtm

[siva, 2015-12-18 18:16:31]

Description was changed from

==========
Fix for issue 25236
- Ensure that no new OSThreads are created once the VM is in shutdown mode
- Do not query thread_list_head_ without a lock (this leads to the race which
causes two threads to delete the lock and TLS at the same time)

BUG=25236
==========

to

==========
Fix for issue 25236
- Ensure that no new OSThreads are created once the VM is in shutdown mode
- Do not query thread_list_head_ without a lock (this leads to the race which
causes two threads to delete the lock and TLS at the same time)

BUG=25236
R=zra@google.com

Committed:
https://github.com/dart-lang/sdk/commit/b40775549f59740c7fb874da734417b9b56cd758
==========

[siva, 2015-12-18 18:16:34]

Committed patchset #3 (id:40001) manually as
b40775549f59740c7fb874da734417b9b56cd758 (presubmit successful).