Backed out changeset e4a7ea0bb90f bug 532208 - asynchronous stream delivery because write events were being dispatched in odd re-entrant states and NPP_DestroyStream was being delivered before all the stream data was delivered.
authorBenjamin Smedberg <benjamin@smedbergs.us>
Thu, 25 Feb 2010 03:00:56 -0800
changeset 38691 7594db5d213a7364f6bd375064eacbc3b80a53ad
parent 38668 e4a7ea0bb90f194343f128880d45f99be27860a7
child 38692 c390c6f279ee5cdc9c83c10c934fe31a1eb94e13
push id11802
push userbsmedberg@mozilla.com
push dateThu, 25 Feb 2010 11:05:10 +0000
treeherderautoland@c390c6f279ee [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs532208
milestone1.9.3a2pre
backs oute4a7ea0bb90f194343f128880d45f99be27860a7
Backed out changeset e4a7ea0bb90f bug 532208 - asynchronous stream delivery because write events were being dispatched in odd re-entrant states and NPP_DestroyStream was being delivered before all the stream data was delivered.
dom/plugins/BrowserStreamChild.cpp
dom/plugins/BrowserStreamChild.h
dom/plugins/BrowserStreamParent.cpp
dom/plugins/BrowserStreamParent.h
dom/plugins/PBrowserStream.ipdl
dom/plugins/PluginInstanceParent.cpp
dom/plugins/PluginModuleChild.cpp
--- a/dom/plugins/BrowserStreamChild.cpp
+++ b/dom/plugins/BrowserStreamChild.cpp
@@ -49,17 +49,16 @@ BrowserStreamChild::BrowserStreamChild(P
                                        const PStreamNotifyChild* notifyData,
                                        const nsCString& headers,
                                        const nsCString& mimeType,
                                        const bool& seekable,
                                        NPError* rv,
                                        uint16_t* stype)
   : mInstance(instance)
   , mClosed(false)
-  , mState(CONSTRUCTING)
   , mURL(url)
   , mHeaders(headers)
 {
   PLUGIN_LOG_DEBUG(("%s (%s, %i, %i, %p, %s, %s)", FULLFUNCTION,
                     url.get(), length, lastmodified, (void*) notifyData,
                     headers.get(), mimeType.get()));
 
   AssertPluginThread();
@@ -87,176 +86,113 @@ BrowserStreamChild::StreamConstructed(
             uint16_t* stype)
 {
   NPError rv = NPERR_NO_ERROR;
 
   *stype = NP_NORMAL;
   rv = mInstance->mPluginIface->newstream(
     &mInstance->mData, const_cast<char*>(NullableStringGet(mimeType)),
     &mStream, seekable, stype);
-  if (rv != NPERR_NO_ERROR) {
+  if (rv != NPERR_NO_ERROR)
     mClosed = true;
-    mState = DELETING;
-  }
-  else {
-    mState = ALIVE;
-  }
 
   return rv;
 }
 
 bool
-BrowserStreamChild::RecvWrite(const int32_t& offset,
-                              const Buffer& data,
-                              const uint32_t& newlength)
+BrowserStreamChild::AnswerNPP_WriteReady(const int32_t& newlength,
+                                         int32_t *size)
 {
   PLUGIN_LOG_DEBUG_FUNCTION;
 
   AssertPluginThread();
 
-  if (ALIVE != mState)
-    NS_RUNTIMEABORT("Unexpected state: received data after NPP_DestroyStream?");
-
-  if (mClosed)
+  if (mClosed) {
+    *size = 0;
     return true;
+  }
 
   mStream.end = newlength;
 
-  NS_ASSERTION(data.Length() > 0, "Empty data");
+  *size = mInstance->mPluginIface->writeready(&mInstance->mData, &mStream);
+  return true;
+}
 
-  PendingData* newdata = mPendingData.AppendElement();
-  newdata->offset = offset;
-  newdata->data = data;
-  newdata->curpos = 0;
+bool
+BrowserStreamChild::AnswerNPP_Write(const int32_t& offset,
+                                    const Buffer& data,
+                                    int32_t* consumed)
+{
+  PLUGIN_LOG_DEBUG(("%s (offset=%i, data.length=%i)", FULLFUNCTION,
+                    offset, data.Length()));
 
-  DeliverData();
+  AssertPluginThread();
 
+  if (mClosed) {
+    *consumed = -1;
+    return true;
+  }
+
+  *consumed = mInstance->mPluginIface->write(&mInstance->mData, &mStream,
+                                             offset, data.Length(),
+                                             const_cast<char*>(data.get()));
   return true;
 }
 
 bool
 BrowserStreamChild::AnswerNPP_StreamAsFile(const nsCString& fname)
 {
   PLUGIN_LOG_DEBUG(("%s (fname=%s)", FULLFUNCTION, fname.get()));
 
   AssertPluginThread();
 
-  if (ALIVE != mState)
-    NS_RUNTIMEABORT("Unexpected state: received file after NPP_DestroyStream?");
-
   if (mClosed)
     return true;
 
   mInstance->mPluginIface->asfile(&mInstance->mData, &mStream,
                                   fname.get());
   return true;
 }
 
 bool
-BrowserStreamChild::RecvNPP_DestroyStream(const NPReason& reason)
-{
-  PLUGIN_LOG_DEBUG_METHOD;
-
-  if (ALIVE != mState)
-    NS_RUNTIMEABORT("Unexpected state: recevied NPP_DestroyStream twice?");
-
-  mState = DYING;
-
-  mClosed = true;
-  mInstance->mPluginIface->destroystream(&mInstance->mData, &mStream, reason);
-
-  SendStreamDestroyed();
-  mState = DELETING;
-
-  return true;
-}
-
-bool
-BrowserStreamChild::Recv__delete__()
+BrowserStreamChild::Answer__delete__(const NPError& reason,
+                                     const bool& artificial)
 {
   AssertPluginThread();
-
-  if (DELETING != mState)
-    NS_RUNTIMEABORT("Bad state, not DELETING");
-
+  if (!artificial)
+    NPP_DestroyStream(reason);
   return true;
 }
 
 NPError
 BrowserStreamChild::NPN_RequestRead(NPByteRange* aRangeList)
 {
   PLUGIN_LOG_DEBUG_FUNCTION;
 
   AssertPluginThread();
 
-  if (ALIVE != mState || mClosed)
-    return NPERR_GENERIC_ERROR;
-
   IPCByteRanges ranges;
   for (; aRangeList; aRangeList = aRangeList->next) {
     IPCByteRange br = {aRangeList->offset, aRangeList->length};
     ranges.push_back(br);
   }
 
   NPError result;
   CallNPN_RequestRead(ranges, &result);
   return result;
 }
 
 void
-BrowserStreamChild::DeliverData()
+BrowserStreamChild::NPP_DestroyStream(NPError reason)
 {
-  if (ALIVE != mState || mClosed) {
-    ClearSuspendedTimer();
-    return;
-  }
+  PLUGIN_LOG_DEBUG(("%s (reason=%i)", FULLFUNCTION, reason));
+
+  AssertPluginThread();
 
-  while (mPendingData.Length()) {
-    PendingData& cur = mPendingData[0];
-    while (cur.curpos < cur.data.Length()) {
-      int32_t r = mInstance->mPluginIface->writeready(&mInstance->mData, &mStream);
-      if (r == 0) {
-        SetSuspendedTimer();
-        return;
-      }
-      r = mInstance->mPluginIface->write(
-        &mInstance->mData, &mStream,
-        cur.offset + cur.curpos, // offset
-        cur.data.Length() - cur.curpos, // length
-        const_cast<char*>(cur.data.BeginReading() + cur.curpos));
-      if (r == 0) {
-        SetSuspendedTimer();
-        return;
-      }
-      if (r < 0) { // error condition
-        if (ALIVE == mState && !mClosed) { // re-check
-          mClosed = true;
-          SendNPN_DestroyStream(NPRES_NETWORK_ERR);
-        }
-        ClearSuspendedTimer();
-        return;
-      }
-      cur.curpos += r;
-    }
-    mPendingData.RemoveElementAt(0);
-  }
+  if (mClosed)
+    return;
 
-  ClearSuspendedTimer();
-}
-
-void
-BrowserStreamChild::SetSuspendedTimer()
-{
-  if (mSuspendedTimer.IsRunning())
-    return;
-  mSuspendedTimer.Start(
-    base::TimeDelta::FromMilliseconds(100),
-    this, &BrowserStreamChild::DeliverData);
-}
-
-void
-BrowserStreamChild::ClearSuspendedTimer()
-{
-  mSuspendedTimer.Stop();
+  mInstance->mPluginIface->destroystream(&mInstance->mData, &mStream, reason);
+  mClosed = true;
 }
 
 } /* namespace plugins */
 } /* namespace mozilla */
