Fix asserts in StackFrameIterator which were effectively disabled

Fix asserts in StackFrameIterator which were effectively disabled

The assertions which tried to assert that we only use
StackFrameIterator to walk frames of the current thread was incorrect.
We already have cases where other threads will walk the stack of the
mutator thread, see below for an example where this can happen.

Thread::VisitObjectPointers was incorrectly passing Thread::Current() to
the StackFrameIterator instead of 'this'.  (Code in thread_registry.cc will
loop over a number of threads and calls VisitObjectPointers on them)

  Mutator thread:

    0  pthread_cond_wait@@GLIBC_2.3.2
    1  dart::Monitor::WaitMicros
    2  dart::Monitor::Wait
    3  dart::MonitorLocker::Wait
    4  dart::ThreadBarrier::Sync
    5  dart::GCMarker::MarkObjects
    6  dart::PageSpace::MarkSweep
    7  dart::Heap::CollectOldSpaceGarbage
    8  dart::Heap::CollectNewSpaceGarbage
    9  dart::Heap::CollectGarbage
    10 dart::DN_HelperObject_<native>
    11 dart::BootstrapNatives::<native>
    <dart frames>

  MarkTask thread:

    1  dart::EntryFrame::VisitObjectPointers
    2  dart::Thread::VisitObjectPointers          <---- Walks mutator thread stack
    3  dart::ThreadRegistry::VisitObjectPointers  <---- Iterates over a number of threads
    4  dart::Isolate::VisitStackPointers
    5  dart::Isolate::VisitObjectPointers
    6  dart::GCMarker::IterateRoots
    7  dart::MarkTask::Run
    8  dart::ThreadPool::Worker::Loop
    9  dart::ThreadPool::Worker::Main
    10 dart::ThreadStart

R=rmacnak@google.com

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

[rmacnak, 2017-04-28 17:52:46]

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

[rmacnak, 2017-04-28 17:52:46]

The exception for Windows is for the profiler, since there the thread
interrupter is the one collecting samples instead of the interrupted thread.
Parallel mark is newer than the profiler, and I think it passes on accident
because of the default thread argument. What we really want to assert is that we
only walk the current thread's stack or the stack of a thread we know is
suspended.

What if we changed StackFrameIterator to remove the default value for the thread
argument and take an additional bool that says whether the thread is suspended?
Only the Windows profiler and the marker tasks would claim the thread is
suspended.

[kustermann, 2017-05-01 15:12:09]

Description was changed from

==========
Remove incorrectly-used AllowStackFrameIteratorFromAnotherThread

The assertion which tried to assert that we only use
StackFrameIterator to walk frames of the current thread was incorrect.
We already have cases where other threads will walk the stack of the
mutator thread, see below for an example where this can happen.

Thread::VisitObjectPointers was incorrectly passing Thread::Current() to
the StackFrameIterator instead of 'this'.  (Code in thread_registry.cc will
loop over a number of threads and calls VisitObjectPointers on them)

  Mutator thread:

    0  pthread_cond_wait@@GLIBC_2.3.2
    1  dart::Monitor::WaitMicros
    2  dart::Monitor::Wait
    3  dart::MonitorLocker::Wait
    4  dart::ThreadBarrier::Sync
    5  dart::GCMarker::MarkObjects
    6  dart::PageSpace::MarkSweep
    7  dart::Heap::CollectOldSpaceGarbage
    8  dart::Heap::CollectNewSpaceGarbage
    9  dart::Heap::CollectGarbage
    10 dart::DN_HelperObject_<native>
    11 dart::BootstrapNatives::<native>
    <dart frames>

  MarkTask thread:

    1  dart::EntryFrame::VisitObjectPointers
    2  dart::Thread::VisitObjectPointers          <---- Walks mutator thread
stack
    3  dart::ThreadRegistry::VisitObjectPointers  <---- Iterates over a number
