516509 - Electrolysis+plugins: fix the plugin host to supply context to per-plugin functions

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Benjamin Smedberg]

Currently Electrolysis can only load one out-of-process plugin because of the HACK_ stuff in PluginModuleParent::Shim. The dynamic trampolines cjones proposed are not really practical: instead I think we should modify the Mozilla plugin host so that it calls the following functions on SharedLibrary directly (and then, I'm hoping that we can just make PluginModuleParent implement SharedLibrary):

NP_Initialize
NP_Shutdown
NP_GetMIMEDescription
NP_GetValue
NP_GetEntryPoints (or skip this entirely for IPC plugins)
NPP_New

Josh, can you take this?

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #0)
> Currently Electrolysis can only load one out-of-process plugin because of the
> HACK_ stuff in PluginModuleParent::Shim. The dynamic trampolines cjones
> proposed are not really practical: instead I think we should modify the Mozilla
> plugin host

Agreed.  This suggestion was made the modus operandi was "don't touch plugin code."  Dynamically allocated trampolines are way too much of a pain.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Concrete TODOs:

 (1) dom/plugins/SharedLibrary.h should be removed in favor of dom/plugins/PluginLibrary.h that has NP_Initialize(), NP_Shutdown(), et al. interface methods
 (2) dom/plugins/SharedPRLibrary.h should be removed in favor of dom/plugins/PluginPRLibrary.h (or something like that) that implements PluginLibrary.  This class should resolve the "NP_Initialize" et al. symbols from the underlying PR_Library when they're needed, and the NP_Initialize() et al. methods should call into those resolved symbols
 (3) dom/plugins/PluginModuleParent should be modified to implement PluginLibrary, and PluginModuleParent::LoadModule() should return the newly-allocated PluginModuleParent*
 (4) PluginModuleParent should implement the PluginLibrary interface.  The methods currently in PluginModuleParent::Shim should just be moved into PluginModuleParent
 (4) PluginModuleParent::Shim should be removed entirely
 (5) nsNPAPIPlugin[Instance] should be modified to call NP_Initialize() et al. by PluginLibrary* method calls rather than by bare function pointer

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Oops, numbering scheme got mangled.

Comment 4

[Josh Aas]

Attachment: fix v0.9

This works on Linux, I can load multiple types of plugins out-of-process. It compiles on Mac OS X. I haven't tried this on Windows, probably needs some compile fixes there. There is still some cleanup to be done before this is ready, most of the areas needing work are marked XXXJOSH.

Comment 5

[Josh Aas]

Attachment: fix v1.0

This probably won't build on Windows, I haven't tested that yet, but we might as well start review while I see what tweaks we need there.

Comment 6

[Josh Aas]

Attachment: fix v1.1

Includes Windows build fixes.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 408540 [details] [diff] [review]
fix v1.1

Nice patch!

>diff --git a/dom/plugins/PluginLibrary.h b/dom/plugins/PluginLibrary.h
>new file mode 100644
>--- /dev/null
>+++ b/dom/plugins/PluginLibrary.h
>@@ -0,0 +1,76 @@
>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
>+ * vim: sw=4 ts=4 et :
>+ * ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Mozilla Plugin App.
>+ *
>+ * The Initial Developer of the Original Code is
>+ *   Josh Aas <[e-mail]>

Nit: I just learned recently that this is supposed to be the "Mozilla Foundation" for copyright purposes.

>+#ifndef PluginLibrary_h
>+#define PluginLibrary_h 1
>+

Nit: I think this should be |mozilla_PluginLibrary_h|, but it's not documented in the style guide.

>+#include "prlink.h"
>+#include "npapi.h"
>+#include "nscore.h"
>+
>+namespace mozilla {
>+
>+class PluginLibrary
>+{
>+public:
>+  virtual ~PluginLibrary() { }
>+
>+  virtual bool HasRequiredFunctions() = 0;
>+
>+#if defined(XP_UNIX) && !defined(XP_MACOSX)
>+  virtual nsresult NP_Initialize(NPNetscapeFuncs* bFuncs, NPPluginFuncs* pFuncs, NPError* error) = 0;
>+#else
>+  virtual nsresult NP_Initialize(NPNetscapeFuncs* bFuncs, NPError* error) = 0;
>+#endif
>+  virtual nsresult NP_Shutdown(NPError* error) = 0;
>+  virtual nsresult NP_GetMIMEDescription(char** mimeDesc) = 0;
>+  virtual nsresult NP_GetValue(void *future, NPPVariable aVariable,
>+                               void *aValue, NPError* error) = 0;
>+#if defined(XP_WIN) || defined(XP_MACOSX)
>+  virtual nsresult NP_GetEntryPoints(NPPluginFuncs* pFuncs, NPError* error) = 0;
>+#endif
>+  virtual nsresult NPP_New(NPMIMEType pluginType, NPP instance,
>+                           uint16_t mode, int16_t argc, char* argn[],
>+                           char* argv[], NPSavedData* saved,
>+                           NPError* error) = 0;
>+};
>+
>+
>+} // namespace mozilla
>+
>+#endif  // ifndef PluginLibrary_h


>diff --git a/dom/plugins/PluginModuleParent.cpp b/dom/plugins/PluginModuleParent.cpp
>--- a/dom/plugins/PluginModuleParent.cpp
>+++ b/dom/plugins/PluginModuleParent.cpp
>@@ -36,58 +36,51 @@
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "mozilla/plugins/PluginModuleParent.h"
> #include "mozilla/plugins/BrowserStreamParent.h"
> 
> #include "nsNPAPIPlugin.h"
> 
>-using mozilla::SharedLibrary;
>+using mozilla::PluginLibrary;
> 
> using mozilla::ipc::NPRemoteIdentifier;
> 
> using namespace mozilla::plugins;
> 
> PR_STATIC_ASSERT(sizeof(NPIdentifier) == sizeof(void*));
> 
>-// HACKS
>-PluginModuleParent* PluginModuleParent::Shim::HACK_target;
>-
>-SharedLibrary*
>-PluginModuleParent::LoadModule(const char* aFilePath, PRLibrary* aLibrary)
>+PluginLibrary*
>+PluginModuleParent::LoadModule(const char* aFilePath)
> {
>     _MOZ_LOG(__FUNCTION__);
> 
>     // Block on the child process being launched and initialized.
>     PluginModuleParent* parent = new PluginModuleParent(aFilePath);
>     parent->mSubprocess.Launch();
>     parent->Open(parent->mSubprocess.GetChannel());
> 
>     // FIXME/cjones: leaking PluginModuleParents ...
>-    return parent->mShim;
>+    return parent;
> }

I /think/ we're not leaking these anymore, right?  (Though we're still not attempting to shut down cleanly; your |NP_Shutdown()| comment is spot on.)  PluginModuleParent should be on par with PluginPRLibrary.  We used to leak PluginModuleParent's because of the Shim insanity.

