[Extension Bindings] Add more CHECKs to track down a crash

There's a crash happening when a JSRunner associated with a context
(which is checked to be valid) somehow doesn't exist. This shouldn't
happen, but somehow is. This patch adds checks that if a context is
considered to be valid, it has an associated JSRunner.

This also slightly reorders tear down flow in order to match this
requirement, putting context invalidation prior to JSRunner deletion.

Bug: 819968

Change-Id: I0e0d634f3e8c910cf50a157dfa367119be522d9a
Reviewed-on: https://chromium-review.googlesource.com/972571Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544698}

extensions/renderer/bindings/api_binding_bridge.cc

@@ -101,6 +101,12 @@ void APIBindingBridge::RegisterCustomHook(v8::Isolate* isolate,
   v8::Local<v8::String> context_type =
       gin::StringToSymbol(isolate, context_type_);
   v8::Local<v8::Value> args[] = {hook_object, extension_id, context_type};
+
+  // TODO(devlin): The context should still be valid at this point - nothing
+  // above should be able to invalidate it. But let's make extra sure.
+  // This CHECK is helping to track down https://crbug.com/819968, and should be
+  // removed when that's fixed.
+  CHECK(binding::IsContextValid(context));
   JSRunner::Get(context)->RunJSFunction(function, context, arraysize(args),
                                         args);
 }

extensions/renderer/bindings/api_binding_util.cc

@@ -9,6 +9,7 @@
 #include "base/supports_user_data.h"
 #include "build/build_config.h"
 #include "extensions/renderer/bindings/get_per_context_data.h"
+#include "extensions/renderer/bindings/js_runner.h"
 #include "gin/converter.h"
 #include "gin/per_context_data.h"
 
@@ -75,7 +76,18 @@ bool IsContextValid(v8::Local<v8::Context> context) {
           ContextInvalidationData::kPerContextDataKey));
   // The context is valid if we've never created invalidation data for it, or if
   // we have and it hasn't been marked as invalid.
-  return !invalidation_data || invalidation_data->is_context_valid();
+  bool is_context_valid =
+      !invalidation_data || invalidation_data->is_context_valid();
+
+  if (is_context_valid) {
+    // As long as the context is valid, there should be an associated
+    // JSRunner.
+    // TODO(devlin): (Likely) Remove this once https://crbug.com/819968, since
+    // this shouldn't necessarily be a hard dependency. At least downgrade it
+    // to a DCHECK.
+    CHECK(JSRunner::Get(context));
+  }
+  return is_context_valid;
 }
 
 bool IsContextValidOrThrowError(v8::Local<v8::Context> context) {

extensions/renderer/native_extension_bindings_system.cc

@@ -405,8 +405,10 @@ void NativeExtensionBindingsSystem::WillReleaseScriptContext(
     ScriptContext* context) {
   v8::HandleScope handle_scope(context->isolate());
   v8::Local<v8::Context> v8_context = context->v8_context();
-  JSRunner::ClearInstanceForContext(v8_context);
   api_system_.WillReleaseContext(v8_context);
+  // Clear the JSRunner only after everything else has been notified that the
+  // context is being released.
+  JSRunner::ClearInstanceForContext(v8_context);
 }
 
 void NativeExtensionBindingsSystem::UpdateBindingsForContext(