Fix for issue #28606. Removed loop which iterated over all threads in the thread registry to check …

Fix for issue #28606. Removed loop which iterated over all threads in the thread registry to check if a handle is valid and replaced it with a single check with the current thread.

BUG=
R=johnmccutchan@google.com

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

[bkonyi, 2017-02-02 22:22:33]

bkonyi@google.com changed reviewers:
+ asiva@google.com, johnmccutchan@google.com

[bkonyi, 2017-02-02 22:22:34]

bkonyi@google.com changed reviewers:
+ asiva@google.com, johnmccutchan@google.com

[bkonyi, 2017-02-02 22:22:34]

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

https://codereview.chromium.org/2672833003/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:543: return Thread::Current()->IsValidHandle(handle)
||
I wasn't able to reproduce the issue described in #28606, but I'm fairly certain
that this should resolve the crash caused by the race.

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

https://codereview.chromium.org/2672833003/diff/1/runtime/vm/dart_api_state.h...
runtime/vm/dart_api_state.h:459: mutex_(new Mutex()),
It looks like PersistentHandles contains shared handles within the current
isolate, so I feel there might be the potential for a race condition here. I
noticed FinalizablePersistentHandles does have locks, so I've added them as
well.

[bkonyi, 2017-02-02 22:42:16]

Description was changed from

==========
Fix for issue #28606. Removed loop which iterated over all threads in the thread
registry to check if a handle is valid and replaced it with a single check with
the current thread. Also added locks within PersistentHandle to avoid race
conditions occurring when adding/removing/reading persistent handles.

BUG=
==========

to

==========
Fix for issue #28606. Removed loop which iterated over all threads in the thread
registry to check if a handle is valid and replaced it with a single check with
the current thread.

BUG=
==========

[bkonyi, 2017-02-02 22:43:04]

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

https://codereview.chromium.org/2672833003/diff/1/runtime/vm/dart_api_state.h...
runtime/vm/dart_api_state.h:459: mutex_(new Mutex()),
On 2017/02/02 22:22:34, bkonyi wrote:
> It looks like PersistentHandles contains shared handles within the current
> isolate, so I feel there might be the potential for a race condition here. I
> noticed FinalizablePersistentHandles does have locks, so I've added them as
> well. 

As discussed with Siva, this isn't needed. Removed in patch set 2.

[Cutch, 2017-02-02 22:44:19]

lgtm

[siva, 2017-02-02 22:51:13]

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

https://codereview.chromium.org/2672833003/diff/1/runtime/vm/dart_api_state.h...
runtime/vm/dart_api_state.h:459: mutex_(new Mutex()),
On 2017/02/02 22:22:34, bkonyi wrote:
> It looks like PersistentHandles contains shared handles within the current
> isolate, so I feel there might be the potential for a race condition here. I
> noticed FinalizablePersistentHandles does have locks, so I've added them as
> well. 

FinalizablePersistentHandles has a lock because we have background finalization
done in a different thread.

PersistentHandles should only be accessed from the mutator thread.

https://codereview.chromium.org/2672833003/diff/20001/runtime/vm/dart_api_imp...
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/2672833003/diff/20001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:543: return Thread::Current()->IsValidHandle(handle)
||
Maybe you should add an assert here
Thread* thread = Thread::Current();
ASSERT(thread->IsMutatorThread());
return thread->IsValidHandle.......

[bkonyi, 2017-02-02 23:08:49]

https://codereview.chromium.org/2672833003/diff/20001/runtime/vm/dart_api_imp...
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/2672833003/diff/20001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:543: return Thread::Current()->IsValidHandle(handle)
||
On 2017/02/02 22:51:13, siva wrote:
> Maybe you should add an assert here
> Thread* thread = Thread::Current();
> ASSERT(thread->IsMutatorThread());
> return thread->IsValidHandle.......

Done.

[bkonyi, 2017-02-02 23:09:12]

Description was changed from

==========
Fix for issue #28606. Removed loop which iterated over all threads in the thread
registry to check if a handle is valid and replaced it with a single check with
the current thread.

BUG=
==========

to

==========
Fix for issue #28606. Removed loop which iterated over all threads in the thread
registry to check if a handle is valid and replaced it with a single check with
the current thread.

BUG=
R=johnmccutchan@google.com

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

[bkonyi, 2017-02-02 23:09:14]

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