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

runtime/vm/unit_test.h

 // Copyright (c) 2011, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #ifndef RUNTIME_VM_UNIT_TEST_H_
 #define RUNTIME_VM_UNIT_TEST_H_
 
 #include "include/dart_native_api.h"
 
 #include "platform/globals.h"
@@ skipping 529 matching lines @@
   do {                                                                         \
     Dart_Handle tmp_handle = (handle);                                         \
     if (Dart_IsError(tmp_handle)) {                                            \
       dart::Expect(__FILE__, __LINE__)                                         \
           .Fail(                                                               \
               "expected '%s' to be a valid handle but found an error "         \
               "handle:\n"                                                      \
               "    '%s'\n",                                                    \
               #handle, Dart_GetError(tmp_handle));                             \
     }                                                                          \
+    if (!Dart_IsValid(tmp_handle)) {                                           \

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

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.

[siva, 2017/01/18 23:27:06]

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

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

Done.

+      dart::Expect(__FILE__, __LINE__)                                         \
+          .Fail(                                                               \
+              "expected '%s' to be a valid handle but '%s' has already been "  \
+              "freed\n",                                                       \
+              #handle, #handle);                                               \
+    }                                                                          \
   } while (0)
 
 #define EXPECT_ERROR(handle, substring)                                        \
   do {                                                                         \
     Dart_Handle tmp_handle = (handle);                                         \

[siva, 2017/01/18 23:27:06]

This should also have a call to Dart_IsValid(...) to ensure the handle is valid
before checking it is an error handle.

     if (Dart_IsError(tmp_handle)) {                                            \
       dart::Expect(__FILE__, __LINE__)                                         \
           .IsSubstring((substring), Dart_GetError(tmp_handle));                \
     } else {                                                                   \
       dart::Expect(__FILE__, __LINE__)                                         \
           .Fail(                                                               \
               "expected '%s' to be an error handle but found a valid "         \
               "handle.\n",                                                     \
               #handle);                                                        \
     }                                                                          \
   } while (0)
 
+#define EXPECT_INVALID(handle)                                                 \
+  do {                                                                         \

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

Should this check for error handles as well, should it be part of EXPECT_ERROR,
or is this fine the way it is?

[siva, 2017/01/18 23:27:06]

Why do we need this macro, couldn't the unit tests to
EXPECT(!Dart_IsValid(handle());

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

You're right. Removed.

+    Dart_Handle tmp_handle = (handle);                                         \
+    if (Dart_IsValid(tmp_handle)) {                                            \
+      dart::Expect(__FILE__, __LINE__)                                         \
+          .Fail(                                                               \
+              "expected '%s' to be an invalid handle but '%s' has not been "   \
+              "freed\n",                                                       \
+              #handle, #handle);                                               \
+    }                                                                          \
+  } while (0)
+
 #define EXPECT_TRUE(handle)                                                    \
   do {                                                                         \
     Dart_Handle tmp_handle = (handle);                                         \
     if (Dart_IsBoolean(tmp_handle)) {                                          \
       bool value;                                                              \
       Dart_BooleanValue(tmp_handle, &value);                                   \
       if (!value) {                                                            \
         dart::Expect(__FILE__, __LINE__)                                       \
             .Fail("expected True, but was '%s'\n", #handle);                   \
       }                                                                        \
@@ skipping 31 matching lines @@
   ~SetFlagScope() { *flag_ = original_value_; }
 
  private:
   T* flag_;
   T original_value_;
 };
 
 }  // namespace dart
 
 #endif  // RUNTIME_VM_UNIT_TEST_H_