>+}
>+
>+nsresult
>+PluginModuleParent::NP_GetMIMEDescription(char** mimeDesc)
>+{
>+    _MOZ_LOG(__FUNCTION__);
>+
>+    *mimeDesc = (char*)"application/x-foobar";
>+    return NS_OK;
>+}
>+
>+nsresult
>+PluginModuleParent::NP_GetValue(void *future, NPPVariable aVariable,
>+                                   void *aValue, NPError* error)
>+{
>+    _MOZ_LOG(__FUNCTION__);
>+
>+    *error = NPERR_GENERIC_ERROR;
>+    return NS_OK;
>+}
>+

Can we get "// TODO" comments here along with a |printf("[%s] NYI\n", __FUNCTION__)|?


>diff --git a/dom/plugins/PluginModuleParent.h b/dom/plugins/PluginModuleParent.h
>--- a/dom/plugins/PluginModuleParent.h
>+++ b/dom/plugins/PluginModuleParent.h
>@@ -45,17 +45,17 @@
>     /**
>      * LoadModule
>      *
>-     * Returns a SharedLibrary from which plugin symbols should be
>+     * Returns a PluginLibrary from which plugin symbols should be
>      * resolved.  This may or may not launch a plugin child process,
>      * and may or may not be very expensive.
>      */
>-    static SharedLibrary* LoadModule(const char* aFilePath,
>-                                     PRLibrary* aLibrary);

