555300 - [OOP] serialize/enable Cocoa NPAPI key events

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Josh Aas]

We need to serialize/enable Cocoa NPAPI key events. This requires serializing a utf16 string buffer.

Comment 1

[Josh Aas]

Attachment: fix v1.0

Comment 2

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

Comment on attachment 435421 [details] [diff] [review]
fix v1.0

>diff --git a/dom/plugins/NPEventOSX.h b/dom/plugins/NPEventOSX.h
>--- a/dom/plugins/NPEventOSX.h
>+++ b/dom/plugins/NPEventOSX.h
>@@ -74,43 +74,87 @@ struct ParamTraits<mozilla::plugins::NPR
>             case NPCocoaEventMouseUp:
>             case NPCocoaEventMouseMoved:
>             case NPCocoaEventMouseEntered:
>             case NPCocoaEventMouseExited:
>             case NPCocoaEventMouseDragged:
>             case NPCocoaEventFocusChanged:
>             case NPCocoaEventWindowFocusChanged:
>             case NPCocoaEventScrollWheel:
>-                // Nothing special to do for these events.
>+                aMsg->WriteBytes(&paramCopy, sizeof(paramType));
>                 break;

These cases should all |return| instead of |break|, since control
flow now always ends at this switch.

>             case NPCocoaEventDrawRect:
>-                // Don't serialize the context pointer
>                 paramCopy.event.data.draw.context = NULL;
>+                aMsg->WriteBytes(&paramCopy, sizeof(paramType));
>+                break;
>+            case NPCocoaEventFlagsChanged:
>+                paramCopy.event.data.key.characters = NULL;
>+                paramCopy.event.data.key.charactersIgnoringModifiers = NULL;
>+                aMsg->WriteBytes(&paramCopy, sizeof(paramType));
>                 break;
>             case NPCocoaEventKeyDown:
>             case NPCocoaEventKeyUp:
>-            case NPCocoaEventFlagsChanged:
>+                paramCopy.event.data.key.characters = NULL;
>+                paramCopy.event.data.key.charactersIgnoringModifiers = NULL;
>+                aMsg->WriteBytes(&paramCopy, sizeof(paramType));
>+                WriteParam(aMsg, aParam.event.data.key.characters);
>+                WriteParam(aMsg, aParam.event.data.key.charactersIgnoringModifiers);
>+                break;
>             case NPCocoaEventTextInput:
>+                paramCopy.event.data.text.text = NULL;
>+                aMsg->WriteBytes(&paramCopy, sizeof(paramType));
>+                WriteParam(aMsg, aParam.event.data.text.text);
>+                break;
>             default:
>                 // ignore any events we don't expect
>-                return; 
>+                return;

This should be |NS_NOTREACHED(); return;|.