of threads
    4  dart::Isolate::VisitStackPointers
    5  dart::Isolate::VisitObjectPointers
    6  dart::GCMarker::IterateRoots
    7  dart::MarkTask::Run
    8  dart::ThreadPool::Worker::Loop
    9  dart::ThreadPool::Worker::Main
    10 dart::ThreadStart
==========

to

==========
Fix asserts in StackFrameIterator which were effectively disabled

The assertions which tried to assert that we only use
StackFrameIterator to walk frames of the current thread was incorrect.
We already have cases where other threads will walk the stack of the
mutator thread, see below for an example where this can happen.

Thread::VisitObjectPointers was incorrectly passing Thread::Current() to
the StackFrameIterator instead of 'this'.  (Code in thread_registry.cc will
loop over a number of threads and calls VisitObjectPointers on them)

  Mutator thread:

    0  pthread_cond_wait@@GLIBC_2.3.2
    1  dart::Monitor::WaitMicros
    2  dart::Monitor::Wait
    3  dart::MonitorLocker::Wait
    4  dart::ThreadBarrier::Sync
    5  dart::GCMarker::MarkObjects
    6  dart::PageSpace::MarkSweep
    7  dart::Heap::CollectOldSpaceGarbage
    8  dart::Heap::CollectNewSpaceGarbage
    9  dart::Heap::CollectGarbage
    10 dart::DN_HelperObject_<native>
    11 dart::BootstrapNatives::<native>
    <dart frames>

  MarkTask thread:

    1  dart::EntryFrame::VisitObjectPointers
    2  dart::Thread::VisitObjectPointers          <---- Walks mutator thread
stack
    3  dart::ThreadRegistry::VisitObjectPointers  <---- Iterates over a number
of threads
    4  dart::Isolate::VisitStackPointers
    5  dart::Isolate::VisitObjectPointers
    6  dart::GCMarker::IterateRoots
    7  dart::MarkTask::Run
    8  dart::ThreadPool::Worker::Loop
    9  dart::ThreadPool::Worker::Main
    10 dart::ThreadStart
==========

[kustermann, 2017-05-01 15:12:22]

PTAL

[kustermann, 2017-05-01 15:13:47]

PTAL

[rmacnak, 2017-05-01 15:51:33]

LGTM

https://codereview.chromium.org/2845053003/diff/40001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/2845053003/diff/40001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:1168: StackFrameIterator iterator(false,
Thread::Current(), false);
Here and elsewhere

StackFrameIterator::kDontValidateFrames, ..., ...

[Vyacheslav Egorov (Google), 2017-05-01 15:54:37]

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

[Vyacheslav Egorov (Google), 2017-05-01 15:54:38]

Drive-by suggestion

https://codereview.chromium.org/2845053003/diff/40001/runtime/lib/errors.cc
File runtime/lib/errors.cc (right):

https://codereview.chromium.org/2845053003/diff/40001/runtime/lib/errors.cc#n...
runtime/lib/errors.cc:78: DartFrameIterator iterator(thread, false);
I suggest that this false needs a name. 

thread, kNoCrossThreadIteration
thread, kAllowCrossThreadIteration

reads better than

thread, false
thread, true

[kustermann, 2017-05-02 07:11:18]

https://codereview.chromium.org/2845053003/diff/40001/runtime/lib/errors.cc
File runtime/lib/errors.cc (right):

https://codereview.chromium.org/2845053003/diff/40001/runtime/lib/errors.cc#n...
runtime/lib/errors.cc:78: DartFrameIterator iterator(thread, false);
On 2017/05/01 15:54:38, Vyacheslav Egorov (Google) wrote:
> I suggest that this false needs a name. 
> 
> thread, kNoCrossThreadIteration
> thread, kAllowCrossThreadIteration
> 
> reads better than
> 
> thread, false
> thread, true
> 

Made both bools an enum

https://codereview.chromium.org/2845053003/diff/40001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/2845053003/diff/40001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:1168: StackFrameIterator iterator(false,
Thread::Current(), false);
On 2017/05/01 15:51:33, rmacnak wrote:
> Here and elsewhere
> 
> StackFrameIterator::kDontValidateFrames, ..., ...

