554676 - implement Core Graphics drawing model for out-of-process plugins

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Benoit Girard (:BenWa)]

A first step at getting OOPP for mac is to get the NPAPI CoreGraphics drawing model negotiation working. 

For now we have the plugin draw in a dummy CGContext without reading back the results.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: CG Negotiation

With the following patch the test plug-in loads and responds to NPCocoaDrawEvents. Other events are not yet supported since we are going for white listing at the moment.

I also bundled this change:
-    if (NPERR_NO_ERROR != *rv) {
-        return false;
-    }
This happens if the plug-in returns an error during NPP_New. It was causing the IPC code to think that the message did not (de)serialize correctly and would not report the error code.

Comment 2

[Benoit Girard (:BenWa)]

Attachment: CG Drawing OOPP

This patch does more then the bug title. It actually implements CG Drawing and a other related negotiation. ipcplugins mochitest pass with this patch.

NOTE: Shmem are resizing when the plugin window' size changes. Currently there is no way to release shmem during without unloading the plugin so right now we will leak memory until that feature is added to shmem.

Comment 3

[Benoit Girard (:BenWa)]

Attachment: CG Drawing

I fixed an abort, CGImageRef leak, logging and #ifdef consistency.

Comment 4

[Benoit Girard (:BenWa)]

Comment on attachment 435067 [details] [diff] [review]
CG Drawing

The ipdl file say we will need a security review since we are modifying them. Might be a good idea to get joe or cjones to look over the shmem usages as well.

Comment 5

[Benoit Girard (:BenWa)]

Attachment: CG Drawing

fixed non osx errors

Comment 6

[Benoit Girard (:BenWa)]

Attachment: CG Drawing

Applied review comments (from IRC)

Comment 7

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

Comment on attachment 435081 [details] [diff] [review]
CG Drawing

>diff --git a/dom/plugins/PluginInstanceChild.cpp b/dom/plugins/PluginInstanceChild.cpp
>--- a/dom/plugins/PluginInstanceChild.cpp
>+++ b/dom/plugins/PluginInstanceChild.cpp
>@@ -77,8 +77,9 @@
> #include <windowsx.h>
> 
> #define NS_OOPP_DOUBLEPASS_MSGID TEXT("MozDoublePassMsg")
>-
>-#endif // defined(OS_WIN)
>+#elif defined(XP_MACOSX)
>+#include <ApplicationServices/ApplicationServices.h>
>+#endif // defined(XP_MACOSX)
> 
> PluginInstanceChild::PluginInstanceChild(const NPPluginFuncs* aPluginIface,
>                                          const nsCString& aMimeType)
>@@ -320,6 +321,45 @@
> #endif
>     }
> 
>+#ifdef XP_MACOSX
>+   case NPNVsupportsCoreGraphicsBool: {
>+        bool v = false;
>+        NPError result;
>+        if (!CallNPN_GetValue_NPNVsupportsCoreGraphicsBool(&v, &result)) {
>+            return NPERR_GENERIC_ERROR;
>+        }
>+        *static_cast<NPBool*>(aValue) = v;
>+        return result;
>+    }
>+
>+    case NPNVsupportsCoreAnimationBool: {
>+        bool v = false;
>+        NPError result;
>+        if (!CallNPN_GetValue_NPNVsupportsCoreAnimationBool(&v, &result)) {
>+            return NPERR_GENERIC_ERROR;
>+        }
>+        *static_cast<NPBool*>(aValue) = v;
>+        return result;
>+    }
>+
>+    case NPNVsupportsCocoaBool: {
>+        bool v = false;
>+        NPError result;
>+        if (!CallNPN_GetValue_NPNVsupportsCocoaBool(&v, &result)) {
>+            return NPERR_GENERIC_ERROR;
>+        }
>+        *static_cast<NPBool*>(aValue) = v;
>+        return result;
>+    }
>+  

If some of these variables are known to be "true" at compile time, it would be better not to ask over IPC.  But I don't know if they are or not.

> bool
>+PluginInstanceChild::AnswerNPP_HandleEvent_Shmem(const NPRemoteEvent& event,
>+                                            Shmem& mem,
>+                                            int16_t* handled,
>+                                            Shmem* rtnmem)

Nit: indentation is weird here.

