[wasm] Managed<T> ensures T's lifetime does not leak past Isolate's

[wasm] Managed<T> ensures T's lifetime does not leak past Isolate's

Native resources allocated by v8, as internal implementation detail,
and held by a Foreign object, must be released when the Isolate is 
torn down. Example: wasm::WasmModule allocated by wasm compile, and
held throughout the lifetime of the WebAssembly.Module object.

This change:
- Extends Managed<CppType> with a mechanism for doing just that
- Separates the role of Managed<CppType> to be strictly an owner of
the lifetime of the native resource. For cases where that's not
desirable, we can polymorphically use Foregin.
- moves managed.h out of wasm, since it's not wasm-specific.

BUG=680065
Review-Url: https://codereview.chromium.org/2676513008
Cr-Commit-Position: refs/heads/master@{#43350}
Committed: https://chromium.googlesource.com/v8/v8/+/caa1d4b262d2e97ce3b9f4163de46f230a8a9929

[Mircea Trofin, 2017-02-04 18:02:54]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-04 18:02:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-04 18:28:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-04 18:28:29]

Dry run: This issue passed the CQ dry run.

[Mircea Trofin, 2017-02-07 05:24:29]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 05:24:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 05:42:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 05:42:52]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[Mircea Trofin, 2017-02-07 22:25:02]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 22:25:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mircea Trofin, 2017-02-07 22:35:56]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 22:36:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 23:11:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 23:11:45]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)

[Mircea Trofin, 2017-02-08 00:50:55]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 00:51:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 01:24:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 01:24:19]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)

[Mircea Trofin, 2017-02-08 05:01:03]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 05:01:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 05:37:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 05:37:19]

Dry run: This issue passed the CQ dry run.

[titzer, 2017-02-08 23:32:47]

titzer@chromium.org changed reviewers:
+ titzer@chromium.org

[titzer, 2017-02-08 23:32:48]