--- a/dom/plugins/BrowserStreamChild.h
+++ b/dom/plugins/BrowserStreamChild.h
@@ -69,64 +69,45 @@ public:
             const uint32_t& length,
             const uint32_t& lastmodified,
             PStreamNotifyChild* notifyData,
             const nsCString& headers,
             const nsCString& mimeType,
             const bool& seekable,
             uint16_t* stype);
 
-  virtual bool RecvWrite(const int32_t& offset,
-                         const Buffer& data,
-                         const uint32_t& newsize);
+  virtual bool AnswerNPP_WriteReady(const int32_t& newlength,
+                                        int32_t *size);
+  virtual bool AnswerNPP_Write(const int32_t& offset,
+                                   const Buffer& data,
+                                   int32_t* consumed);
+
   virtual bool AnswerNPP_StreamAsFile(const nsCString& fname);
-  virtual bool RecvNPP_DestroyStream(const NPReason& reason);
-  virtual bool Recv__delete__();
+  virtual bool Answer__delete__(const NPError& reason,
+                                const bool& artificial);
+
 
   void EnsureCorrectInstance(PluginInstanceChild* i)
   {
     if (i != mInstance)
       NS_RUNTIMEABORT("Incorrect stream instance");
   }
   void EnsureCorrectStream(NPStream* s)
   {
     if (s != &mStream)
       NS_RUNTIMEABORT("Incorrect stream data");
   }
 
   NPError NPN_RequestRead(NPByteRange* aRangeList);
