Clear EventHandler on Stop, to avoid future usages.

Clear EventHandler on Stop, to avoid future usages.

BUG=
R=fschneider@google.com, kustermann@google.com

Committed: https://code.google.com/p/dart/source/detail?r=25087

[Anders Johnsen, 2013-07-17 08:33:18]

This is a fix for the eventhandler where an isolate can try to send data,
_after_ the main isolate is closed (along with the event-handler).

[Florian Schneider, 2013-07-17 08:38:09]

https://codereview.chromium.org/19546002/diff/1/runtime/bin/eventhandler.cc
File runtime/bin/eventhandler.cc (right):

https://codereview.chromium.org/19546002/diff/1/runtime/bin/eventhandler.cc#n...
runtime/bin/eventhandler.cc:69: MutexLocker locker(mutex_);
Why is a mutex needed here?

https://codereview.chromium.org/19546002/diff/1/runtime/bin/eventhandler.cc#n...
runtime/bin/eventhandler.cc:103: EventHandler* event_handler =
GetEventHandler(handle);
Maybe not such a good idea to have a local variable event_handler with the same
name as a global.

[Florian Schneider, 2013-07-17 08:49:02]

lgtm

[kustermann, 2013-07-17 08:49:46]

https://codereview.chromium.org/19546002/diff/1/runtime/bin/eventhandler.cc
File runtime/bin/eventhandler.cc (right):

https://codereview.chromium.org/19546002/diff/1/runtime/bin/eventhandler.cc#n...
runtime/bin/eventhandler.cc:105: if (event_handler) {
Please add a comment when event_handler is NULL and why it's OK to do nothing in
that case.
In a couple of months nobody will remember why this is here.

[kustermann, 2013-07-17 08:58:02]

newest patch lgtm

https://codereview.chromium.org/19546002/diff/4001/runtime/bin/eventhandler.cc
File runtime/bin/eventhandler.cc (right):

https://codereview.chromium.org/19546002/diff/4001/runtime/bin/eventhandler.c...
runtime/bin/eventhandler.cc:106: event_handler->SendData(id, dart_port, data);
As you/fschneider proposed, you could just do:
...
{
  MutexLocker locker(mutex_);
  event_handler->SendData(id, dart_port, data);
}
to narrow the critical area.

[Anders Johnsen, 2013-07-17 08:59:49]

PTAL

https://codereview.chromium.org/19546002/diff/1/runtime/bin/eventhandler.cc
File runtime/bin/eventhandler.cc (right):

https://codereview.chromium.org/19546002/diff/1/runtime/bin/eventhandler.cc#n...
runtime/bin/eventhandler.cc:69: MutexLocker locker(mutex_);
On 2013/07/17 08:38:09, Florian Schneider wrote:
> Why is a mutex needed here?

Done.

https://codereview.chromium.org/19546002/diff/1/runtime/bin/eventhandler.cc#n...
runtime/bin/eventhandler.cc:103: EventHandler* event_handler =
GetEventHandler(handle);
On 2013/07/17 08:38:09, Florian Schneider wrote:
> Maybe not such a good idea to have a local variable event_handler with the
same
> name as a global.

Done.

https://codereview.chromium.org/19546002/diff/1/runtime/bin/eventhandler.cc#n...
runtime/bin/eventhandler.cc:105: if (event_handler) {
On 2013/07/17 08:49:46, kustermann wrote:
> Please add a comment when event_handler is NULL and why it's OK to do nothing
in
> that case.
> In a couple of months nobody will remember why this is here.

Done.

[Florian Schneider, 2013-07-17 09:00:57]

On 2013/07/17 08:58:02, kustermann wrote:
> newest patch lgtm
> 
> https://codereview.chromium.org/19546002/diff/4001/runtime/bin/eventhandler.cc
> File runtime/bin/eventhandler.cc (right):
> 
>
https://codereview.chromium.org/19546002/diff/4001/runtime/bin/eventhandler.c...
> runtime/bin/eventhandler.cc:106: event_handler->SendData(id, dart_port, data);
> As you/fschneider proposed, you could just do:
> ...
> {
>   MutexLocker locker(mutex_);
>   event_handler->SendData(id, dart_port, data);
> }
> to narrow the critical area.

Suggestion for a future improvement: If it is performance critical, consider
using double-checked locking instead of simple locking.

[kustermann, 2013-07-17 09:05:27]

On 2013/07/17 09:00:57, Florian Schneider wrote:
> On 2013/07/17 08:58:02, kustermann wrote:
> > newest patch lgtm
> > 
> >
https://codereview.chromium.org/19546002/diff/4001/runtime/bin/eventhandler.cc
> > File runtime/bin/eventhandler.cc (right):
> > 
> >
>
https://codereview.chromium.org/19546002/diff/4001/runtime/bin/eventhandler.c...
> > runtime/bin/eventhandler.cc:106: event_handler->SendData(id, dart_port,
data);
> > As you/fschneider proposed, you could just do:
> > ...
> > {
> >   MutexLocker locker(mutex_);
> >   event_handler->SendData(id, dart_port, data);
> > }
> > to narrow the critical area.
> 
> Suggestion for a future improvement: If it is performance critical, consider
> using double-checked locking instead of simple locking.

I thought about it as well, but I think it doesn't matter in this case, because
in 99% of cases, event_handler != NULL.

lgtm again

[Anders Johnsen, 2013-07-17 09:06:15]

Committed patchset #3 manually as r25087 (presubmit successful).