https://codereview.chromium.org/2676513008/diff/100001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2676513008/diff/100001/src/isolate.cc#newcode...
src/isolate.cc:2045: void Isolate::ManagedLifeline::Clear() {
Something seems weird with this. Why are we using a both a destructor and an
explicit clear? Why not just remove the destructor and put it all into clear?

https://codereview.chromium.org/2676513008/diff/100001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/2676513008/diff/100001/src/isolate.h#newcode1206
src/isolate.h:1206: class ManagedLifeline final {
ManagedObjectList?

https://codereview.chromium.org/2676513008/diff/100001/src/isolate.h#newcode1206
src/isolate.h:1206: class ManagedLifeline final {
Please just make this a struct, forward declare it, and put it in the isolate.cc
file.

https://codereview.chromium.org/2676513008/diff/100001/src/isolate.h#newcode1216
src/isolate.h:1216: : prev_(nullptr), next_(nullptr), deleter_(nullptr),
value_(nullptr) {}
I think you can use the inline initializer for these fields now.

https://codereview.chromium.org/2676513008/diff/100001/src/wasm/managed.h
File src/wasm/managed.h (right):

https://codereview.chromium.org/2676513008/diff/100001/src/wasm/managed.h#new...
src/wasm/managed.h:41: static Handle<Managed<CppType>> NewForTesting(Isolate*
isolate,
Do we really need to have a separate constructor for testing? What is the harm
of registering the weak callback for delete for testing too?

[Mircea Trofin, 2017-02-08 23:52:36]

https://codereview.chromium.org/2676513008/diff/100001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2676513008/diff/100001/src/isolate.cc#newcode...
src/isolate.cc:2045: void Isolate::ManagedLifeline::Clear() {
On 2017/02/08 23:32:47, titzer wrote:
> Something seems weird with this. Why are we using a both a destructor and an
> explicit clear? Why not just remove the destructor and put it all into clear?

This clears the whole list. The destructor deletes a node and may be invoked
either by finalizer or when the list is cleared at Teardown

https://codereview.chromium.org/2676513008/diff/100001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/2676513008/diff/100001/src/isolate.h#newcode1206
src/isolate.h:1206: class ManagedLifeline final {
On 2017/02/08 23:32:47, titzer wrote:
> Please just make this a struct, forward declare it, and put it in the
isolate.cc
> file.

The user of this needs to call the value() API (see managed.h, get()).

https://codereview.chromium.org/2676513008/diff/100001/src/isolate.h#newcode1206
src/isolate.h:1206: class ManagedLifeline final {
On 2017/02/08 23:32:48, titzer wrote:
> ManagedObjectList?

Maybe - although the fact it's a list is an implementation detail to its
consumer. The consumer sees it as something that it can get a value from and can
be deleted. It's a handle, rather, but that term is something else in V8 :)

https://codereview.chromium.org/2676513008/diff/100001/src/isolate.h#newcode1216
src/isolate.h:1216: : prev_(nullptr), next_(nullptr), deleter_(nullptr),
value_(nullptr) {}
On 2017/02/08 23:32:47, titzer wrote:
> I think you can use the inline initializer for these fields now.

Acknowledged.

https://codereview.chromium.org/2676513008/diff/100001/src/wasm/managed.h
File src/wasm/managed.h (right):

https://codereview.chromium.org/2676513008/diff/100001/src/wasm/managed.h#new...
src/wasm/managed.h:41: static Handle<Managed<CppType>> NewForTesting(Isolate*
isolate,
On 2017/02/08 23:32:48, titzer wrote:
> Do we really need to have a separate constructor for testing? What is the harm
> of registering the weak callback for delete for testing too?

Not super convinced of the cleanliness of the case that uses the Testing API
(the non-deleting case), to be honest. We should take a look seaparately. In the
meantime, I wanted to separate that one case in cctest out.

My specific concern is: what is the role of Managed<T>, if auto-cleanup isn't
it?

[titzer, 2017-02-09 04:25:18]

https://codereview.chromium.org/2676513008/diff/100001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2676513008/diff/100001/src/isolate.cc#newcode...
src/isolate.cc:2045: void Isolate::ManagedLifeline::Clear() {
On 2017/02/08 23:52:35, Mircea Trofin wrote:
> On 2017/02/08 23:32:47, titzer wrote:
> > Something seems weird with this. Why are we using a both a destructor and an
> > explicit clear? Why not just remove the destructor and put it all into
clear?
> 
> This clears the whole list. The destructor deletes a node and may be invoked
> either by finalizer or when the list is cleared at Teardown

Sure, but I don't think it's necessary to have a destructor do either of those
jobs. You can just have a struct that forms the linked list. In the call site in
the code below, you can just iterate over the whole list and delete() both the
managed object and the link in the list.

[Mircea Trofin, 2017-02-09 04:28:00]

On 2017/02/09 04:25:18, titzer wrote:
> https://codereview.chromium.org/2676513008/diff/100001/src/isolate.cc
> File src/isolate.cc (right):
> 
>
https://codereview.chromium.org/2676513008/diff/100001/src/isolate.cc#newcode...
> src/isolate.cc:2045: void Isolate::ManagedLifeline::Clear() {
> On 2017/02/08 23:52:35, Mircea Trofin wrote:
> > On 2017/02/08 23:32:47, titzer wrote:
> > > Something seems weird with this. Why are we using a both a destructor and
an
> > > explicit clear? Why not just remove the destructor and put it all into
> clear?
> > 
> > This clears the whole list. The destructor deletes a node and may be invoked
> > either by finalizer or when the list is cleared at Teardown
> 
> Sure, but I don't think it's necessary to have a destructor do either of those
> jobs. You can just have a struct that forms the linked list. In the call site
in
> the code below, you can just iterate over the whole list and delete() both the
> managed object and the link in the list.

Why iterate in the GC case (O(N)), when we can promptly clear just the specific
entry?

[titzer, 2017-02-09 17:39:21]

On 2017/02/09 04:28:00, Mircea Trofin wrote:
> On 2017/02/09 04:25:18, titzer wrote:
> > https://codereview.chromium.org/2676513008/diff/100001/src/isolate.cc
> > File src/isolate.cc (right):
> > 
> >
>
https://codereview.chromium.org/2676513008/diff/100001/src/isolate.cc#newcode...
> > src/isolate.cc:2045: void Isolate::ManagedLifeline::Clear() {
> > On 2017/02/08 23:52:35, Mircea Trofin wrote:
> > > On 2017/02/08 23:32:47, titzer wrote:
> > > > Something seems weird with this. Why are we using a both a destructor
and
> an
> > > > explicit clear? Why not just remove the destructor and put it all into
> > clear?
> > > 
> > > This clears the whole list. The destructor deletes a node and may be
invoked
> > > either by finalizer or when the list is cleared at Teardown
> > 
> > Sure, but I don't think it's necessary to have a destructor do either of
those
> > jobs. You can just have a struct that forms the linked list. In the call
site
> in
> > the code below, you can just iterate over the whole list and delete() both
the
> > managed object and the link in the list.
> 
> Why iterate in the GC case (O(N)), when we can promptly clear just the
specific
> entry?

I meant the call site of Clear(), in isolate shutdown. In the GC case, yes, you
can just remove the linked list entry.
But in the Clear() case, you can just iterate them all. No need for a
destructor. Think good ole C.

[Mircea Trofin, 2017-02-09 17:43:25]

On 2017/02/09 17:39:21, titzer wrote:
> On 2017/02/09 04:28:00, Mircea Trofin wrote:
> > On 2017/02/09 04:25:18, titzer wrote:
> > > https://codereview.chromium.org/2676513008/diff/100001/src/isolate.cc
> > > File src/isolate.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2676513008/diff/100001/src/isolate.cc#newcode...
> > > src/isolate.cc:2045: void Isolate::ManagedLifeline::Clear() {
> > > On 2017/02/08 23:52:35, Mircea Trofin wrote:
> > > > On 2017/02/08 23:32:47, titzer wrote:
> > > > > Something seems weird with this. Why are we using a both a destructor
> and
> > an
> > > > > explicit clear? Why not just remove the destructor and put it all into
> > > clear?
> > > > 
> > > > This clears the whole list. The destructor deletes a node and may be
> invoked
> > > > either by finalizer or when the list is cleared at Teardown
> > > 
> > > Sure, but I don't think it's necessary to have a destructor do either of
> those
> > > jobs. You can just have a struct that forms the linked list. In the call
> site
> > in
> > > the code below, you can just iterate over the whole list and delete() both
> the
> > > managed object and the link in the list.
> > 
> > Why iterate in the GC case (O(N)), when we can promptly clear just the
> specific
> > entry?
> 
> I meant the call site of Clear(), in isolate shutdown. In the GC case, yes,
you
> can just remove the linked list entry.
> But in the Clear() case, the  you can just iterate them all. No need for a
> destructor. Think good ole C.

Ah, yes. I think I see what you mean.

[Mircea Trofin, 2017-02-10 23:20:54]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 23:20:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 23:22:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-10 23:22:11]

Dry run: Try jobs failed on following builders:
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/17263)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/34519)

[Mircea Trofin, 2017-02-10 23:24:26]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 23:24:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mircea Trofin, 2017-02-11 00:06:48]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-11 00:06:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mircea Trofin, 2017-02-11 00:12:31]

Description was changed from

==========
[wasm] Managed is always to be managed by runtime

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

to

==========
[wasm] Managed<CppType> ensures lifetime does not extend past Isolate's

Native resources allocated by v8, as internal implementation detail,
and held by a Foreign object, must be released when the Isolate is 
torn down. Example: wasm::WasmModule allocated by wasm compile, and
held throughout the lifetime of the WebAssembly.Module object.

This change:
- Extends Managed<CppType> with a mechanism for doing just that
- Separates the role of Managed<CppType> to be strictly an owner of
the lifetime of the native resource. For cases where that's not
desirable, we can polymorphically use Foregin.
- moves managed.h out of wasm, since it's not wasm-specific.

BUG=680065
==========

[Mircea Trofin, 2017-02-11 00:12:31]

mtrofin@chromium.org changed reviewers:
+ bradnelson@chromium.org

[Mircea Trofin, 2017-02-11 00:13:20]

Description was changed from

==========
[wasm] Managed<CppType> ensures lifetime does not extend past Isolate's

Native resources allocated by v8, as internal implementation detail,
and held by a Foreign object, must be released when the Isolate is 
torn down. Example: wasm::WasmModule allocated by wasm compile, and
held throughout the lifetime of the WebAssembly.Module object.

This change:
- Extends Managed<CppType> with a mechanism for doing just that
- Separates the role of Managed<CppType> to be strictly an owner of
the lifetime of the native resource. For cases where that's not
desirable, we can polymorphically use Foregin.
- moves managed.h out of wasm, since it's not wasm-specific.

BUG=680065
==========

to

==========
[wasm] Managed<T> ensures T's lifetime does not leak past Isolate's

Native resources allocated by v8, as internal implementation detail,
and held by a Foreign object, must be released when the Isolate is 
torn down. Example: wasm::WasmModule allocated by wasm compile, and
held throughout the lifetime of the WebAssembly.Module object.

This change:
- Extends Managed<CppType> with a mechanism for doing just that
- Separates the role of Managed<CppType> to be strictly an owner of
the lifetime of the native resource. For cases where that's not
desirable, we can polymorphically use Foregin.
- moves managed.h out of wasm, since it's not wasm-specific.

BUG=680065
==========

[commit-bot: I haz the power, 2017-02-11 00:35:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-11 00:35:17]

Dry run: This issue passed the CQ dry run.

[Eric Holk, 2017-02-11 02:19:29]

eholk@chromium.org changed reviewers:
+ eholk@chromium.org

[Eric Holk, 2017-02-11 02:19:30]

https://codereview.chromium.org/2676513008/diff/160001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2676513008/diff/160001/src/isolate.cc#newcode...
src/isolate.cc:2033: delete current;
Out of curiosity, would it make sense to just rely on current's destructor
rather than having a separate Deleter?

[Mircea Trofin, 2017-02-11 02:29:32]

https://codereview.chromium.org/2676513008/diff/160001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2676513008/diff/160001/src/isolate.cc#newcode...
src/isolate.cc:2033: delete current;
On 2017/02/11 02:19:29, Eric Holk wrote:
> Out of curiosity, would it make sense to just rely on current's destructor
> rather than having a separate Deleter?

You mean, instead of a separate Dispose?

https://codereview.chromium.org/2676513008/diff/160001/src/isolate.cc#newcode...
src/isolate.cc:2033: delete current;
On 2017/02/11 02:19:29, Eric Holk wrote:
> Out of curiosity, would it make sense to just rely on current's destructor
> rather than having a separate Deleter?

You mean, instead of a separate Dispose?

[Michael Lippautz, 2017-02-11 12:49:17]

mlippautz@chromium.org changed reviewers:
+ mlippautz@chromium.org

[Michael Lippautz, 2017-02-11 12:49:18]

dbc: long shot, but could we create a unit test for this one? If not, let's move
the cctest out of wasm?

[Mircea Trofin, 2017-02-13 17:03:47]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 17:03:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mircea Trofin, 2017-02-13 17:04:51]

On 2017/02/11 12:49:18, Michael Lippautz wrote:
> dbc: long shot, but could we create a unit test for this one? If not, let's
move
> the cctest out of wasm?

Good suggestion, done.

[commit-bot: I haz the power, 2017-02-13 17:09:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-13 17:09:27]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/20905)
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/3...)

[Mircea Trofin, 2017-02-13 19:10:59]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 19:11:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-13 19:28:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-13 19:28:55]

Dry run: Try jobs failed on following builders:
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[Mircea Trofin, 2017-02-13 21:23:11]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 21:23:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-13 21:29:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-13 21:29:33]

Dry run: Try jobs failed on following builders:
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/3...)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[Mircea Trofin, 2017-02-13 21:55:06]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 21:55:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Holk, 2017-02-13 21:55:35]

