Use correct lifetime for TryCatch::exception()/message() return value (…

…#380)

According to v8.h, "the returned handle is valid until this TryCatch
block has been destroyed". This is incorrect, as can be demonstrated
with the test below. In practice the return value lives no longer and
no shorter than the active HandleScope at the time these methods are
called. An issue has been opened about this in the V8 bug tracker:
https://bugs.chromium.org/p/v8/issues/detail?id=10537.

```rust
fn try_catch_bad_lifetimes() {
  let _setup_guard = setup();
  let mut isolate = v8::Isolate::new(Default::default());
  let mut hs = v8::HandleScope::new(&mut isolate);
  let scope = hs.enter();
  let context = v8::Context::new(scope);
  let mut cs = v8::ContextScope::new(scope, context);
  let scope = cs.enter();
  let caught_msg_2 = {
    let mut try_catch = v8::TryCatch::new(scope);
    let try_catch = try_catch.enter();
    let caught_msg_1 = {
      let mut hs = v8::HandleScope::new(scope);
      let scope = hs.enter();

      // Throw exception #1.
      let msg_1 = v8::String::new(scope, "BOOM!").unwrap();
      let exc_1 = v8::Exception::type_error(scope, msg_1);
      scope.isolate().throw_exception(exc_1);
      // Catch exception #1.
      let caught_msg_1 = try_catch.message().unwrap();
      let caught_str_1 =
        caught_msg_1.get(scope).to_rust_string_lossy(scope);
      assert!(caught_str_1.contains("BOOM"));
      // Move `caught_msg_1` out of the HandleScope it was created in.
      // The borrow checker allows this because `caught_msg_1`'s
      // lifetime is contrained to not outlive the TryCatch, but it is
      // allowed to outlive the HandleScope that was active when the
      // exception was caught.
      caught_msg_1
    };
    // Next line crashes.
    let caught_str_1 =
      caught_msg_1.get(scope).to_rust_string_lossy(scope);
    assert!(caught_str_1.contains("BOOM"));

    // Throws exception #2.
    let msg_2 = v8::String::new(scope, "DANG!").unwrap();
    let exc_2 = v8::Exception::type_error(scope, msg_2);
    scope.isolate().throw_exception(exc_2);
    // Catch exception #2.
    let caught_msg_2 = try_catch.message().unwrap();
    let caught_str_2 =
      caught_msg_2.get(scope).to_rust_string_lossy(scope);
    assert!(caught_str_2.contains("DANG"));
    // Move `caught_msg_2` out of the extent of the TryCatch, but still
    // within the extent of its HandleScope. This is unnecessarily
    // rejected at compile time.
    caught_msg_2
  };
  let caught_str_2 =
    caught_msg_2.get(scope).to_rust_string_lossy(scope);
  assert!(caught_str_2.contains("DANG"));
}
```

src/try_catch.rs

@@ -110,9 +110,28 @@ impl<'tc> TryCatch<'tc> {
   /// Returns the exception caught by this try/catch block. If no exception has
   /// been caught an empty handle is returned.
   ///
-  /// The returned handle is valid until this TryCatch block has been destroyed.
-  pub fn exception(&self) -> Option<Local<'tc, Value>> {
-    unsafe { Local::from_raw(v8__TryCatch__Exception(&self.0)) }
+  /// Note: v8.h states that "the returned handle is valid until this TryCatch
+  /// block has been destroyed". This is incorrect; the return value lives
+  /// no longer and no shorter than the active HandleScope at the time this
+  /// method is called. An issue has been opened about this in the V8 bug
+  /// tracker: https://bugs.chromium.org/p/v8/issues/detail?id=10537.
+  pub fn exception<'sc>(
+    &self,
+    scope: &mut impl ToLocal<'sc>,
+  ) -> Option<Local<'sc, Value>> {
+    unsafe { scope.to_local(v8__TryCatch__Exception(&self.0)) }
+  }
+
+  /// Returns the message associated with this exception. If there is
+  /// no message associated an empty handle is returned.
+  ///
+  /// Note: the remark about the lifetime for the `exception()` return value
+  /// applies here too.
+  pub fn message<'sc>(
+    &self,
+    scope: &mut impl ToLocal<'sc>,
+  ) -> Option<Local<'sc, Message>> {
+    unsafe { scope.to_local(v8__TryCatch__Message(&self.0)) }
   }
 
   /// Returns the .stack property of the thrown object. If no .stack