+  void NPP_DestroyStream(NPError reason);
 
 private:
-  /**
-   * Deliver the data currently in mPending, scheduling
-   * or cancelling the suspended timer as needed.
-   */
-  void DeliverData();
-  void SetSuspendedTimer();
-  void ClearSuspendedTimer();
-
   PluginInstanceChild* mInstance;
   NPStream mStream;
   bool mClosed;
-  enum {
-    CONSTRUCTING,
-    ALIVE,
-    DYING,
-    DELETING
-  } mState;
   nsCString mURL;
   nsCString mHeaders;
-
-  struct PendingData
-  {
-    int32_t offset;
-    Buffer data;
-    int32_t curpos;
-  };
-  nsTArray<PendingData> mPendingData;
-
-  base::RepeatingTimer<BrowserStreamChild> mSuspendedTimer;
 };
 
 } // namespace plugins
 } // namespace mozilla
 
 #endif /* mozilla_plugins_BrowserStreamChild_h */
--- a/dom/plugins/BrowserStreamParent.cpp
+++ b/dom/plugins/BrowserStreamParent.cpp
@@ -1,52 +1,34 @@
 /* -*- Mode: C++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 8 -*- */
 
 #include "BrowserStreamParent.h"
 #include "PluginInstanceParent.h"
 