>         }
>-
>-        aMsg->WriteBytes(&paramCopy, sizeof(paramType));
>     }
> 
>     static bool Read(const Message* aMsg, void** aIter, paramType* aResult)
>     {
>         const char* bytes = 0;
> 
>         if (!aMsg->ReadBytes(aIter, &bytes, sizeof(paramType))) {
>             return false;
>         }
>         memcpy(aResult, bytes, sizeof(paramType));
> 
>+        switch (aResult->event.type) {
>+            case NPCocoaEventMouseDown:
>+            case NPCocoaEventMouseUp:
>+            case NPCocoaEventMouseMoved:
>+            case NPCocoaEventMouseEntered:
>+            case NPCocoaEventMouseExited:
>+            case NPCocoaEventMouseDragged:
>+            case NPCocoaEventFocusChanged:
>+            case NPCocoaEventWindowFocusChanged:
>+            case NPCocoaEventScrollWheel:
>+            case NPCocoaEventDrawRect:
>+            case NPCocoaEventFlagsChanged:
>+                break;
>+            case NPCocoaEventKeyDown:
>+            case NPCocoaEventKeyUp:            
>+                if (!ReadParam(aMsg, aIter, &aResult->event.data.key.characters)) {
>+                    return false;
>+                }
>+                if (!ReadParam(aMsg, aIter, &aResult->event.data.key.charactersIgnoringModifiers)) {
>+                    return false;
>+                }

Nit: if (!ReadParam(...) ||
         !ReadParam(...))
       return false;

>diff --git a/dom/plugins/PluginMessageUtils.h b/dom/plugins/PluginMessageUtils.h
>--- a/dom/plugins/PluginMessageUtils.h
>+++ b/dom/plugins/PluginMessageUtils.h

This is general enough that it could live in
ipc/glue/IPCMessageUtils.h, but there's no hurry to move it
there.

>@@ -440,16 +440,69 @@ struct ParamTraits<NPString>
>   }
> 
>   static void Log(const paramType& aParam, std::wstring* aLog)
>   {
>     aLog->append(StringPrintf(L"%s", aParam.UTF8Characters));
>   }
> };
> 
>+#ifdef XP_MACOSX
>+template <>
>+struct ParamTraits<NPNSString*>
>+{
>+  typedef NPNSString* paramType;
>+
>+  // Empty string writes a length of 0 and no buffer.
>+  // We don't write a NULL terminating character in buffers.
>+  static void Write(Message* aMsg, const paramType& aParam)
>+  {
>+    CFStringRef cfString = (CFStringRef)aParam;
>+    long length = ::CFStringGetLength(cfString);
>+    WriteParam(aMsg, length);
>+    if (length == 0) {
>+      return;
>+    }
>+

Per IRC conversation, we may be able to avoid allocating here by
using CFStringGetCharactersPtr and falling back on this code if
that fails.  May want to leave an XXX comment to that effect.

>+    UniChar *buffer = (UniChar*)malloc(length * sizeof(UniChar));
>+    if (!buffer) {
>+      return;
>+    }

This is an infallible serializer, so you should use
|moz_xmalloc()| here.  Or if these strings might be really long,
and you want to explicitly eat memory allocation failures, then
allocate this buffer before writing the string length to the
message, and write length=0 if allocation failed.

Looks good on the whole, but I'd like to take a look at the v2
NSString serializer.

Comment 3

[Josh Aas]

Attachment: fix v1.1

Comment 4

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

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

>diff --git a/dom/plugins/PluginMessageUtils.h b/dom/plugins/PluginMessageUtils.h
>--- a/dom/plugins/PluginMessageUtils.h
>+++ b/dom/plugins/PluginMessageUtils.h
>+#ifdef XP_MACOSX
>+template <>
>+struct ParamTraits<NPNSString*>
>+{
>+  static bool Read(const Message* aMsg, void** aIter, paramType* aResult)
>+  {
>+    long length;
>+    if (!ReadParam(aMsg, aIter, &length)) {
>+      return false;
>+    }
>+
>+    UniChar* buffer = nsnull;
>+    if (length != 0) {
>+      aMsg->ReadBytes(aIter, (const char**)&buffer, length * sizeof(UniChar));
>+      if (!buffer) {
>+        return false;
>+      }

This should be |if (!aMsg->ReadBytes(...) || !buffer) return false;|.

>+    }
>+
>+    *aResult = (NPNSString*)::CFStringCreateWithBytes(kCFAllocatorDefault, (UInt8*)buffer,
>+                                                      length * sizeof(UniChar),
>+                                                      kCFStringEncodingUTF16, false);

The docs aren't explicit about what happens with buffer=NULL and length=0, just to want to be sure it does the right thing.

Looks good, r=me.

Comment 5

[Josh Aas]

Attachment: fix v1.2

Comment 6

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/26be64ca9977

Comment 7

[Josh Aas]

(In reply to comment #4)
> (From update of attachment 435670 [details] [diff] [review])
> >+    *aResult = (NPNSString*)::CFStringCreateWithBytes(kCFAllocatorDefault, (UInt8*)buffer,
> >+                                                      length * sizeof(UniChar),
> >+                                                      kCFStringEncodingUTF16, false);
> 
> The docs aren't explicit about what happens with buffer=NULL and length=0, just
> to want to be sure it does the right thing.

It does the right thing, creates an empty CFString.