bug 517923: support serializing ns*Strings that represent NULL, use this mechanism in PluginInstanceParent/PluginModuleChild. also add basic crash-handling to *Channel code and some NS_OVERRIDE annotations.

dom/plugins/PluginInstanceParent.cpp

@@ -157,7 +157,9 @@ PluginInstanceParent::AnswerNPN_GetURL(const nsCString& url,
                                        const nsCString& target,
                                        NPError* result)
 {
-    *result = mNPNIface->geturl(mNPP, url.get(), target.get());
+    *result = mNPNIface->geturl(mNPP,
+                                NullableStringGet(url),
+                                NullableStringGet(target));
     return true;
 }
 
@@ -184,11 +186,15 @@ PluginInstanceParent::PStreamNotifyConstructor(const nsCString& url,
     StreamNotifyParent* notifyData = new StreamNotifyParent();
 
     if (!post) {
-        *result = mNPNIface->geturlnotify(mNPP, url.get(), target.get(),
+        *result = mNPNIface->geturlnotify(mNPP,
+                                          NullableStringGet(url),
+                                          NullableStringGet(target),
                                           notifyData);
     }
     else {
-        *result = mNPNIface->posturlnotify(mNPP, url.get(), target.get(),
+        *result = mNPNIface->posturlnotify(mNPP,
+                                           NullableStringGet(url),
+                                           NullableStringGet(target),
                                            buffer.Length(), buffer.get(),
                                            file, notifyData);
     }
@@ -266,6 +272,7 @@ PluginInstanceParent::NPP_NewStream(NPMIMEType type, NPStream* stream,
     BrowserStreamParent* bs = new BrowserStreamParent(this, stream);
 
     NPError err;
+    // TODO are any of these strings nullable?
     if (!CallPBrowserStreamConstructor(bs,
                                        nsCString(stream->url),
                                        stream->end,

dom/plugins/PluginMessageUtils.h

@@ -84,6 +84,29 @@ typedef nsCString Buffer;
 
 } /* namespace mozilla */
 
+
+namespace {
+
+// in NPAPI, char* == NULL is sometimes meaningful.  the following is
+// helper code for dealing with nullable nsCString's
+nsCString
+NullableString(const char* aString)
+{
+    if (!aString) {
+        nsCString str;
+        str.SetIsVoid(PR_TRUE);
+        return str;
+    }
+    return nsCString(aString);
+}
+
+} // namespace <anon>
+
+// TODO is there any safe way for this to be a function?
+#define NullableStringGet(__string)                     \
+    ( __string.IsVoid() ? NULL : __string.get())
+
+
 namespace IPC {
 
 template <>

dom/plugins/PluginModuleChild.cpp

@@ -59,9 +59,10 @@ using mozilla::ipc::NPRemoteIdentifier;
 using namespace mozilla::plugins;
 
 namespace {
-    PluginModuleChild* gInstance = nsnull;
+PluginModuleChild* gInstance = nsnull;
 }
 
+
 PluginModuleChild::PluginModuleChild() :
     mLibrary(0),
     mInitializeFunc(0),
@@ -451,11 +452,11 @@ _geturlnotify(NPP aNPP,
 {
     _MOZ_LOG(__FUNCTION__);
 
-    const nsDependentCString url(aRelativeURL);
+    nsCString url = NullableString(aRelativeURL);
     NPError err;
     InstCast(aNPP)->CallPStreamNotifyConstructor(
         new StreamNotifyChild(url, aNotifyData),
-        url, nsDependentCString(aTarget), false, nsCString(), false, &err);
+        url, NullableString(aTarget), false, nsCString(), false, &err);
     // TODO: what if this fails?
     return err;
 }
@@ -486,8 +487,8 @@ _geturl(NPP aNPP,
 {
     _MOZ_LOG(__FUNCTION__);
     NPError err;
-    InstCast(aNPP)->CallNPN_GetURL(nsDependentCString(aRelativeURL),
+    InstCast(aNPP)->CallNPN_GetURL(NullableString(aRelativeURL),
-                                   nsDependentCString(aTarget), &err);
+                                   NullableString(aTarget), &err);
     return err;
 }
 
@@ -502,11 +503,12 @@ _posturlnotify(NPP aNPP,
 {
     _MOZ_LOG(__FUNCTION__);
 
-    const nsDependentCString url(aRelativeURL);
+    nsCString url = NullableString(aRelativeURL);
     NPError err;
+    // FIXME what should happen when |aBuffer| is null?
     InstCast(aNPP)->CallPStreamNotifyConstructor(
         new StreamNotifyChild(url, aNotifyData),
-        url, nsDependentCString(aTarget), true,
+        url, NullableString(aTarget), true,
         nsDependentCString(aBuffer, aLength), aIsFile, &err);
     // TODO: what if this fails?
     return err;
@@ -522,8 +524,9 @@ _posturl(NPP aNPP,
 {
     _MOZ_LOG(__FUNCTION__);
     NPError err;
-    InstCast(aNPP)->CallNPN_PostURL(nsDependentCString(aRelativeURL),
+    // FIXME what should happen when |aBuffer| is null?
-                                    nsDependentCString(aTarget),
+    InstCast(aNPP)->CallNPN_PostURL(NullableString(aRelativeURL),
+                                    NullableString(aTarget),
                                     nsDependentCString(aBuffer, aLength),
                                     aIsFile, &err);
     return err;
@@ -1026,8 +1029,8 @@ PluginModuleChild::AnswerPPluginInstanceConstructor(PPluginInstanceChild* aActor
 
     printf ("(plugin args: ");
     for (int i = 0; i < argc; ++i) {
-        argn[i] = const_cast<char*>(aNames[i].get());
+        argn[i] = const_cast<char*>(NullableStringGet(aNames[i]));
-        argv[i] = const_cast<char*>(aValues[i].get());
+        argv[i] = const_cast<char*>(NullableStringGet(aValues[i]));
         printf("%s=%s, ", argn[i], argv[i]);
     }
     printf(")\n");
@@ -1035,7 +1038,7 @@ PluginModuleChild::AnswerPPluginInstanceConstructor(PPluginInstanceChild* aActor
     NPP npp = childInstance->GetNPP();
 
     // FIXME/cjones: use SAFE_CALL stuff
-    *rv = mFunctions.newp((char*)aMimeType.get(),
+    *rv = mFunctions.newp((char*)NullableStringGet(aMimeType),
                           npp,
                           aMode,
                           argc,

ipc/glue/AsyncChannel.cpp

@@ -112,9 +112,11 @@ AsyncChannel::Close()
 bool
 AsyncChannel::Send(Message* msg)
 {
-    NS_ASSERTION(ChannelConnected == mChannelState,
+    NS_ABORT_IF_FALSE(MSG_ROUTING_NONE != msg->routing_id(), "need a route");
-                 "trying to Send() to a channel not yet open");
+
-    NS_PRECONDITION(MSG_ROUTING_NONE != msg->routing_id(), "need a route");
+    if (!Connected())
+        // trying to Send() to a closed or error'd channel
+        return false;
 
     mIOLoop->PostTask(FROM_HERE,
                       NewRunnableMethod(this, &AsyncChannel::OnSend, msg));

ipc/glue/AsyncChannel.h

@@ -120,11 +120,15 @@ public:
     bool Send(Message* msg);
 
     // Implement the IPC::Channel::Listener interface
-    virtual void OnMessageReceived(const Message& msg);
+    NS_OVERRIDE virtual void OnMessageReceived(const Message& msg);
-    virtual void OnChannelConnected(int32 peer_pid);
+    NS_OVERRIDE virtual void OnChannelConnected(int32 peer_pid);
-    virtual void OnChannelError();
+    NS_OVERRIDE virtual void OnChannelError();
 
 protected:
+    bool Connected() {
+        return ChannelConnected == mChannelState;
+    }
+
     // Additional methods that execute on the worker thread
     void OnDispatchMessage(const Message& aMsg);
 

ipc/glue/IPCMessageUtils.h

@@ -1,3 +1,5 @@
+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim: set sw=2 ts=8 et tw=80 : */
 /* ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
  *
@@ -56,6 +58,13 @@ struct ParamTraits<nsACString>
 
   static void Write(Message* aMsg, const paramType& aParam)
   {
+    bool isVoid = aParam.IsVoid();
+    aMsg->WriteBool(isVoid);
+
+    if (isVoid)
+      // represents a NULL pointer
+      return;
+
     PRUint32 length = aParam.Length();
     WriteParam(aMsg, length);
     aMsg->WriteBytes(aParam.BeginReading(), length);
@@ -63,6 +72,15 @@ struct ParamTraits<nsACString>
 
   static bool Read(const Message* aMsg, void** aIter, paramType* aResult)
   {
+    bool isVoid;
+    if (!aMsg->ReadBool(aIter, &isVoid))
+      return false;
+
+    if (isVoid) {
+      aResult->SetIsVoid(PR_TRUE);
+      return true;
+    }
+
     PRUint32 length;
     if (ReadParam(aMsg, aIter, &length)) {
       const char* buf;
@@ -76,7 +94,10 @@ struct ParamTraits<nsACString>
 
   static void Log(const paramType& aParam, std::wstring* aLog)
   {
-    aLog->append(UTF8ToWide(aParam.BeginReading()));
+    if (aParam.IsVoid())
+      aLog->append(L"(NULL)");
+    else
+      aLog->append(UTF8ToWide(aParam.BeginReading()));
   }
 };
 
@@ -87,6 +108,13 @@ struct ParamTraits<nsAString>
 
   static void Write(Message* aMsg, const paramType& aParam)
   {
+    bool isVoid = aParam.IsVoid();
+    aMsg->WriteBool(isVoid);
+
+    if (isVoid)
+      // represents a NULL pointer
+      return;
+
     PRUint32 length = aParam.Length();
     WriteParam(aMsg, length);
     aMsg->WriteBytes(aParam.BeginReading(), length * sizeof(PRUnichar));
@@ -94,6 +122,15 @@ struct ParamTraits<nsAString>
 
   static bool Read(const Message* aMsg, void** aIter, paramType* aResult)
   {
+    bool isVoid;
+    if (!aMsg->ReadBool(aIter, &isVoid))
+      return false;
+
+    if (isVoid) {
+      aResult->SetIsVoid(PR_TRUE);
+      return true;
+    }
+
     PRUint32 length;
     if (ReadParam(aMsg, aIter, &length)) {
       const PRUnichar* buf;
@@ -108,14 +145,18 @@ struct ParamTraits<nsAString>
 
   static void Log(const paramType& aParam, std::wstring* aLog)
   {
+    if (aParam.IsVoid())
+      aLog->append(L"(NULL)");
+    else {
 #ifdef WCHAR_T_IS_UTF16
-    aLog->append(reinterpret_cast<const wchar_t*>(aParam.BeginReading()));
+      aLog->append(reinterpret_cast<const wchar_t*>(aParam.BeginReading()));
 #else
-    PRUint32 length = aParam.Length();
+      PRUint32 length = aParam.Length();
-    for (PRUint32 index = 0; index < length; index++) {
+      for (PRUint32 index = 0; index < length; index++) {
-      aLog->push_back(std::wstring::value_type(aParam[index]));
+        aLog->push_back(std::wstring::value_type(aParam[index]));
-    }
+      }
 #endif
+    }
   }
 };
 

ipc/glue/RPCChannel.cpp

@@ -60,10 +60,12 @@ RPCChannel::Call(Message* msg, Message* reply)
 {
     NS_ABORT_IF_FALSE(!ProcessingSyncMessage(),
                       "violation of sync handler invariant");
-    NS_ASSERTION(ChannelConnected == mChannelState,
+    NS_ABORT_IF_FALSE(msg->is_rpc(),
-                 "trying to Send() to a channel not yet open");
+                      "can only Call() RPC messages here");
-    NS_PRECONDITION(msg->is_rpc(),
+
-                    "can only Call() RPC messages here");
+    if (!Connected())
+        // trying to Send() to a closed or error'd channel
+        return false;
 
     MutexAutoLock lock(mMutex);
 
@@ -77,10 +79,14 @@ RPCChannel::Call(Message* msg, Message* reply)
     while (1) {
         // here we're waiting for something to happen. see long
         // comment about the queue in RPCChannel.h
-        while (mPending.empty()) {
+        while (Connected() && mPending.empty()) {
             mCvar.Wait();
         }
 
+        if (!Connected())
+            // FIXME more sophisticated error handling
+            return false;
+
         Message recvd = mPending.front();
         mPending.pop();
 
@@ -363,5 +369,25 @@ RPCChannel::OnMessageReceived(const Message& msg)
 }
 
 
+void
+RPCChannel::OnChannelError()
+{
+    {
+        MutexAutoLock lock(mMutex);
+
+        mChannelState = ChannelError;
+
+        if (AwaitingSyncReply()
+            || 0 < StackDepth()) {
+            mCvar.Notify();
+        }
+    }
+
+    // skip SyncChannel::OnError(); we subsume its duties
+
+    return AsyncChannel::OnChannelError();
+}
+
+
 } // namespace ipc
 } // namespace mozilla

ipc/glue/RPCChannel.h

@@ -80,7 +80,8 @@ public:
     bool Call(Message* msg, Message* reply);
 
     // Override the SyncChannel handler so we can dispatch RPC messages
-    virtual void OnMessageReceived(const Message& msg);
+    NS_OVERRIDE virtual void OnMessageReceived(const Message& msg);
+    NS_OVERRIDE virtual void OnChannelError();
 
 protected:
     // Only exists because we can't schedule SyncChannel::OnDispatchMessage

ipc/glue/SyncChannel.cpp

@@ -59,18 +59,26 @@ SyncChannel::Send(Message* msg, Message* reply)
 {
     NS_ABORT_IF_FALSE(!ProcessingSyncMessage(),
                       "violation of sync handler invariant");
-    NS_ASSERTION(ChannelConnected == mChannelState,
+    NS_ABORT_IF_FALSE(msg->is_sync(), "can only Send() sync messages here");
-                 "trying to Send() to a channel not yet open");
+
-    NS_PRECONDITION(msg->is_sync(), "can only Send() sync messages here");
+    if (!Connected())
+        // trying to Send() to a closed or error'd channel
+        return false;
 
     MutexAutoLock lock(mMutex);
 
     mPendingReply = msg->type() + 1;
-    /*assert*/AsyncChannel::Send(msg);
+    if (!AsyncChannel::Send(msg))
+        // FIXME more sophisticated error handling
+        return false;
 
     // wait for the next sync message to arrive
     mCvar.Wait();
 
+    if (!Connected())
+        // FIXME more sophisticated error handling
+        return false;
+
     // we just received a synchronous message from the other side.
     // If it's not the reply we were awaiting, there's a serious
     // error: either a mistimed/malformed message or a sync in-message
@@ -156,6 +164,22 @@ SyncChannel::OnMessageReceived(const Message& msg)
     }
 }
 
+void
+SyncChannel::OnChannelError()
+{
+    {
+        MutexAutoLock lock(mMutex);
+
+        mChannelState = ChannelError;
+
+        if (AwaitingSyncReply()) {
+            mCvar.Notify();
+        }
+    }
+
+    return AsyncChannel::OnChannelError();
+}
+
 void
 SyncChannel::OnSendReply(Message* aReply)
 {

ipc/glue/SyncChannel.h

@@ -84,7 +84,8 @@ public:
     bool Send(Message* msg, Message* reply);
 
     // Override the AsyncChannel handler so we can dispatch sync messages
-    virtual void OnMessageReceived(const Message& msg);
+    NS_OVERRIDE virtual void OnMessageReceived(const Message& msg);
+    NS_OVERRIDE virtual void OnChannelError();
 
 protected:
     // Executed on the worker thread