@@ -125,15 +144,6 @@ impl<'tc> TryCatch<'tc> {
     unsafe { scope.to_local(v8__TryCatch__StackTrace(&self.0, &*context)) }
   }
 
-  /// Returns the message associated with this exception. If there is
-  /// no message associated an empty handle is returned.
-  ///
-  /// The returned handle is valid until this TryCatch block has been
-  /// destroyed.
-  pub fn message(&self) -> Option<Local<'tc, Message>> {
-    unsafe { Local::from_raw(v8__TryCatch__Message(&self.0)) }
-  }
-
   /// Clears any exceptions that may have been caught by this try/catch block.
   /// After this method has been called, HasCaught() will return false. Cancels
   /// the scheduled exception if it is caught and ReThrow() is not called before.

tests/compile_fail/try_catch_exception_lifetime.rs

@@ -3,13 +3,13 @@ use rusty_v8 as v8;
 
 pub fn main() {
   let mut isolate = v8::Isolate::new(mock());
-  let mut hs = v8::HandleScope::new(&mut isolate);
-  let hs = hs.enter();
+  let mut try_catch = v8::TryCatch::new(&mut isolate);
+  let try_catch = try_catch.enter();
 
   let _exception = {
-    let mut try_catch = v8::TryCatch::new(hs);
-    let tc = try_catch.enter();
-    tc.exception().unwrap()
+    let mut hs = v8::HandleScope::new(&mut isolate);
+    let hs = hs.enter();
+    try_catch.exception(hs).unwrap()
   };
 }
 

tests/compile_fail/try_catch_exception_lifetime.stderr

@@ -1,11 +1,11 @@
-error[E0597]: `try_catch` does not live long enough
+error[E0597]: `hs` does not live long enough
   --> $DIR/try_catch_exception_lifetime.rs:11:14
    |
 9  |   let _exception = {
    |       ---------- borrow later stored here
-10 |     let mut try_catch = v8::TryCatch::new(hs);
-11 |     let tc = try_catch.enter();
-   |              ^^^^^^^^^ borrowed value does not live long enough
-12 |     tc.exception().unwrap()
+10 |     let mut hs = v8::HandleScope::new(&mut isolate);
+11 |     let hs = hs.enter();
+   |              ^^ borrowed value does not live long enough
+12 |     try_catch.exception(hs).unwrap()
 13 |   };
-   |   - `try_catch` dropped here while still borrowed
+   |   - `hs` dropped here while still borrowed

tests/compile_fail/try_catch_message_lifetime.rs