Will make it an enum, same for the other boolean.

[kustermann, 2017-05-03 08:27:10]

Description was changed from

==========
Fix asserts in StackFrameIterator which were effectively disabled

The assertions which tried to assert that we only use
StackFrameIterator to walk frames of the current thread was incorrect.
We already have cases where other threads will walk the stack of the
mutator thread, see below for an example where this can happen.

Thread::VisitObjectPointers was incorrectly passing Thread::Current() to
the StackFrameIterator instead of 'this'.  (Code in thread_registry.cc will
loop over a number of threads and calls VisitObjectPointers on them)

  Mutator thread:

    0  pthread_cond_wait@@GLIBC_2.3.2
    1  dart::Monitor::WaitMicros
    2  dart::Monitor::Wait
    3  dart::MonitorLocker::Wait
    4  dart::ThreadBarrier::Sync
    5  dart::GCMarker::MarkObjects
    6  dart::PageSpace::MarkSweep
    7  dart::Heap::CollectOldSpaceGarbage
    8  dart::Heap::CollectNewSpaceGarbage
    9  dart::Heap::CollectGarbage
    10 dart::DN_HelperObject_<native>
    11 dart::BootstrapNatives::<native>
    <dart frames>

  MarkTask thread:

    1  dart::EntryFrame::VisitObjectPointers
    2  dart::Thread::VisitObjectPointers          <---- Walks mutator thread
stack
    3  dart::ThreadRegistry::VisitObjectPointers  <---- Iterates over a number
of threads
    4  dart::Isolate::VisitStackPointers
    5  dart::Isolate::VisitObjectPointers
    6  dart::GCMarker::IterateRoots
    7  dart::MarkTask::Run
    8  dart::ThreadPool::Worker::Loop
    9  dart::ThreadPool::Worker::Main
    10 dart::ThreadStart
==========

to

==========
Fix asserts in StackFrameIterator which were effectively disabled

The assertions which tried to assert that we only use
StackFrameIterator to walk frames of the current thread was incorrect.
We already have cases where other threads will walk the stack of the
mutator thread, see below for an example where this can happen.

Thread::VisitObjectPointers was incorrectly passing Thread::Current() to
the StackFrameIterator instead of 'this'.  (Code in thread_registry.cc will
loop over a number of threads and calls VisitObjectPointers on them)

  Mutator thread:

    0  pthread_cond_wait@@GLIBC_2.3.2
    1  dart::Monitor::WaitMicros
    2  dart::Monitor::Wait
    3  dart::MonitorLocker::Wait
    4  dart::ThreadBarrier::Sync
    5  dart::GCMarker::MarkObjects
    6  dart::PageSpace::MarkSweep
    7  dart::Heap::CollectOldSpaceGarbage
    8  dart::Heap::CollectNewSpaceGarbage
    9  dart::Heap::CollectGarbage
    10 dart::DN_HelperObject_<native>
    11 dart::BootstrapNatives::<native>
    <dart frames>

  MarkTask thread:

    1  dart::EntryFrame::VisitObjectPointers
    2  dart::Thread::VisitObjectPointers          <---- Walks mutator thread
stack
    3  dart::ThreadRegistry::VisitObjectPointers  <---- Iterates over a number
of threads
    4  dart::Isolate::VisitStackPointers
    5  dart::Isolate::VisitObjectPointers
    6  dart::GCMarker::IterateRoots
    7  dart::MarkTask::Run
    8  dart::ThreadPool::Worker::Loop
    9  dart::ThreadPool::Worker::Main
    10 dart::ThreadStart

R=rmacnak@google.com

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

[kustermann, 2017-05-03 08:27:12]

Committed patchset #4 (id:60001) manually as
ac8d2056a3f3dabb29a39c5181b56cf5a212c9b0 (presubmit successful).