Nit: this comment should be updated; no symbols are being resolved anymore, the library returned offers the PluginLibrary interface.


>diff --git a/modules/plugin/base/src/nsNPAPIPluginInstance.cpp b/modules/plugin/base/src/nsNPAPIPluginInstance.cpp
>--- a/modules/plugin/base/src/nsNPAPIPluginInstance.cpp
>+++ b/modules/plugin/base/src/nsNPAPIPluginInstance.cpp
>@@ -264,17 +264,17 @@ nsresult nsNPAPIPluginStreamListener::Cl
>     return rv;
> 
>   NPP npp;
>   mInst->GetNPP(&npp);
> 
>   if (mStreamStarted && callbacks->destroystream) {
>     NPPAutoPusher nppPusher(npp);
> 
>-    mozilla::SharedLibrary* lib = nsnull;
>+    mozilla::PluginLibrary* lib = nsnull;

>@@ -749,17 +749,17 @@ nsNPAPIPluginStreamListener::OnFileAvail
>   const NPPluginFuncs *callbacks = nsnull;
>   mInst->GetCallbacks(&callbacks);
>   if (!callbacks || !callbacks->asfile)
>     return NS_ERROR_FAILURE;
>   
>   NPP npp;
>   mInst->GetNPP(&npp);
> 
>-  mozilla::SharedLibrary* lib = nsnull;
>+  mozilla::PluginLibrary* lib = nsnull;

>@@ -869,17 +869,17 @@ nsInstanceStream::nsInstanceStream()
> 
> nsInstanceStream::~nsInstanceStream()
> {
> }
> 
> NS_IMPL_ISUPPORTS1(nsNPAPIPluginInstance, nsIPluginInstance)
> 
> nsNPAPIPluginInstance::nsNPAPIPluginInstance(NPPluginFuncs* callbacks,
>-                                             mozilla::SharedLibrary* aLibrary)
>+                                             mozilla::PluginLibrary* aLibrary)

Nits: style guide would have |mozilla::PluginLibrary| be privately typedef'd to |PluginLibrary| in nsNPAPIPluginInstance.h so you don't have to keep typing |mozilla::|.

>@@ -1197,18 +1195,19 @@ nsNPAPIPluginInstance::InitializePlugin(
>   mStarted = PR_TRUE;
> 
>   PRBool oldVal = mInPluginInitCall;
>   mInPluginInitCall = PR_TRUE;
> 
>   // Need this on the stack before calling NPP_New otherwise some callbacks that
>   // the plugin may make could fail (NPN_HasProperty, for example).
>   NPPAutoPusher autopush(&mNPP);
>-
>-  NS_TRY_SAFE_CALL_RETURN(error, (*mCallbacks->newp)((char*)mimetype, &mNPP, (PRUint16)mode, count, (char**)names, (char**)values, NULL), mLibrary,this);
>+  nsresult newResult = mLibrary->NPP_New((char*)mimetype, &mNPP, (PRUint16)mode, count, (char**)names, (char**)values, NULL, &error);
>+  if (NS_FAILED(newResult))
>+    return newResult;
> 

Should we be using |NS_TRY_SAFE_CALL_RETURN()| in PluginPRLibrary and PluginInstanceChild?  We could edit the latter in a follow-up patch, if it's necessary.

Comment 8

[Josh Aas]

Attachment: fix v1.2

pushed to electrolysis repo

http://hg.mozilla.org/projects/electrolysis/rev/05ac28100696

Comment 9

[Josh Aas]

I did not include a fix for calling |NS_TRY_SAFE_CALL...|. We should figure out what we want to do about that in a later bug.