>+{
>+    PLUGIN_LOG_DEBUG_FUNCTION;
>+    AssertPluginThread();
>+
>+    // We return access to the shared memory buffer after returning.
>+
>+#ifdef XP_MACOSX
>+    NPCocoaEvent evcopy = event.event;
>+
>+    if (evcopy.type == NPCocoaEventDrawRect) {
>+        CGColorSpaceRef cSpace = ::CGColorSpaceCreateWithName(kCGColorSpaceGenericRGB);
>+        if (!cSpace) {
>+            PLUGIN_LOG_DEBUG(("Could not allocate ColorSpace."));
>+            *handled = false;
>+            *rtnmem = mem;
>+            return true;
>+        } 
>+        void* cgContextByte = mem.get<char>();
>+        CGContextRef ctxt = ::CGBitmapContextCreate(cgContextByte, mWindow.width, mWindow.height, 8, mWindow.width * 4, cSpace, kCGImageAlphaPremultipliedFirst | kCGBitmapByteOrder32Host);
>+        ::CGColorSpaceRelease(cSpace);
>+        if (!ctxt) {
>+            PLUGIN_LOG_DEBUG(("Could not allocate CGBitmapContext."));
>+            *handled = false;
>+            *rtnmem = mem;
>+            return true;
>+        }
>+        evcopy.data.draw.context = ctxt; 
>+    } else {
>+        PLUGIN_LOG_DEBUG(("Invalid event type for AnswerNNP_HandleEvent_Shmem."));
>+        *handled = false;
>+        *rtnmem = mem;
>+        return true;
>+    } 
>+
>+    if (!mPluginIface->event) {
>+        *handled = false;
>+        *rtnmem = mem;
>+    } else {
>+        *handled = mPluginIface->event(&mData, reinterpret_cast<void*>(&evcopy));
>+        *rtnmem = mem;
>+    }
>+
>+    // Some events need cleaning up.
>+    if (evcopy.type == NPCocoaEventDrawRect) {
>+        ::CGContextRelease(evcopy.data.draw.context);
>+    }
>+
>+    *rtnmem = mem;

You'll always assign this twice because of the |if (!mPluginIface->event)...| statement above.

>+    return true;
>+#else
>+    PLUGIN_LOG_DEBUG(("Invalid event type for AnswerNNP_HandleEvent_Shmem."));
>+    *rtnmem = mem;
>+    return true;
>+#endif
>+

This should be an NS_RUNTIMEABORT("not reached").  Also, my personal taste is to have separate definitions for do-nothing placeholders, but I'll leave that to your judgment.

>diff --git a/dom/plugins/PluginInstanceParent.cpp b/dom/plugins/PluginInstanceParent.cpp
>--- a/dom/plugins/PluginInstanceParent.cpp
>+++ b/dom/plugins/PluginInstanceParent.cpp
>+bool
>+PluginInstanceParent::AnswerNPN_GetValue_NPNVsupportsCoreGraphicsBool(
>+                                           bool* value, NPError* result)
>+{
>+#ifdef XP_MACOSX
>+    NPBool v;
>+    *result = mNPNIface->getvalue(mNPP, NPNVsupportsCoreGraphicsBool, &v);
>+    *value = v;
>+    return true;
>+#else
>+    *result = NPERR_GENERIC_ERROR;
>+    *value = false;
>+    return true;
>+#endif
>+}
>+
>+bool
>+PluginInstanceParent::AnswerNPN_GetValue_NPNVsupportsCoreAnimationBool(
>+                                           bool* value, NPError* result)
>+{
>+#ifdef XP_MACOSX
>+    //NPBool v;
>+    // For now we will disable it since we don't support it across process yet.
>+    //*result = mNPNIface->getvalue(mNPP, NPNVsupportsCoreAnimationBool, &v);
>+    *value = false;
>+    return true;
>+#else
>+    *result = NPERR_GENERIC_ERROR;
>+    *value = false;
>+    return true;
>+#endif
>+}
>+
>+bool
>+PluginInstanceParent::AnswerNPN_GetValue_NPNVsupportsCocoaBool(
>+                                           bool* value, NPError* result)
>+{
>+#ifdef XP_MACOSX
>+    NPBool v;
>+    *result = mNPNIface->getvalue(mNPP, NPNVsupportsCocoaBool, &v);
>+    *value = v;
>+    return true;
>+#else
>+    *result = NPERR_GENERIC_ERROR;
>+    *value = false;
>+    return true;
>+#endif
>+}
> 
> bool
> PluginInstanceParent::AnswerNPN_SetValue_NPPVpluginWindow(
>@@ -313,6 +367,33 @@
>     return true;
> }
> 
>+bool
>+PluginInstanceParent::AnswerNPN_SetValue_NPPVpluginDrawingModel(
>+    const int& drawingModel, NPError* result)
>+{
>+#ifdef XP_MACOSX
>+    *result = mNPNIface->setvalue(mNPP, NPPVpluginDrawingModel,
>+                                  (void*)drawingModel);
>+    return true;
>+#else
>+    *result = NPERR_GENERIC_ERROR;
>+    return true;
>+#endif
>+}
>+
>+bool
>+PluginInstanceParent::AnswerNPN_SetValue_NPPVpluginEventModel(
>+    const int& eventModel, NPError* result)
>+{
>+#ifdef XP_MACOSX
>+    *result = mNPNIface->setvalue(mNPP, NPPVpluginEventModel,
>+                                  (void*)eventModel);
>+    return true;
>+#else
>+    *result = NPERR_GENERIC_ERROR;
>+    return true;
>+#endif
>+}
> 

All these should be NS_RUNTIMEABORT("not reached") for platforms other than OS X.  You could save some lines of code by putting all the OS X implementations under one ifdef, and putting all the non-XP_MACOSX under another, but no biggie; again up to you.

>+#ifdef XP_MACOSX
>+    if (npevent->type == NPCocoaEventDrawRect) {
>+        if (mShWidth == 0 && mShHeight == 0) {
>+            PLUGIN_LOG_DEBUG(("NPCocoaEventDrawRect on window of size 0."));
>+            return false;
>+        }
>+        if (!mShSurface.IsReadable()) {
>+            PLUGIN_LOG_DEBUG(("Shmem is not readable."));
>+            return false;
>+        }
>+        CallNPP_HandleEvent_Shmem(npremoteevent, mShSurface, &handled, &mShSurface);

You need to check the return code of this call and return false if it does.  It can return false without changing mShSurface permissions.

r=me with these addressed.  Cool stuff, nice work!

Comment 8

[Benoit Girard (:BenWa)]

Attachment: CG Drawing (superreview)

Comment 9

[Benoit Girard (:BenWa)]

Attachment: CG Drawing (superreview)

forgot a review comment.

Comment 10

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

I filed bug 555275 and bug 555276 for the shmem deficiencies.  Just noting because they don't block this bug.

Comment 11

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/edb9b1b565e6