From 1ffcc4917b8839bc501e12bb3626006658feb05f Mon Sep 17 00:00:00 2001
From: Lina Cambridge <lina@yakshaving.ninja>
Date: Mon, 25 Mar 2019 04:49:36 +0000
Subject: [PATCH] Bug 1482608 - Convert null pointers passed to `xpcom_method`s
 into `Option`s. r=myk,nika

Differential Revision: https://phabricator.services.mozilla.com/D20075

--HG--
extra : moz-landing-system : lando
---
 xpcom/rust/xpcom/src/method.rs           | 57 ++++++++++++++++++------
 xpcom/rust/xpcom/xpcom_macros/src/lib.rs | 20 ++++++++-
 2 files changed, 61 insertions(+), 16 deletions(-)

diff --git a/xpcom/rust/xpcom/src/method.rs b/xpcom/rust/xpcom/src/method.rs
index b7937acfd151..fb60be87fe93 100644
--- a/xpcom/rust/xpcom/src/method.rs
+++ b/xpcom/rust/xpcom/src/method.rs
@@ -88,10 +88,10 @@ use nserror::{nsresult, NS_ERROR_NULL_POINTER};
 /// a custom error type, which the XPCOM stub will convert to nsresult.
 ///
 /// This macro assumes that all non-null pointer arguments are valid!
-/// It does ensure that they aren't null, returning NS_ERROR_NULL_POINTER
-/// for null pointers.  But it doesn't otherwise check their validity.
-/// That makes the function unsafe, so callers must ensure that they only
-/// call it with valid pointer arguments.
+/// It does ensure that they aren't null, using the `ensure_param` macro.
+/// But it doesn't otherwise check their validity. That makes the function
+/// unsafe, so callers must ensure that they only call it with valid pointer
+/// arguments.
 #[macro_export]
 macro_rules! xpcom_method {
     // `#[allow(non_snake_case)]` is used for each method because `$xpcom_name`
@@ -187,10 +187,15 @@ macro_rules! xpcom_method {
     };
 }
 
-/// The ensure_param macro converts raw pointer arguments to references,
-/// returning NS_ERROR_NULL_POINTER if the argument is_null().  It isn't
+/// The ensure_param macro converts raw pointer arguments to references, and
+/// passes them to a Rustic implementation of an XPCOM method. This macro isn't
 /// intended to be used directly but rather via the xpcom_method macro.
 ///
+/// If the argument `is_null()`, and the corresponding Rustic method parameter
+/// is `&T`, the macro returns `NS_ERROR_NULL_POINTER`. However, if the
+/// parameter is `Option<&T>`, the macro passes `None` instead. This makes it
+/// easy to use optional arguments safely.
+///
 /// Given the appropriate extern crate and use declarations (which include
 /// using the Ensure trait from this module):
 ///
@@ -200,22 +205,36 @@ macro_rules! xpcom_method {
 /// use xpcom::Ensure;
 /// ```
 ///
-/// Invoking the macro with a parameter like `foo: *const nsIBar`:
+/// Invoking the macro like this:
 ///
 /// ```ignore
-/// ensure_param!(foo: *const nsIBar);
+/// fn do_something_with_foo(foo: &nsIBar) {}
+/// fn DoSomethingWithFoo(foo: *const nsIBar) -> nsresult {
+///     let foo = ensure_param!(foo);
+///     do_something_with_foo(foo);
+/// }
 /// ```
 ///
-/// Results in the macro generating a code block like the following:
+/// Expands to code like this:
 ///
 /// ```ignore
-/// let foo = match Ensure::ensure(foo) {
-///     Ok(val) => val,
-///     Err(result) => return result,
-/// };
+/// fn do_something_with_foo(foo: &nsIBar) {}
+/// fn DoSomethingWithFoo(foo: *const nsIBar) -> nsresult {
+///     let foo = match Ensure::ensure(foo) {
+///         Ok(val) => val,
+///         Err(result) => return result,
+///     };
+///     do_something_with_foo(foo);
+/// }
 /// ```
 ///
-/// Which converts `foo` from a `*const nsIBar` to a `&nsIBar`.
+/// Which converts `foo` from a `*const nsIBar` to a `&nsIBar`, or returns
+/// `NS_ERROR_NULL_POINTER` if `foo` is null.
+///
+/// To pass an optional argument, we only need to change
+/// `do_something_with_foo` to take an `Option<&nsIBar> instead. The macro
+/// generates the same code, but `do_something_with_foo` receives `None` if
+/// `foo` is null.
 ///
 /// Notes:
 ///
@@ -259,6 +278,16 @@ impl<'a, T: 'a> Ensure<*const T> for Result<&'a T, nsresult> {
     }
 }
 
+impl<'a, T: 'a> Ensure<*const T> for Result<Option<&'a T>, nsresult> {
+    unsafe fn ensure(ptr: *const T) -> Result<Option<&'a T>, nsresult> {
+        Ok(if ptr.is_null() {
+            None
+        } else {
+            Some(&*ptr)
+        })
+    }
+}
+
 impl<T: Copy> Ensure<T> for Result<T, nsresult> {
     unsafe fn ensure(copyable: T) -> Result<T, nsresult> {
         Ok(copyable)
diff --git a/xpcom/rust/xpcom/xpcom_macros/src/lib.rs b/xpcom/rust/xpcom/xpcom_macros/src/lib.rs
index 332055200766..c19b1ae7a75b 100644
--- a/xpcom/rust/xpcom/xpcom_macros/src/lib.rs
+++ b/xpcom/rust/xpcom/xpcom_macros/src/lib.rs
@@ -22,7 +22,7 @@
 //!
 //! // Implementing methods on an XPCOM Struct
 //! impl ImplRunnable {
-//!     pub fn Run(&self) -> nsresult {
+//!     unsafe fn Run(&self) -> nsresult {
 //!         println!("{}", self.i);
 //!         NS_OK
 //!     }
@@ -110,7 +110,7 @@
 //! // implemented manually.
 //! impl ImplRunnable {
 //!     // The method should have the same name as the corresponding C++ method.
-//!     pub fn Run(&self) -> nsresult {
+//!     unsafe fn Run(&self) -> nsresult {
 //!         // Fields defined on the `Init` struct will be directly on the
 //!         // generated struct.
 //!         println!("{}", self.i);
@@ -119,6 +119,22 @@
 //! }
 //! ```
 //!
+//! XPCOM methods implemented in Rust have signatures similar to methods
+//! implemented in C++.
+//!
+//! ```ignore
+//! // nsISupports foo(in long long bar, in AString baz);
+//! unsafe fn Foo(&self, bar: libc::int64_t, baz: *const nsAString,
+//!               _retval: *mut *const nsISupports) -> nsresult;
+//!
+//! // AString qux(in nsISupports ham);
+//! unsafe fn Qux(&self, ham: *const nsISupports,
+//!               _retval: *mut nsAString) -> nsresult;
+//! ```
+//!
+//! This is a little tedious, so the `xpcom_method!` macro provides a convenient
+//! way to generate wrappers around more idiomatic Rust methods.
+//!
 //! [`xpcom`]: ../xpcom/index.html
 //! [`nsIRunnable`]: ../xpcom/struct.nsIRunnable.html
 //! [`RefCounted`]: ../xpcom/struct.RefCounted.html