Fix race in deletion of native message handlers (fixes #28484).

Fix race in deletion of native message handlers (fixes #28484).

Fix leak of snapshot HeapPages.

R=fschneider@google.com

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

[rmacnak, 2017-01-24 01:46:39]

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

[Florian Schneider, 2017-01-24 02:06:07]

Lgtm

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

https://codereview.chromium.org/2650583006/diff/1/runtime/vm/pages.cc#newcode100
runtime/vm/pages.cc:100: free(this);
Maybe change this to new/delete instead of malloc/free.

[siva, 2017-01-24 03:18:41]

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

[siva, 2017-01-24 03:18:42]

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

https://codereview.chromium.org/2650583006/diff/1/runtime/vm/message_handler....
runtime/vm/message_handler.cc:435: end_callback_(callback_data_);
If the handler is deleted wouldn't end_callback_ be garbage at this point? (i.e
the handler was deleted after we released the monitor above).

[rmacnak, 2017-01-24 03:28:36]

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

https://codereview.chromium.org/2650583006/diff/1/runtime/vm/message_handler....
runtime/vm/message_handler.cc:435: end_callback_(callback_data_);
On 2017/01/24 03:18:42, siva wrote:
> If the handler is deleted wouldn't end_callback_ be garbage at this point?
(i.e
> the handler was deleted after we released the monitor above).

Those cases are mutually exclusive. Only a native message handler may be deleted
here, and they never have an end callback.

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

https://codereview.chromium.org/2650583006/diff/1/runtime/vm/pages.cc#newcode100
runtime/vm/pages.cc:100: free(this);
On 2017/01/24 02:06:07, Florian Schneider wrote:
> Maybe change this to new/delete instead of malloc/free.

Trying this out, I see the original reason this uses malloc is to avoid removing
DISALLOW_ALLOCATION, etc from HeapPage. Doing so would give a false impression
that the constructor always runs, which it still wouldn't when normal heap pages
are allocated by reinterpret-casting the  mmap'd page.

[rmacnak, 2017-01-24 03:46:07]

Description was changed from

==========
Fix race in deletion of native message handlers (fixes #28484).

Fix leak of snapshot HeapPages.
==========

to

==========
Fix race in deletion of native message handlers (fixes #28484).

Fix leak of snapshot HeapPages.

R=fschneider@google.com

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

[rmacnak, 2017-01-24 03:46:09]

Committed patchset #1 (id:1) manually as
4dc2677859de3b218864d0e7150684fdc40e0884 (presubmit successful).

[Cutch, 2017-01-24 15:00:48]

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

[Cutch, 2017-01-24 15:00:48]

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

https://codereview.chromium.org/2650583006/diff/1/runtime/vm/message_handler....
runtime/vm/message_handler.cc:435: end_callback_(callback_data_);
On 2017/01/24 03:28:36, rmacnak wrote:
> On 2017/01/24 03:18:42, siva wrote:
> > If the handler is deleted wouldn't end_callback_ be garbage at this point?
> (i.e
> > the handler was deleted after we released the monitor above).
> 
> Those cases are mutually exclusive. Only a native message handler may be
deleted
> here, and they never have an end callback.

DBC-

Can you add an ASSERT that checks for this mutual exclusivity?

[siva, 2017-01-24 15:58:26]

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

https://codereview.chromium.org/2650583006/diff/1/runtime/vm/message_handler....
runtime/vm/message_handler.cc:435: end_callback_(callback_data_);
On 2017/01/24 03:28:36, rmacnak wrote:
> On 2017/01/24 03:18:42, siva wrote:
> > If the handler is deleted wouldn't end_callback_ be garbage at this point?
> (i.e
> > the handler was deleted after we released the monitor above).
> 
> Those cases are mutually exclusive. Only a native message handler may be
deleted
> here, and they never have an end callback.

In that case why not store end_callback_ into a local variable (end_callback)
above when holding the monitor and then do the callback using that local
ASSERT(!delete_me || end_callback == NULL);
if (end_callback != NULL) {
  ...
}

[Cutch, 2017-01-24 17:01:27]

On 2017/01/24 15:58:26, siva wrote:
>
https://codereview.chromium.org/2650583006/diff/1/runtime/vm/message_handler.cc
> File runtime/vm/message_handler.cc (right):
> 
>
https://codereview.chromium.org/2650583006/diff/1/runtime/vm/message_handler....
> runtime/vm/message_handler.cc:435: end_callback_(callback_data_);
> On 2017/01/24 03:28:36, rmacnak wrote:
> > On 2017/01/24 03:18:42, siva wrote:
> > > If the handler is deleted wouldn't end_callback_ be garbage at this point?
> > (i.e
> > > the handler was deleted after we released the monitor above).
> > 
> > Those cases are mutually exclusive. Only a native message handler may be
> deleted
> > here, and they never have an end callback.
> 
> In that case why not store end_callback_ into a local variable (end_callback)
> above when holding the monitor and then do the callback using that local
> ASSERT(!delete_me || end_callback == NULL);
> if (end_callback != NULL) {
>   ...
> }

Done here: https://codereview.chromium.org/2656613003/