https://codereview.chromium.org/2676513008/diff/160001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2676513008/diff/160001/src/isolate.cc#newcode...
src/isolate.cc:2033: delete current;
On 2017/02/11 02:29:32, Mircea Trofin wrote:
> On 2017/02/11 02:19:29, Eric Holk wrote:
> > Out of curiosity, would it make sense to just rely on current's destructor
> > rather than having a separate Deleter?
> 
> You mean, instead of a separate Dispose?

Sure, although I was referring to the Deleter in isolate.h, which seems to
include a Dispose method.

[titzer, 2017-02-13 21:58:25]

https://codereview.chromium.org/2676513008/diff/240001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/2676513008/diff/240001/src/isolate.h#newcode1214
src/isolate.h:1214: class ManagedLifeline final {
Can we just make this a struct and hide it in the .cc file? Then the members
don't even need to be private, since it can't be seen outside the .cc file.

[commit-bot: I haz the power, 2017-02-13 22:04:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-13 22:04:54]

Dry run: Try jobs failed on following builders:
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[Mircea Trofin, 2017-02-14 02:59:22]

On 2017/02/13 17:04:51, Mircea Trofin wrote:
> On 2017/02/11 12:49:18, Michael Lippautz wrote:
> > dbc: long shot, but could we create a unit test for this one? If not, let's
> move
> > the cctest out of wasm?
> 
> Good suggestion, done.

ugh... hitting issues with Windows. Until I get a windows setup (I don't have
one), I opened v8:5966, and will just move the cctest out of wasm, for now.

[Mircea Trofin, 2017-02-14 03:25:05]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 03:25:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 03:58:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-14 03:58:44]

