Resolution for issue #5092: Unit test handle checks consider dangling handles to be valid.

Resolution for issue #5092: Unit test handle checks consider dangling handles to be valid.

Updated the IS_VALID(handle) macro to also check if the handle is still in
scope or is a read-only handle.

BUG=
R=asiva@google.com

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

[bkonyi, 2017-01-18 00:44:46]

bkonyi@google.com changed reviewers:
+ johnmccutchan@google.com, sivaa@google.com, zra@google.com

[bkonyi, 2017-01-18 00:44:47]

https://codereview.chromium.org/2640573003/diff/1/runtime/vm/dart_api_impl.h
File runtime/vm/dart_api_impl.h (right):

https://codereview.chromium.org/2640573003/diff/1/runtime/vm/dart_api_impl.h#...
runtime/vm/dart_api_impl.h:178: return
isolate->thread_registry()->IsValidHandle(handle) ||
I'm not sure if I should be checking to see if any isolate contains this handle
or if I should only be looking at the current isolate. All tests currently pass
in the current state.

https://codereview.chromium.org/2640573003/diff/1/runtime/vm/unit_test.h
File runtime/vm/unit_test.h (right):

https://codereview.chromium.org/2640573003/diff/1/runtime/vm/unit_test.h#newc...
runtime/vm/unit_test.h:575: do {                                                
                        \
Should this check for error handles as well, should it be part of EXPECT_ERROR,
or is this fine the way it is?

[bkonyi, 2017-01-18 00:47:01]

https://codereview.chromium.org/2640573003/diff/1/runtime/vm/unit_test.h
File runtime/vm/unit_test.h (right):

https://codereview.chromium.org/2640573003/diff/1/runtime/vm/unit_test.h#newc...
runtime/vm/unit_test.h:550: if (!Dart_IsValid(tmp_handle)) {                    
                      \
I'm not sure if we'd rather create a new macro or just modify EXPECT_VALID to
check for dangling handles. Personally, this makes the most sense to me as it
won't require any changes to the current tests.

[Cutch, 2017-01-18 14:49:52]

DBC. Also, you have the wrong email address for Siva- our Siva is asiva@

https://codereview.chromium.org/2640573003/diff/1/runtime/vm/dart_api_impl.h
File runtime/vm/dart_api_impl.h (right):

https://codereview.chromium.org/2640573003/diff/1/runtime/vm/dart_api_impl.h#...
runtime/vm/dart_api_impl.h:178: return
isolate->thread_registry()->IsValidHandle(handle) ||
On 2017/01/18 00:44:47, bkonyi wrote:
> I'm not sure if I should be checking to see if any isolate contains this
handle
> or if I should only be looking at the current isolate. All tests currently
pass
> in the current state.

In general our API always interacts with the current isolate.

[bkonyi, 2017-01-18 17:37:48]

bkonyi@google.com changed reviewers:
+ asiva@google.com
- sivaa@google.com

[siva, 2017-01-18 23:27:07]

https://codereview.chromium.org/2640573003/diff/1/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/2640573003/diff/1/runtime/include/dart_api.h#...
runtime/include/dart_api.h:266: DART_EXPORT bool Dart_IsValid(Dart_Handle
handle);
Why do we need this new API call, the issue you are addressing seems to only
talk about the EXPECT macros which are all internal to the VM

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

https://codereview.chromium.org/2640573003/diff/1/runtime/vm/handles_test.cc#...
runtime/vm/handles_test.cc:92: EXPECT_INVALID(handle);
EXPECT(!Api::IsValid(handle));

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

https://codereview.chromium.org/2640573003/diff/1/runtime/vm/thread_registry....
runtime/vm/thread_registry.cc:117: return false;
What about persistent handles, weak persistent handles etc.

https://codereview.chromium.org/2640573003/diff/1/runtime/vm/unit_test.h
File runtime/vm/unit_test.h (right):

https://codereview.chromium.org/2640573003/diff/1/runtime/vm/unit_test.h#newc...
runtime/vm/unit_test.h:550: if (!Dart_IsValid(tmp_handle)) {                    
                      \
On 2017/01/18 00:47:01, bkonyi wrote:
> I'm not sure if we'd rather create a new macro or just modify EXPECT_VALID to
> check for dangling handles. Personally, this makes the most sense to me as it
> won't require any changes to the current tests.

The check for the handle being valid should happen before the check for
Dart_IsError.

https://codereview.chromium.org/2640573003/diff/1/runtime/vm/unit_test.h#newc...
runtime/vm/unit_test.h:561: Dart_Handle tmp_handle = (handle);                  
                      \
This should also have a call to Dart_IsValid(...) to ensure the handle is valid
before checking it is an error handle.

https://codereview.chromium.org/2640573003/diff/1/runtime/vm/unit_test.h#newc...
runtime/vm/unit_test.h:575: do {                                                
                        \
On 2017/01/18 00:44:47, bkonyi wrote:
> Should this check for error handles as well, should it be part of
EXPECT_ERROR,
> or is this fine the way it is?
Why do we need this macro, couldn't the unit tests to
EXPECT(!Dart_IsValid(handle());

[bkonyi, 2017-01-19 01:05:13]

https://codereview.chromium.org/2640573003/diff/1/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/2640573003/diff/1/runtime/include/dart_api.h#...
runtime/include/dart_api.h:266: DART_EXPORT bool Dart_IsValid(Dart_Handle
handle);
On 2017/01/18 at 23:27:06, siva wrote:
> Why do we need this new API call, the issue you are addressing seems to only
talk about the EXPECT macros which are all internal to the VM

I was following the pattern of Dart_IsError so just assumed that there should be
a Dart_IsValid as well. I've removed it.

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

https://codereview.chromium.org/2640573003/diff/1/runtime/vm/handles_test.cc#...
runtime/vm/handles_test.cc:92: EXPECT_INVALID(handle);
On 2017/01/18 at 23:27:06, siva wrote:
> EXPECT(!Api::IsValid(handle));

Done.

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

https://codereview.chromium.org/2640573003/diff/1/runtime/vm/thread_registry....
runtime/vm/thread_registry.cc:117: return false;
On 2017/01/18 at 23:27:06, siva wrote:
> What about persistent handles, weak persistent handles etc.

See the changes to Api::IsValid, which should now check for these types of
handles. Let me know if I missed any other special types of handles somewhere
else.

https://codereview.chromium.org/2640573003/diff/1/runtime/vm/unit_test.h
File runtime/vm/unit_test.h (right):

https://codereview.chromium.org/2640573003/diff/1/runtime/vm/unit_test.h#newc...
runtime/vm/unit_test.h:550: if (!Dart_IsValid(tmp_handle)) {                    
                      \
On 2017/01/18 at 23:27:06, siva wrote:
> On 2017/01/18 00:47:01, bkonyi wrote:
> > I'm not sure if we'd rather create a new macro or just modify EXPECT_VALID
to
> > check for dangling handles. Personally, this makes the most sense to me as
it
> > won't require any changes to the current tests.
> 
> The check for the handle being valid should happen before the check for
Dart_IsError.

Done.

https://codereview.chromium.org/2640573003/diff/1/runtime/vm/unit_test.h#newc...
runtime/vm/unit_test.h:575: do {                                                
                        \
On 2017/01/18 at 23:27:06, siva wrote:
> On 2017/01/18 00:44:47, bkonyi wrote:
> > Should this check for error handles as well, should it be part of
EXPECT_ERROR,
> > or is this fine the way it is?
> Why do we need this macro, couldn't the unit tests to
> EXPECT(!Dart_IsValid(handle());

You're right. Removed.

[siva, 2017-01-20 17:40:03]

lgtm with comment about adding to the unit test case.

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

https://codereview.chromium.org/2640573003/diff/20001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:780: }
Not needed as you have dropped it from the API.

https://codereview.chromium.org/2640573003/diff/20001/runtime/vm/handles_test.cc
File runtime/vm/handles_test.cc (right):

https://codereview.chromium.org/2640573003/diff/20001/runtime/vm/handles_test...
runtime/vm/handles_test.cc:93: EXPECT(!Api::IsValid(handle));
Can you enhance this test to also include tests for other handle types, e.g:
- create and delete a persistent handle, retain reference to the handle and
check if it is valid
- same for weak persistent handle
- start a scope and create a scoped handle, close the scope and check validity
of the handle

[bkonyi, 2017-01-20 23:07:11]

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

https://codereview.chromium.org/2640573003/diff/20001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:780: }
On 2017/01/20 at 17:40:03, siva wrote:
> Not needed as you have dropped it from the API.

Done.

https://codereview.chromium.org/2640573003/diff/20001/runtime/vm/handles_test.cc
File runtime/vm/handles_test.cc (right):

https://codereview.chromium.org/2640573003/diff/20001/runtime/vm/handles_test...
runtime/vm/handles_test.cc:93: EXPECT(!Api::IsValid(handle));
On 2017/01/20 at 17:40:03, siva wrote:
> Can you enhance this test to also include tests for other handle types, e.g:
> - create and delete a persistent handle, retain reference to the handle and
check if it is valid
> - same for weak persistent handle
> - start a scope and create a scoped handle, close the scope and check validity
of the handle

I've gone ahead and added explicit tests for scoped, persistent, and
weakpersistent handles.

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

https://codereview.chromium.org/2640573003/diff/60001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:544: isolate->api_state()->IsActivePersistentHandle(
I added IsActive(Weak)PersistentHandle to check if the handle is valid and is
not part of the free list.

[siva, 2017-01-20 23:48:55]

lgtm

[bkonyi, 2017-01-20 23:50:22]

Description was changed from

==========
Resolution for issue #5092: Unit test handle checks consider dangling handles to
be valid.

Updated the IS_VALID(handle) macro to also check if the handle is still in
scope or is a read-only handle.

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

to

==========
Resolution for issue #5092: Unit test handle checks consider dangling handles to
be valid.

Updated the IS_VALID(handle) macro to also check if the handle is still in
scope or is a read-only handle.

BUG=
R=asiva@google.com

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

[bkonyi, 2017-01-20 23:50:23]

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