Replace NPObject usage in ppapi with gin

content/renderer/pepper/pepper_plugin_instance_impl.cc

Index: content/renderer/pepper/pepper_plugin_instance_impl.cc
diff --git a/content/renderer/pepper/pepper_plugin_instance_impl.cc b/content/renderer/pepper/pepper_plugin_instance_impl.cc
index 682a6dadd197f5856274897687d9a5e8b8ec91b4..29f8fb4802d2550f5bb919a1ef88c33f518eb043 100644
--- a/content/renderer/pepper/pepper_plugin_instance_impl.cc
+++ b/content/renderer/pepper/pepper_plugin_instance_impl.cc
@@ -33,12 +33,12 @@
 #include "content/renderer/pepper/host_dispatcher_wrapper.h"
 #include "content/renderer/pepper/host_globals.h"
 #include "content/renderer/pepper/message_channel.h"
-#include "content/renderer/pepper/npapi_glue.h"
 #include "content/renderer/pepper/pepper_browser_connection.h"
 #include "content/renderer/pepper/pepper_compositor_host.h"
 #include "content/renderer/pepper/pepper_file_ref_renderer_host.h"
 #include "content/renderer/pepper/pepper_graphics_2d_host.h"
 #include "content/renderer/pepper/pepper_in_process_router.h"
+#include "content/renderer/pepper/pepper_try_catch.h"
 #include "content/renderer/pepper/pepper_url_loader_host.h"
 #include "content/renderer/pepper/plugin_module.h"
 #include "content/renderer/pepper/plugin_object.h"
@@ -115,6 +115,7 @@
 #include "third_party/WebKit/public/web/WebPrintParams.h"
 #include "third_party/WebKit/public/web/WebPrintScalingOption.h"
 #include "third_party/WebKit/public/web/WebScopedUserGesture.h"
+#include "third_party/WebKit/public/web/WebScriptSource.h"
 #include "third_party/WebKit/public/web/WebSecurityOrigin.h"
 #include "third_party/WebKit/public/web/WebUserGestureIndicator.h"
 #include "third_party/WebKit/public/web/WebView.h"