Dry run: Try jobs failed on following builders:
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/20875)
  v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)

[Mircea Trofin, 2017-02-14 04:22:09]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 04:22:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 04:50:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-14 04:50:16]

Dry run: This issue passed the CQ dry run.

[Mircea Trofin, 2017-02-14 04:50:55]

https://codereview.chromium.org/2676513008/diff/240001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/2676513008/diff/240001/src/isolate.h#newcode1214
src/isolate.h:1214: class ManagedLifeline final {
On 2017/02/13 21:58:25, titzer wrote:
> Can we just make this a struct and hide it in the .cc file? Then the members
> don't even need to be private, since it can't be seen outside the .cc file.

That would mean:
- changing managed_lifelines_root_ to a ManagedLifeline*
- figuring out its allocation/release. We want it to be an empty head, 
because it makes the insertion/removal logic simpler. But now we need 
to track its lifetime. Not that it's difficult, but adds some 
complexity
- adding a forward declaration. The C++ style guide discourages that: 
https://google.github.io/styleguide/cppguide.html#Forward_Declarations
- adding an Isolate member to the tune of "void 
DisposeLifeline(ManagedLifeline* value)" or a bool option to 
UnregisterFromReleaseAtTeardown to say "also delete" or just make that 
latter method also delete.


The current CL fits into the style of isolate.h - there are 
similar other inner classes. The proposed new pattern would:
- change that for this one case;
- add some complexity, albeit small (lines 1&2 above)
- sidestep a style guide

Unless there's a strong motivation for doing this, I'd rather not.
Also, if there is a strong motivation for going in the private, should
it be also applied to the other inner types in this file, or 
elsewhere?

[Mircea Trofin, 2017-02-14 15:53:27]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 15:53:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 16:19:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-14 16:19:02]