-// How much data are we willing to send across the wire
-// in one chunk?
-static const int32_t kSendDataChunk = 0x1000;
-
 namespace mozilla {
 namespace plugins {
 
 BrowserStreamParent::BrowserStreamParent(PluginInstanceParent* npp,
                                          NPStream* stream)
   : mNPP(npp)
   , mStream(stream)
-  , mState(ALIVE)
 {
   mStream->pdata = static_cast<AStream*>(this);
 }
 
 BrowserStreamParent::~BrowserStreamParent()
 {
 }
 
 bool
 BrowserStreamParent::AnswerNPN_RequestRead(const IPCByteRanges& ranges,
                                            NPError* result)
 {
   PLUGIN_LOG_DEBUG_FUNCTION;
 
-  switch (mState) {
-  case ALIVE:
-    break;
-
-  case DYING:
-    *result = NPERR_GENERIC_ERROR;
-    return true;
-
-  default:
-    NS_ERROR("Unexpected state");
-    return false;
-  }
-
   if (!mStream)
     return false;
 
   if (ranges.size() > PR_INT32_MAX)
     return false;
 
   nsAutoArrayPtr<NPByteRange> rp(new NPByteRange[ranges.size()]);
   for (PRUint32 i = 0; i < ranges.size(); ++i) {
@@ -56,87 +38,62 @@ BrowserStreamParent::AnswerNPN_RequestRe
   }
   rp[ranges.size() - 1].next = NULL;
 
   *result = mNPP->mNPNIface->requestread(mStream, rp);
   return true;
 }
 
 bool
-BrowserStreamParent::RecvNPN_DestroyStream(const NPReason& reason)
+BrowserStreamParent::Answer__delete__(const NPError& reason,
+                                      const bool& artificial)
 {
-  switch (mState) {
-  case ALIVE:
-    break;
-
-  case DYING:
-    return true;
-
-  default:
-    NS_ERROR("Unexpected state");
-    return false;
-  };
-
-  mNPP->mNPNIface->destroystream(mNPP->mNPP, mStream, reason);
-  return true;
-}
-
-void
-BrowserStreamParent::NPP_DestroyStream(NPReason reason)
-{
-  NS_ASSERTION(ALIVE == mState, "NPP_DestroyStream called twice?");
-  mState = DYING;
-  SendNPP_DestroyStream(reason);
-}
-
-bool
-BrowserStreamParent::RecvStreamDestroyed()
-{
-  if (DYING != mState) {
-    NS_ERROR("Unexpected state");
-    return false;
-  }
-
-  mState = DELETING;
-  Send__delete__(this);
+  if (!artificial)
+    NPN_DestroyStream(reason);
   return true;
 }
 
 int32_t
 BrowserStreamParent::WriteReady()
 {
-  return kSendDataChunk;
+  PLUGIN_LOG_DEBUG_FUNCTION;
+
+  int32_t result;
+  if (!CallNPP_WriteReady(mStream->end, &result))
+    return -1;
+
+  return result;
 }
 
 int32_t
 BrowserStreamParent::Write(int32_t offset,
                            int32_t len,
                            void* buffer)
 {
   PLUGIN_LOG_DEBUG_FUNCTION;
 
-  NS_ASSERTION(ALIVE == mState, "Sending data after NPP_DestroyStream?");
-  NS_ASSERTION(len > 0, "Non-positive length to NPP_Write");
-
-  if (len > kSendDataChunk)
-    len = kSendDataChunk;
+  int32_t result;
+  if (!CallNPP_Write(offset,
+                     nsCString(static_cast<char*>(buffer), len),
+                     &result))
+    return -1;
 
-  SendWrite(offset,
-    nsCString(static_cast<char*>(buffer), len),
-    mStream->end);
-
-  return len;
+  return result;
 }
 
 void
 BrowserStreamParent::StreamAsFile(const char* fname)
 {
   PLUGIN_LOG_DEBUG_FUNCTION;
 
-  NS_ASSERTION(ALIVE == mState,
-               "Calling streamasfile after NPP_DestroyStream?");
+  CallNPP_StreamAsFile(nsCString(fname));
+}
 
-  CallNPP_StreamAsFile(nsCString(fname));
-  return;
+NPError
+BrowserStreamParent::NPN_DestroyStream(NPReason reason)
+{
+  PLUGIN_LOG_DEBUG_FUNCTION;
+
+  return mNPP->mNPNIface->destroystream(mNPP->mNPP, mStream, reason);
 }
 
 } // namespace plugins
 } // namespace mozilla
--- a/dom/plugins/BrowserStreamParent.h
+++ b/dom/plugins/BrowserStreamParent.h
@@ -56,35 +56,26 @@ public:
                       NPStream* stream);
   virtual ~BrowserStreamParent();
 
   NS_OVERRIDE virtual bool IsBrowserStream() { return true; }
 
   virtual bool AnswerNPN_RequestRead(const IPCByteRanges& ranges,
                                      NPError* result);
 
-  virtual bool RecvNPN_DestroyStream(const NPReason& reason);
-
-  virtual bool RecvStreamDestroyed();
+  virtual bool
+  Answer__delete__(const NPError& reason, const bool& artificial);
 
   int32_t WriteReady();
   int32_t Write(int32_t offset, int32_t len, void* buffer);
   void StreamAsFile(const char* fname);
 
-  void NPP_DestroyStream(NPReason reason);
-
 private:
-  using PBrowserStreamParent::SendNPP_DestroyStream;
+  NPError NPN_DestroyStream(NPError reason);
 
   PluginInstanceParent* mNPP;
   NPStream* mStream;
-
-  enum {
-    ALIVE,
-    DYING,
-    DELETING
-  } mState;
 };
 
 } // namespace plugins
 } // namespace mozilla
 
 #endif