@@ -3,13 +3,13 @@ use rusty_v8 as v8;
 
 pub fn main() {
   let mut isolate = v8::Isolate::new(mock());
-  let mut hs = v8::HandleScope::new(&mut isolate);
-  let hs = hs.enter();
+  let mut try_catch = v8::TryCatch::new(&mut isolate);
+  let try_catch = try_catch.enter();
 
-  let _message = {
-    let mut try_catch = v8::TryCatch::new(hs);
-    let tc = try_catch.enter();
-    tc.message().unwrap()
+  let _exception = {
+    let mut hs = v8::HandleScope::new(&mut isolate);
+    let hs = hs.enter();
+    try_catch.message(hs).unwrap()
   };
 }
 

tests/compile_fail/try_catch_message_lifetime.stderr

@@ -1,11 +1,11 @@
-error[E0597]: `try_catch` does not live long enough
+error[E0597]: `hs` does not live long enough
   --> $DIR/try_catch_message_lifetime.rs:11:14
    |
-9  |   let _message = {
-   |       -------- borrow later stored here
-10 |     let mut try_catch = v8::TryCatch::new(hs);
-11 |     let tc = try_catch.enter();
-   |              ^^^^^^^^^ borrowed value does not live long enough
-12 |     tc.message().unwrap()
+9  |   let _exception = {
+   |       ---------- borrow later stored here
+10 |     let mut hs = v8::HandleScope::new(&mut isolate);
+11 |     let hs = hs.enter();
+   |              ^^ borrowed value does not live long enough
+12 |     try_catch.message(hs).unwrap()
 13 |   };
-   |   - `try_catch` dropped here while still borrowed
+   |   - `hs` dropped here while still borrowed

tests/test_api.rs

@@ -536,11 +536,14 @@ fn try_catch() {
       let result = eval(scope, context, "throw new Error('foo')");
       assert!(result.is_none());
       assert!(tc.has_caught());
-      assert!(tc.exception().is_some());
+      assert!(tc.exception(scope).is_some());
       assert!(tc.stack_trace(scope, context).is_some());
-      assert!(tc.message().is_some());
+      assert!(tc.message(scope).is_some());
       assert_eq!(
-        tc.message().unwrap().get(scope).to_rust_string_lossy(scope),
+        tc.message(scope)
+          .unwrap()
+          .get(scope)
+          .to_rust_string_lossy(scope),
         "Uncaught Error: foo"
       );
     };
@@ -551,9 +554,9 @@ fn try_catch() {
       let result = eval(scope, context, "1 + 1");
       assert!(result.is_some());
       assert!(!tc.has_caught());
-      assert!(tc.exception().is_none());
+      assert!(tc.exception(scope).is_none());
       assert!(tc.stack_trace(scope, context).is_none());
-      assert!(tc.message().is_none());
+      assert!(tc.message(scope).is_none());
       assert!(tc.rethrow().is_none());
     };
     {
@@ -574,6 +577,41 @@ fn try_catch() {
   }
 }
 
+#[test]
+fn try_catch_caught_lifetime() {
+  let _setup_guard = setup();
+  let mut isolate = v8::Isolate::new(Default::default());
+  let mut hs = v8::HandleScope::new(&mut isolate);
+  let scope = hs.enter();
+  let context = v8::Context::new(scope);
+  let mut cs = v8::ContextScope::new(scope, context);
+  let scope = cs.enter();
+  let (caught_exc, caught_msg) = {
+    let mut try_catch = v8::TryCatch::new(scope);
+    let try_catch = try_catch.enter();
+    // Throw exception.
+    let msg = v8::String::new(scope, "DANG!").unwrap();
+    let exc = v8::Exception::type_error(scope, msg);
+    scope.isolate().throw_exception(exc);
+    // Catch exception.
+    let caught_exc = try_catch.exception(scope).unwrap();
+    let caught_msg = try_catch.message(scope).unwrap();
+    // Move `caught_exc` and `caught_msg` out of the extent of the TryCatch,
+    // but still within the extent of the enclosing HandleScope.
+    (caught_exc, caught_msg)
+  };
+  // This should not crash.
+  assert!(caught_exc
+    .to_string(scope)
+    .unwrap()
+    .to_rust_string_lossy(scope)
+    .contains("DANG"));
+  assert!(caught_msg
+    .get(scope)
+    .to_rust_string_lossy(scope)
+    .contains("DANG"));
+}
+
 #[test]
 fn throw_exception() {
   let _setup_guard = setup();
@@ -591,7 +629,7 @@ fn throw_exception() {
       scope.isolate().throw_exception(exception.into());
       assert!(tc.has_caught());
       assert!(tc
-        .exception()
+        .exception(scope)
         .unwrap()
         .strict_equals(v8_str(scope, "boom").into()));
     };
@@ -1605,7 +1643,7 @@ fn module_instantiation_failures1() {
       assert!(result.is_none());
       assert!(tc.has_caught());
       assert!(tc
-        .exception()
+        .exception(scope)
         .unwrap()
         .strict_equals(v8_str(scope, "boom").into()));
       assert_eq!(v8::ModuleStatus::Uninstantiated, module.get_status());