@@ -559,7 +560,6 @@ PepperPluginInstanceImpl::PepperPluginInstanceImpl(
       pending_user_gesture_(0.0),
       document_loader_(NULL),
       external_document_load_(false),
-      npp_(new NPP_t),
       isolate_(v8::Isolate::GetCurrent()),
       is_deleted_(false),
       last_input_number_(0),
@@ -620,17 +620,15 @@ PepperPluginInstanceImpl::PepperPluginInstanceImpl(
 PepperPluginInstanceImpl::~PepperPluginInstanceImpl() {
   DCHECK(!fullscreen_container_);
 
-  // Free all the plugin objects. This will automatically clear the back-
-  // pointer from the NPObject so WebKit can't call into the plugin any more.
-  //
-  // Swap out the set so we can delete from it (the objects will try to
-  // unregister themselves inside the delete call).

[dmichael (off chromium), 2014/08/21 22:24:24]

If this comment is still true, I think we should keep it. If it's not true
anymore, then you don't need to swap.

[raymes, 2014/08/22 08:28:40]

Done.

+  // Notify all the plugin objects of deletion. This will prevent WebKit from

[dmichael (off chromium), 2014/08/21 22:24:24]

s/WebKit/blink ?

[raymes, 2014/08/22 08:28:40]

Done.

+  // calling into the plugin any more.
   PluginObjectSet plugin_object_copy;
   live_plugin_objects_.swap(plugin_object_copy);
   for (PluginObjectSet::iterator i = plugin_object_copy.begin();
        i != plugin_object_copy.end();
-       ++i)
-    delete *i;
+       ++i) {
+    (*i)->InstanceDeleted();
+  }
 
   if (TrackedCallback::IsPending(lock_mouse_callback_))
     lock_mouse_callback_->Abort();
@@ -661,6 +659,19 @@ PepperPluginInstanceImpl::~PepperPluginInstanceImpl() {
 // If a method needs to access a member of the instance after the call has
 // returned, then it needs to keep its own reference on the stack.
 
+v8::Local<v8::Object> PepperPluginInstanceImpl::GetMessageChannelObject() {
+  return v8::Local<v8::Object>::New(isolate_, message_channel_);
+}
+
+MessageChannel* PepperPluginInstanceImpl::GetMessageChannel() {

[dmichael (off chromium), 2014/08/21 22:24:24]

Would it be simpler to have PepperPluginInstanceImpl just hold a
MessageChannel*? Or is holding the v8::Persistent not enough to guarantee the
MessageChannel stays alive?

If there's something more complicated that forces you in to doing it this way,
it's probably worth a comment.

[raymes, 2014/08/22 08:28:40]

Not completely sure what you mean. We need to store the v8::Persistent to ensure
the MessageChannel stays alive. We might be able to store a pointer to the
MessageChannel in addition to that but I feared that this was somehow unsafe
because V8 was managing the object. What do you think? Should I store the
pointer as well? 

[dmichael (off chromium), 2014/08/22 16:28:46]

That's all I meant, is I think it makes sense to *also* store the pointer. I
don't think it's any less safe, because the raw pointer you get from this
function is also only guaranteed to be pinned in memory while the Persistent is
alive.

[raymes, 2014/08/25 01:40:00]

Done.

+  MessageChannel* message_channel = NULL;
+  v8::HandleScope scope(isolate_);
+  v8::Context::Scope context_scope(GetContext());
+  if (gin::ConvertFromV8(isolate_, GetMessageChannelObject(), &message_channel))
+    return message_channel;
+  return NULL;
+}
+
 v8::Local<v8::Context> PepperPluginInstanceImpl::GetContext() {
   if (!container_)
     return v8::Handle<v8::Context>();
@@ -687,7 +698,7 @@ void PepperPluginInstanceImpl::Delete() {
   // release our last reference to the "InstanceObject" and will probably
   // destroy it. We want to do this prior to calling DidDestroy in case the
   // destructor of the instance object tries to use the instance.
-  message_channel_->SetPassthroughObject(NULL);
+  GetMessageChannel()->SetPassthroughObject(v8::Handle<v8::Object>());
   // If this is a NaCl plugin instance, shut down the NaCl plugin by calling
   // its DidDestroy. Don't call DidDestroy on the untrusted plugin instance,
   // since there is little that it can do at this point.
@@ -849,7 +860,7 @@ bool PepperPluginInstanceImpl::Initialize(
     bool full_frame) {
   if (!render_frame_)
     return false;
-  message_channel_.reset(new MessageChannel(this));
+  MessageChannel::Create(this, &message_channel_);

[dmichael (off chromium), 2014/08/21 22:24:24]

Curious why this doesn't just return the handle? Did you maybe once-upon-a-time
have it returning the MessageChannel*?

I think having it return MessageChannel* and doing the handle out-param seems
reasonable.

[raymes, 2014/08/22 08:28:40]

Ok that sounds reasonable, I'll revisit it in the next patchset.

 
   full_frame_ = full_frame;
 
@@ -874,7 +885,7 @@ bool PepperPluginInstanceImpl::Initialize(
   // A host for external plugins will call ResetAsProxied later, at which point
   // we can Start() the message_channel_.
   if (success && (!module_->renderer_ppapi_host()->IsExternalPluginHost()))
-    message_channel_->Start();
+    GetMessageChannel()->Start();
   return success;
 }
 
@@ -1218,10 +1229,12 @@ bool PepperPluginInstanceImpl::HandleBlockingMessage(ScopedPPVar message,
   return was_handled;
 }
 
-PP_Var PepperPluginInstanceImpl::GetInstanceObject() {
+PP_Var PepperPluginInstanceImpl::GetInstanceObject(v8::Isolate* isolate) {
   // Keep a reference on the stack. See NOTE above.
   scoped_refptr<PepperPluginInstanceImpl> ref(this);
 
+  DCHECK_EQ(isolate, isolate_);
+
   // If the plugin supports the private instance interface, try to retrieve its
   // instance object.
   if (LoadPrivateInterface())
@@ -1357,7 +1370,7 @@ void PepperPluginInstanceImpl::SetTextInputType(ui::TextInputType type) {
 }
 
 void PepperPluginInstanceImpl::PostMessageToJavaScript(PP_Var message) {
-  message_channel_->PostMessageToJavaScript(message);
+  GetMessageChannel()->PostMessageToJavaScript(message);
 }
 
 int32_t PepperPluginInstanceImpl::RegisterMessageHandler(
@@ -2361,70 +2374,67 @@ PP_Var PepperPluginInstanceImpl::GetWindowObject(PP_Instance instance) {
   if (!container_)
     return PP_MakeUndefined();
 
+  PepperTryCatchVar try_catch(this, true, NULL);
   WebLocalFrame* frame = container_->element().document().frame();
-  if (!frame)
+  if (!frame) {
+    try_catch.SetException("No frame exists for window object.");
     return PP_MakeUndefined();
+  }
 
-  return NPObjectToPPVar(this, frame->windowObject());
+  ScopedPPVar result =
+      try_catch.FromV8(frame->mainWorldScriptContext()->Global());
+  DCHECK(!try_catch.HasException());
+  return result.Release();
 }
 
 PP_Var PepperPluginInstanceImpl::GetOwnerElementObject(PP_Instance instance) {
   if (!container_)
     return PP_MakeUndefined();
-  return NPObjectToPPVar(this, container_->scriptableObjectForElement());
+  PepperTryCatchVar try_catch(this, true, NULL);

[dmichael (off chromium), 2014/08/21 22:24:24]

optional nit: Stuff like "true" and NULL in callsites are a pet peeve of mine,
because I prefer if I don't have to go digging to understand the call. Maybe it
would suffice to throw in comments, e.g. "true /* convert_objects */"

(Alternatively, you could make an enum instead of bool and make the exception
param required... but I think even though I like it that way, it's probably
overkill to most devs)

[raymes, 2014/08/22 08:28:40]

Done.

+  ScopedPPVar result = try_catch.FromV8(container_->v8ObjectForElement());
+  DCHECK(!try_catch.HasException());
+  return result.Release();
 }
 
 PP_Var PepperPluginInstanceImpl::ExecuteScript(PP_Instance instance,
-                                               PP_Var script,
+                                               PP_Var script_var,
                                                PP_Var* exception) {
+  if (!container_)
+    return PP_MakeUndefined();
+
   // Executing the script may remove the plugin from the DOM, so we need to keep
   // a reference to ourselves so that we can still process the result after the
   // WebBindings::evaluate() below.
   scoped_refptr<PepperPluginInstanceImpl> ref(this);
-  TryCatch try_catch(exception);
-  if (try_catch.has_exception())
-    return PP_MakeUndefined();
-
-  // Convert the script into an inconvenient NPString object.
-  StringVar* script_string = StringVar::FromPPVar(script);
-  if (!script_string) {
-    try_catch.SetException("Script param to ExecuteScript must be a string.");
+  PepperTryCatchVar try_catch(this, true, exception);
+  WebLocalFrame* frame = container_->element().document().frame();
+  if (!frame) {
+    try_catch.SetException("No frame to execute script in.");
     return PP_MakeUndefined();
   }
-  NPString np_script;
-  np_script.UTF8Characters = script_string->value().c_str();
-  np_script.UTF8Length = script_string->value().length();
 
-  // Get the current frame to pass to the evaluate function.
-  WebLocalFrame* frame = NULL;
-  if (container_)
-    frame = container_->element().document().frame();
-  if (!frame || !frame->windowObject()) {
-    try_catch.SetException("No context in which to execute script.");
+  StringVar* script_string_var = StringVar::FromPPVar(script_var);
+  if (!script_string_var) {
+    try_catch.SetException("Script param to ExecuteScript must be a string.");
     return PP_MakeUndefined();
   }
 
-  NPVariant result;
-  bool ok = false;
+  std::string script_string = script_string_var->value();
+  blink::WebScriptSource script(
+      blink::WebString::fromUTF8(script_string.c_str()));
+  v8::Handle<v8::Value> result;
   if (IsProcessingUserGesture()) {
     blink::WebScopedUserGesture user_gesture(CurrentUserGestureToken());
-    ok =
-        WebBindings::evaluate(NULL, frame->windowObject(), &np_script, &result);
+    result = frame->executeScriptAndReturnValue(script);
   } else {
-    ok =
-        WebBindings::evaluate(NULL, frame->windowObject(), &np_script, &result);
-  }
-  if (!ok) {
-    // TryCatch doesn't catch the exceptions properly. Since this is only for
-    // a trusted API, just set to a general exception message.
-    try_catch.SetException("Exception caught");
-    WebBindings::releaseVariantValue(&result);
-    return PP_MakeUndefined();
+    result = frame->executeScriptAndReturnValue(script);
   }
 
-  PP_Var ret = NPVariantToPPVar(this, &result);
-  WebBindings::releaseVariantValue(&result);
-  return ret;
+  ScopedPPVar var_result = try_catch.FromV8(result);
+  if (try_catch.HasException())
+    return PP_MakeUndefined();
+
+  return var_result.Release();
 }
 
 uint32_t PepperPluginInstanceImpl::GetAudioHardwareOutputSampleRate(
@@ -2953,7 +2963,7 @@ PP_ExternalPluginResult PepperPluginInstanceImpl::ResetAsProxied(
   if (!instance_interface_->DidCreate(
           pp_instance(), argn_.size(), argn_array.get(), argv_array.get()))
     return PP_EXTERNAL_PLUGIN_ERROR_INSTANCE;
-  message_channel_->Start();
+  GetMessageChannel()->Start();
 
   // Clear sent_initial_did_change_view_ and cancel any pending DidChangeView
   // event. This way, SendDidChangeView will send the "current" view
@@ -2982,8 +2992,6 @@ bool PepperPluginInstanceImpl::IsValidInstanceOf(PluginModule* module) {
   return module == module_.get() || module == original_module_.get();
 }
 
-NPP PepperPluginInstanceImpl::instanceNPP() { return npp_.get(); }
-
 PepperPluginInstance* PepperPluginInstance::Get(PP_Instance instance_id) {
   return HostGlobals::Get()->GetInstance(instance_id);
 }
@@ -3190,7 +3198,7 @@ int PepperPluginInstanceImpl::MakePendingFileRefRendererHost(
 }
 
 void PepperPluginInstanceImpl::SetEmbedProperty(PP_Var key, PP_Var value) {
-  message_channel_->SetReadOnlyProperty(key, value);
+  GetMessageChannel()->SetReadOnlyProperty(key, value);
 }
 
 bool PepperPluginInstanceImpl::CanAccessMainFrame() const {