Fix sticky error propagation from thread to isolate

Fix sticky error propagation from thread to isolate

BUG=
R=asiva@google.com, iposva@google.com

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

[srdjan, 2016-03-07 21:58:06]

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

[siva, 2016-03-08 08:54:59]

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

https://codereview.chromium.org/1769183003/diff/1/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:2595:
thread->isolate()->object_store()->SetStickyErrorFromThread(thread);
If multiple threads exit at the same time won't this  result in the sticky error
being overwritten to the one exiting last.

[srdjan, 2016-03-08 20:21:09]

PTAL

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

https://codereview.chromium.org/1769183003/diff/1/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:2595:
thread->isolate()->object_store()->SetStickyErrorFromThread(thread);
On 2016/03/08 08:54:59, siva wrote:
> If multiple threads exit at the same time won't this  result in the sticky
error
> being overwritten to the one exiting last.

Modified code to allow only mutator thread to propagate sticky error to isolate.
'SetStickyError' checks that a sticky error is not overwritten.

[siva, 2016-03-09 07:07:51]

lgtm

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

https://codereview.chromium.org/1769183003/diff/40001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:1630: T = Thread::Current();
Do we need this here since Api::NewHandle is called in the error path we could
just do:
Dart_Handle error = Api::NewHandle(Thread::Current(), I->o.....);

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

https://codereview.chromium.org/1769183003/diff/40001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:2597: }
Could you add a comment here

[Ivan Posva, 2016-03-09 08:27:17]

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

[Ivan Posva, 2016-03-09 08:27:18]

LGTM with comments addressed.

-Ivan

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

https://codereview.chromium.org/1769183003/diff/40001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:1615: ::Dart_ExitIsolate();
To avoid my question from below we should structure this code as follows to make
sure the lifetime of T is clear.

Isolate* I;
{
  Thread* T = Thread::Current();
  I = T->isolate();
  ...
  ::Dart_ExitIsolate();
}
{
  Monitor monitor;
  MonitorLocker ml(&monitor);
  ...
}
::Dart_EnterIsolate(...);

https://codereview.chromium.org/1769183003/diff/40001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:1630: T = Thread::Current();
On 2016/03/09 07:07:51, siva wrote:
> Do we need this here since Api::NewHandle is called in the error path we could
> just do:
> Dart_Handle error = Api::NewHandle(Thread::Current(), I->o.....);

Why is this even necessary. How can T have changed between the call at the top
of this function and here?

https://codereview.chromium.org/1769183003/diff/40001/runtime/vm/object_store.h
File runtime/vm/object_store.h (right):

https://codereview.chromium.org/1769183003/diff/40001/runtime/vm/object_store...
runtime/vm/object_store.h:371: void SetStickyErrorFromThread(const Thread*
value);
Now we are putting functionality into the ObjectStore, please move this to
wherever this is being called. It is a single spot only as far as I can tell.

[srdjan, 2016-03-09 19:10:15]

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

https://codereview.chromium.org/1769183003/diff/40001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:1615: ::Dart_ExitIsolate();
On 2016/03/09 08:27:18, Ivan Posva wrote:
> To avoid my question from below we should structure this code as follows to
make
> sure the lifetime of T is clear.
> 
> Isolate* I;
> {
>   Thread* T = Thread::Current();
>   I = T->isolate();
>   ...
>   ::Dart_ExitIsolate();
> }
> {
>   Monitor monitor;
>   MonitorLocker ml(&monitor);
>   ...
> }
> ::Dart_EnterIsolate(...);

Adapted to the lifetime of stack objets:

  Isolate* I;
  {
    Thread* T = Thread::Current();
    I = T->isolate();
    CHECK_API_SCOPE(T);
    CHECK_CALLBACK_STATE(T);
  }
  API_TIMELINE_BEGIN_END;
  ::Dart_ExitIsolate();
  {
    Monitor monitor;
    MonitorLocker ml(&monitor);
.....

https://codereview.chromium.org/1769183003/diff/40001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:1630: T = Thread::Current();
On 2016/03/09 07:07:51, siva wrote:
> Do we need this here since Api::NewHandle is called in the error path we could
> just do:
> Dart_Handle error = Api::NewHandle(Thread::Current(), I->o.....);

Done.

https://codereview.chromium.org/1769183003/diff/40001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:1630: T = Thread::Current();
On 2016/03/09 08:27:18, Ivan Posva wrote:
> On 2016/03/09 07:07:51, siva wrote:
> > Do we need this here since Api::NewHandle is called in the error path we
could
> > just do:
> > Dart_Handle error = Api::NewHandle(Thread::Current(), I->o.....);
> 
> Why is this even necessary. How can T have changed between the call at the top
> of this function and here?

::Dart_EnterIsolate(...)  sets new Thread::Current(); incidentally it is the
same thread as in T, since we reuse the just released thread (thread =
GetFromFreelistLocked(isolate)). That is how we were accidentally (but
correctly) passing sticky_error, and why clearing sticky_error when unscheduling
broke code.

[srdjan, 2016-03-09 20:40:49]

Description was changed from

==========
Fix sticky error propagation from thread to isolate

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

to

==========
Fix sticky error propagation from thread to isolate

BUG=
R=asiva@google.com, iposva@google.com

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

[srdjan, 2016-03-09 20:40:51]

Committed patchset #5 (id:80001) manually as
60ce67d91fd8afe46e15e5cd536cd14648635e7b (presubmit successful).