Update security review guidance for callback wrappers.

Clarify the language and provide an example of acceptable usage.

Change-Id: Iac610fa996445ebb43cbaeaeae78fb88bb619a85
Reviewed-on: https://chromium-review.googlesource.com/909798
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535821}

docs/security/mojo.md

@@ -345,7 +345,7 @@ for (size_t i = 0; i < request->element_size(); ++i) {
 ## C++ Best Practices
 
 
-### Use mojo::WrapCallbackWithDefaultInvokeIfNotRun And mojo::WrapCallbackWithDropHandler Sparingly
+### Use mojo::WrapCallbackWithDefaultInvokeIfNotRun And mojo::WrapCallbackWithDropHandler sparingly
 
 Mojo provides several convenience helpers to automatically invoke a callback if
 the callback has not already been invoked in some other way when the callback is
@@ -358,15 +358,30 @@ destroyed, e.g.:
   }  // |cb| is automatically invoked with an argument of -1.
 ```
 
-Unfortunately, the fact that this callback is guaranteed to always run is hidden
-from the type system and can propagate in surprising ways. Avoid using this
-construction unless there are no better alternatives. Uses of these helpers must
-be well-commented to describe why this behavior is required.
+This can be useful for detecting interface errors:
 
-Note that using this from the renderer is often unnecessary. Message pipes are
-often closed as part of a Document shutting down; since many Blink objects
-already inherit `blink::ContextLifecycleObserver`, it is usually more idiomatic
-to use this signal to perform any needed cleanup work.
+```c++
+  process->GetMemoryStatistics(
+      mojo::WrapCallbackWithDefaultInvokeIfNotRun(
+          base::Bind(&MemoryProfiler::OnReplyFromRenderer), <failure args>));
+  // If the remote process dies, &MemoryProfiler::OnReplyFromRenderer will be
+  // invoked with <failure args> when Mojo drops outstanding callbacks due to
+  // a connection error on |process|.
+```
+
+However, due to limitations of the current implementation, it's difficult to
+tell if a callback object has invoke-on-destroy behavior. In general:
+
+1.  Prefer error connection handlers where possible.
+1.  Only use the callback helpers for detecting interface errors. These
+    callbacks may be invoked during destruction and must carefully consider
+    receiver object lifetime. For more information, please see the
+    [Mojo documentation][mojo-doc-process-crashes].
+
+> Note that using the callback wrappers in the renderer is often unnecessary.
+> Message pipes are typically closed as part of a Document shutting down; since
+> many Blink objects already inherit `blink::ContextLifecycleObserver`, it is
+> usually more idiomatic to use this signal to perform any needed cleanup work.
 
 ### Use StructTraits
 
@@ -606,3 +621,4 @@ safe, vulnerabilities could arise.
 
 [security-tips-for-ipc]: https://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc
 [NfcTypeConverter.java]: https://chromium.googlesource.com/chromium/src/+/e97442ee6e8c4cf6bcf7f5623c6fb2cc8cce92ac/services/device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java
+[mojo-doc-process-crashes]: https://chromium.googlesource.com/chromium/src/+/master/mojo/public/cpp/bindings#Best-practices-for-dealing-with-process-crashes-and-callbacks