Dry run: This issue passed the CQ dry run.

[Michael Lippautz, 2017-02-15 17:30:54]

On 2017/02/14 02:59:22, Mircea Trofin wrote:
> On 2017/02/13 17:04:51, Mircea Trofin wrote:
> > On 2017/02/11 12:49:18, Michael Lippautz wrote:
> > > dbc: long shot, but could we create a unit test for this one? If not,
let's
> > move
> > > the cctest out of wasm?
> > 
> > Good suggestion, done.
> 
> ugh... hitting issues with Windows. Until I get a windows setup (I don't have
> one), I opened v8:5966, and will just move the cctest out of wasm, for now.

In case you want to try, I think you would've needed something along the lines
of 
  template <class CppType>
  class V8_EXPORT_PRIVATE Managed : NON_EXPORTED_BASE(public Foreign) {};

But cctest is anways good enough :)

[Mircea Trofin, 2017-02-15 17:35:32]

On 2017/02/15 17:30:54, Michael Lippautz wrote:
> On 2017/02/14 02:59:22, Mircea Trofin wrote:
> > On 2017/02/13 17:04:51, Mircea Trofin wrote:
> > > On 2017/02/11 12:49:18, Michael Lippautz wrote:
> > > > dbc: long shot, but could we create a unit test for this one? If not,
> let's
> > > move
> > > > the cctest out of wasm?
> > > 
> > > Good suggestion, done.
> > 
> > ugh... hitting issues with Windows. Until I get a windows setup (I don't
have
> > one), I opened v8:5966, and will just move the cctest out of wasm, for now.
> 
> In case you want to try, I think you would've needed something along the lines
> of 
>   template <class CppType>
>   class V8_EXPORT_PRIVATE Managed : NON_EXPORTED_BASE(public Foreign) {};
> 
> But cctest is anways good enough :)