--- a/dom/plugins/PBrowserStream.ipdl
+++ b/dom/plugins/PBrowserStream.ipdl
@@ -52,50 +52,32 @@ namespace plugins {
  * NPBrowserStream represents a NPStream sent from the browser to the plugin.
  */
 
 rpc protocol PBrowserStream
 {
   manager PPluginInstance;
 
 child:
-  async Write(int32_t offset, Buffer data,
-              uint32_t newlength);
-  rpc NPP_StreamAsFile(nsCString fname);
+  rpc NPP_WriteReady(int32_t newlength)
+    returns (int32_t size);
 
-  /**
-   * NPP_DestroyStream may race with other messages: the child acknowledges
-   * the message with StreamDestroyed before this actor is deleted.
-   */
-  async NPP_DestroyStream(NPReason reason);
-  async __delete__();
+  rpc NPP_Write(int32_t offset,
+                Buffer data)
+    returns (int32_t consumed);
+
+  rpc NPP_StreamAsFile(nsCString fname);
 
 parent:
   rpc NPN_RequestRead(IPCByteRanges ranges)
     returns (NPError result);
-  async NPN_DestroyStream(NPReason reason);
-  async StreamDestroyed();
 
-/*
-  TODO: turn on state machine.
-
-  // need configurable start state: if the constructor
-  // returns an error in result, start state should
-  // be DELETING.
-start state ALIVE:
-  send Write goto ALIVE;
-  call NPP_StreamAsFile goto ALIVE;
-  send NPP_DestroyStream goto ALIVE;
-  answer NPN_RequestRead goto ALIVE;
-  recv NPN_DestroyStream goto DYING;
-
-state DYING:
-  answer NPN_RequestRead goto DYING;
-  recv NPN_DestroyStream goto DYING;
-  recv StreamDestroyed goto DELETING;
-
-state DELETING:
-  send __delete__;
-*/
+both:
+  /**
+   * ~PBrowserStream is for both NPN_DestroyStream and NPP_DestroyStream.
+   * @param artificial True when the stream is closed as a by-product of
+   *                        some other call (such as a failure in NPP_Write).
+   */
+  rpc __delete__(NPReason reason, bool artificial);
 };
 
 } // namespace plugins
 } // namespace mozilla
--- a/dom/plugins/PluginInstanceParent.cpp
+++ b/dom/plugins/PluginInstanceParent.cpp
@@ -635,17 +635,17 @@ PluginInstanceParent::NPP_NewStream(NPMI
                                        stream->lastmodified,
                                        static_cast<PStreamNotifyParent*>(stream->notifyData),
                                        NullableString(stream->headers),
                                        NullableString(type), seekable,
                                        &err, stype))
         return NPERR_GENERIC_ERROR;
 
     if (NPERR_NO_ERROR != err)
-        PBrowserStreamParent::Send__delete__(bs);
+        PBrowserStreamParent::Call__delete__(bs, NPERR_GENERIC_ERROR, true);
 
     return err;
 }
 
 NPError
 PluginInstanceParent::NPP_DestroyStream(NPStream* stream, NPReason reason)
 {
     PLUGIN_LOG_DEBUG(("%s (stream=%p, reason=%i)",
@@ -653,17 +653,17 @@ PluginInstanceParent::NPP_DestroyStream(
 
     AStream* s = static_cast<AStream*>(stream->pdata);
     if (s->IsBrowserStream()) {
         BrowserStreamParent* sp =
             static_cast<BrowserStreamParent*>(s);
         if (sp->mNPP != this)
             NS_RUNTIMEABORT("Mismatched plugin data");
 
-        sp->NPP_DestroyStream(reason);
+        PBrowserStreamParent::Call__delete__(sp, reason, false);
         return NPERR_NO_ERROR;
     }
     else {
         PluginStreamParent* sp =
             static_cast<PluginStreamParent*>(s);
         if (sp->mInstance != this)
             NS_RUNTIMEABORT("Mismatched plugin data");
 
--- a/dom/plugins/PluginModuleChild.cpp
+++ b/dom/plugins/PluginModuleChild.cpp
@@ -812,17 +812,17 @@ NPError NP_CALLBACK
     PLUGIN_LOG_DEBUG_FUNCTION;
     AssertPluginThread();
 
     PluginInstanceChild* p = InstCast(aNPP);
     AStream* s = static_cast<AStream*>(aStream->ndata);
     if (s->IsBrowserStream()) {
         BrowserStreamChild* bs = static_cast<BrowserStreamChild*>(s);
         bs->EnsureCorrectInstance(p);
-        bs->SendNPN_DestroyStream(aReason);
+        PBrowserStreamChild::Call__delete__(bs, aReason, false);
     }
     else {
         PluginStreamChild* ps = static_cast<PluginStreamChild*>(s);
         ps->EnsureCorrectInstance(p);
         PPluginStreamChild::Call__delete__(ps, aReason, false);
     }
     return NPERR_NO_ERROR;
 }