I tried that in the preceding change to the one I "gave up", no luck. I need to
get a windows setup to be productive on that front. Once I got that, should be
quick to move to unittest-it makes sense for these tests to be there, as you
suggested.

[bradnelson, 2017-02-16 23:59:18]

bradnelson@chromium.org changed reviewers:
+ mvstanton@chromium.org

[bradnelson, 2017-02-16 23:59:20]

lgtm

+mvstanton as we're adding something on the isolate, can you take a look?

Thanks

https://codereview.chromium.org/2676513008/diff/300001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2676513008/diff/300001/src/isolate.cc#newcode...
src/isolate.cc:2035: current != nullptr;) {
The for loop vs while obscures this slightly, maybe a while?

https://codereview.chromium.org/2676513008/diff/300001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/2676513008/diff/300001/src/isolate.h#newcode1212
src/isolate.h:1212: // implementation
Reflow the text.

[Mircea Trofin, 2017-02-21 03:39:50]

The CQ bit was checked by mtrofin@chromium.org

[commit-bot: I haz the power, 2017-02-21 03:39:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 04:07:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-21 04:07:28]

Failed to apply patch for BUILD.gn:
While running git apply --index -p1;
  error: patch failed: BUILD.gn:2564
  error: BUILD.gn: patch does not apply

Patch:       BUILD.gn
Index: BUILD.gn
diff --git a/BUILD.gn b/BUILD.gn
index
e79dd562bb6393fe31ae6883bcfe518f0467ea3b..4e232d224a189ec6972fca8e584ffd010a53c3a0
100644
--- a/BUILD.gn
+++ b/BUILD.gn
@@ -1576,6 +1576,7 @@ v8_source_set("v8_base") {
     "src/machine-type.cc",
     "src/machine-type.h",
     "src/macro-assembler.h",
+    "src/managed.h",
     "src/map-updater.cc",
     "src/map-updater.h",
     "src/messages.cc",
@@ -1807,7 +1808,6 @@ v8_source_set("v8_base") {
     "src/wasm/function-body-decoder.cc",
     "src/wasm/function-body-decoder.h",
     "src/wasm/leb-helper.h",
-    "src/wasm/managed.h",
     "src/wasm/module-decoder.cc",
     "src/wasm/module-decoder.h",
     "src/wasm/signature-map.cc",
@@ -2564,8 +2564,8 @@ group("v8_clusterfuzz") {
   if (v8_multi_arch_build) {
     deps += [
       ":d8(//build/toolchain/linux:clang_x64)",
-      ":d8(//build/toolchain/linux:clang_x86)",
       ":d8(//build/toolchain/linux:clang_x64_v8_arm64)",
+      ":d8(//build/toolchain/linux:clang_x86)",
       ":d8(//build/toolchain/linux:clang_x86_v8_arm)",
     ]
   }

[Mircea Trofin, 2017-02-21 04:27:27]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 04:27:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mircea Trofin, 2017-02-21 04:38:54]

Incorporated bradnelson's feedback.

Also, gentle nudge to mvstanton.

Thanks!

[commit-bot: I haz the power, 2017-02-21 04:54:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-21 04:54:04]

Dry run: This issue passed the CQ dry run.

[mvstanton, 2017-02-21 09:21:50]

LGTM, thanks!

[titzer, 2017-02-21 09:30:08]

https://codereview.chromium.org/2676513008/diff/320001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/2676513008/diff/320001/src/isolate.h#newcode1215
src/isolate.h:1215: class ManagedLifeline final {
Maybe a better name here? "Lifeline" feels like it is introducing a technical
term, when it really shouldn't.

[Mircea Trofin, 2017-02-21 15:52:28]

https://codereview.chromium.org/2676513008/diff/320001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/2676513008/diff/320001/src/isolate.h#newcode1215
src/isolate.h:1215: class ManagedLifeline final {
On 2017/02/21 09:30:08, titzer wrote:
> Maybe a better name here? "Lifeline" feels like it is introducing a technical
> term, when it really shouldn't.

How about "NativeObjectLifetime{Manager|Handler}"

or 

"NativeObjectFinalizer"

[titzer, 2017-02-21 16:33:10]

On 2017/02/21 15:52:28, Mircea Trofin wrote:
> https://codereview.chromium.org/2676513008/diff/320001/src/isolate.h
> File src/isolate.h (right):
> 
>
https://codereview.chromium.org/2676513008/diff/320001/src/isolate.h#newcode1215
> src/isolate.h:1215: class ManagedLifeline final {
> On 2017/02/21 09:30:08, titzer wrote:
> > Maybe a better name here? "Lifeline" feels like it is introducing a
technical
> > term, when it really shouldn't.
> 
> How about "NativeObjectLifetime{Manager|Handler}"
> 
> or 
> 
> "NativeObjectFinalizer"

NativeObjectFinalizer sounds ok, or even ManagedObjectFinalizer, to make the
more explicit connection between this and Managed<T>.

[Mircea Trofin, 2017-02-21 16:41:04]

On 2017/02/21 16:33:10, titzer wrote:
> On 2017/02/21 15:52:28, Mircea Trofin wrote:
> > https://codereview.chromium.org/2676513008/diff/320001/src/isolate.h
> > File src/isolate.h (right):
> > 
> >
>
https://codereview.chromium.org/2676513008/diff/320001/src/isolate.h#newcode1215
> > src/isolate.h:1215: class ManagedLifeline final {
> > On 2017/02/21 09:30:08, titzer wrote:
> > > Maybe a better name here? "Lifeline" feels like it is introducing a
> technical
> > > term, when it really shouldn't.
> > 
> > How about "NativeObjectLifetime{Manager|Handler}"
> > 
> > or 
> > 
> > "NativeObjectFinalizer"
> 
> NativeObjectFinalizer sounds ok, or even ManagedObjectFinalizer, to make the
> more explicit connection between this and Managed<T>.

Good point! ManagedObjectFinalizer then.

[Mircea Trofin, 2017-02-21 16:56:01]

The CQ bit was checked by mtrofin@chromium.org

[Mircea Trofin, 2017-02-21 16:56:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
bradnelson@chromium.org, mvstanton@chromium.org
Link to the patchset: https://codereview.chromium.org/2676513008/#ps360001
(title: "renamed to ManagedObjectFinalizer, and using "finalizer"`")

[commit-bot: I haz the power, 2017-02-21 16:56:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 17:23:39]

CQ is committing da patch.

Bot data: {"patchset_id": 360001, "attempt_start_ts": 1487696161359570,
"parent_rev": "a49ff6abb76370114106ca3516850496c96e9c73", "commit_rev":
"caa1d4b262d2e97ce3b9f4163de46f230a8a9929"}

[commit-bot: I haz the power, 2017-02-21 17:23:45]

Description was changed from

==========
[wasm] Managed<T> ensures T's lifetime does not leak past Isolate's

Native resources allocated by v8, as internal implementation detail,
and held by a Foreign object, must be released when the Isolate is 
torn down. Example: wasm::WasmModule allocated by wasm compile, and
held throughout the lifetime of the WebAssembly.Module object.

This change:
- Extends Managed<CppType> with a mechanism for doing just that
- Separates the role of Managed<CppType> to be strictly an owner of
the lifetime of the native resource. For cases where that's not
desirable, we can polymorphically use Foregin.
- moves managed.h out of wasm, since it's not wasm-specific.

BUG=680065
==========

to

==========
[wasm] Managed<T> ensures T's lifetime does not leak past Isolate's

Native resources allocated by v8, as internal implementation detail,
and held by a Foreign object, must be released when the Isolate is 
torn down. Example: wasm::WasmModule allocated by wasm compile, and
held throughout the lifetime of the WebAssembly.Module object.

This change:
- Extends Managed<CppType> with a mechanism for doing just that
- Separates the role of Managed<CppType> to be strictly an owner of
the lifetime of the native resource. For cases where that's not
desirable, we can polymorphically use Foregin.
- moves managed.h out of wasm, since it's not wasm-specific.

BUG=680065

Review-Url: https://codereview.chromium.org/2676513008
Cr-Commit-Position: refs/heads/master@{#43350}
Committed:
https://chromium.googlesource.com/v8/v8/+/caa1d4b262d2e97ce3b9f4163de46f230a8...
==========

[commit-bot: I haz the power, 2017-02-21 17:23:48]

Committed patchset #19 (id:360001) as
https://chromium.googlesource.com/v8/v8/+/caa1d4b262d2e97ce3b9f4163de46f230a8...