Bug 551049 (was bug 532208) part 2 - Delay delivery of NPP_DestroyStream until stream data is delivered, and make sure that data delivery doesn't re-enter, r=bent
authorBenjamin Smedberg <benjamin@smedbergs.us>
Sat, 06 Mar 2010 16:02:31 -0500
changeset 39282 d5aa9bd8864d3ee31a4a977a41c61128e30d1390
parent 39281 b594d5b63dd0cceb44f5f47f290981ca6cb414fd
child 39283 beed4bc507c9c6a48cbaa7f828bf3304eb97931d
push id12114
push userbsmedberg@mozilla.com
push dateThu, 11 Mar 2010 16:57:13 +0000
treeherdermozilla-central@dc3af645f713 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbent
bugs551049, 532208
milestone1.9.3a3pre
Bug 551049 (was bug 532208) part 2 - Delay delivery of NPP_DestroyStream until stream data is delivered, and make sure that data delivery doesn't re-enter, r=bent
dom/plugins/BrowserStreamChild.cpp
dom/plugins/BrowserStreamChild.h
dom/plugins/PluginModuleChild.cpp
--- a/dom/plugins/BrowserStreamChild.cpp
+++ b/dom/plugins/BrowserStreamChild.cpp
@@ -52,16 +52,18 @@ BrowserStreamChild::BrowserStreamChild(P
                                        const bool& seekable,
                                        NPError* rv,
                                        uint16_t* stype)
   : mInstance(instance)
   , mClosed(false)
   , mState(CONSTRUCTING)
   , mURL(url)
   , mHeaders(headers)
+  , mDestroyPending(kDestroyNotPending)
+  , mDeliverDataTracker(this)
 {
   PLUGIN_LOG_DEBUG(("%s (%s, %i, %i, %p, %s, %s)", FULLFUNCTION,
                     url.get(), length, lastmodified, (void*) notifyData,
                     headers.get(), mimeType.get()));
 
   AssertPluginThread();
 
   memset(&mStream, 0, sizeof(mStream));
@@ -122,17 +124,19 @@ BrowserStreamChild::RecvWrite(const int3
 
   NS_ASSERTION(data.Length() > 0, "Empty data");
 
   PendingData* newdata = mPendingData.AppendElement();
   newdata->offset = offset;
   newdata->data = data;
   newdata->curpos = 0;
 
-  DeliverData();
+  if (mDeliverDataTracker.empty())
+    MessageLoop::current()->PostTask(FROM_HERE,
+      mDeliverDataTracker.NewRunnableMethod(&BrowserStreamChild::DeliverData));
 
   return true;
 }
 
 bool
 BrowserStreamChild::AnswerNPP_StreamAsFile(const nsCString& fname)
 {
   PLUGIN_LOG_DEBUG(("%s (fname=%s)", FULLFUNCTION, fname.get()));
@@ -154,22 +158,21 @@ bool
 BrowserStreamChild::RecvNPP_DestroyStream(const NPReason& reason)
 {
   PLUGIN_LOG_DEBUG_METHOD;
 
   if (ALIVE != mState)
     NS_RUNTIMEABORT("Unexpected state: recevied NPP_DestroyStream twice?");
 
   mState = DYING;
+  mDestroyPending = reason;
 
-  mClosed = true;
-  mInstance->mPluginIface->destroystream(&mInstance->mData, &mStream, reason);
-
-  SendStreamDestroyed();
-  mState = DELETING;
+  if (mDeliverDataTracker.empty())
+    MessageLoop::current()->PostTask(FROM_HERE,
+      mDeliverDataTracker.NewRunnableMethod(&BrowserStreamChild::DeliverData));
 
   return true;
 }
 
 bool
 BrowserStreamChild::Recv__delete__()
 {
   AssertPluginThread();
@@ -197,57 +200,98 @@ BrowserStreamChild::NPN_RequestRead(NPBy
   }
 
   NPError result;
   CallNPN_RequestRead(ranges, &result);
   return result;
 }
 
 void
+BrowserStreamChild::NPN_DestroyStream(NPReason reason)
+{
+  mClosed = true;
+  if (ALIVE == mState)
+    SendNPN_DestroyStream(NPRES_NETWORK_ERR);
+
+  ClearSuspendedTimer();
+  mPendingData.Clear();
+  MaybeDeliverNPP_DestroyStream();
+}
+
+void
 BrowserStreamChild::DeliverData()
 {
-  if (ALIVE != mState || mClosed) {
+  if (mState != ALIVE && mState != DYING)
+    NS_RUNTIMEABORT("Unexpected state");
+
+  if (mClosed) {
     ClearSuspendedTimer();
+    MaybeDeliverNPP_DestroyStream();
     return;
   }
 
   while (mPendingData.Length()) {
-    PendingData& cur = mPendingData[0];
-    while (cur.curpos < cur.data.Length()) {
+    while (mPendingData[0].curpos < mPendingData[0].data.Length()) {
       int32_t r = mInstance->mPluginIface->writeready(&mInstance->mData, &mStream);
       if (r == 0) {
         SetSuspendedTimer();
         return;
       }
+      if (!mPendingData.Length())
+        break;
       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));
+        mPendingData[0].offset + mPendingData[0].curpos, // offset
+        mPendingData[0].data.Length() - mPendingData[0].curpos, // length
+        const_cast<char*>(mPendingData[0].data.BeginReading() + mPendingData[0].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();
+        NPN_DestroyStream(NPRES_NETWORK_ERR);
         return;
       }
-      cur.curpos += r;
+      if (!mPendingData.Length())
+        break;
+
+      mPendingData[0].curpos += r;
     }
+    if (!mPendingData.Length())
+      break;
     mPendingData.RemoveElementAt(0);
   }
 
   ClearSuspendedTimer();
+  MaybeDeliverNPP_DestroyStream();
 }
 
 void
+BrowserStreamChild::MaybeDeliverNPP_DestroyStream()
+{
+  if (kDestroyNotPending != mDestroyPending) {
+    NS_ASSERTION(mPendingData.Length() == 0, "Pending data?");
+
+    if (mState != DYING)
+      NS_RUNTIMEABORT("mDestroyPending but state not DYING");
+
+    mClosed = true;
+    NPReason reason = mDestroyPending;
+    mDestroyPending = kDestroyNotPending;
+
+    (void) mInstance->mPluginIface
+      ->destroystream(&mInstance->mData, &mStream, reason);
+
+    SendStreamDestroyed();
+    mState = DELETING;
+  }
+}
+
+
+void
 BrowserStreamChild::SetSuspendedTimer()
 {
   if (mSuspendedTimer.IsRunning())
     return;
   mSuspendedTimer.Start(
     base::TimeDelta::FromMilliseconds(100),
     this, &BrowserStreamChild::DeliverData);
 }
--- a/dom/plugins/BrowserStreamChild.h
+++ b/dom/plugins/BrowserStreamChild.h
@@ -88,45 +88,66 @@ public:
   }
   void EnsureCorrectStream(NPStream* s)
   {
     if (s != &mStream)
       NS_RUNTIMEABORT("Incorrect stream data");
   }
 
   NPError NPN_RequestRead(NPByteRange* aRangeList);
+  void NPN_DestroyStream(NPReason reason);
 
 private:
+  using PBrowserStreamChild::SendNPN_DestroyStream;
+
   /**
    * Deliver the data currently in mPending, scheduling
    * or cancelling the suspended timer as needed.
    */
   void DeliverData();
+  void MaybeDeliverNPP_DestroyStream();
   void SetSuspendedTimer();
   void ClearSuspendedTimer();
 
   PluginInstanceChild* mInstance;
   NPStream mStream;
   bool mClosed;
   enum {
     CONSTRUCTING,
     ALIVE,
     DYING,
     DELETING
   } mState;
   nsCString mURL;
   nsCString mHeaders;
 
+  static const NPReason kDestroyNotPending = -1;
+
+  /**
+   * When NPP_DestroyStream is delivered with pending data, we postpone
+   * delivering or acknowledging it until the pending data has been written.
+   *
+   * The default/initial value is kDestroyNotPending
+   */
+  NPReason mDestroyPending;
+
   struct PendingData
   {
     int32_t offset;
     Buffer data;
     int32_t curpos;
   };
   nsTArray<PendingData> mPendingData;
 
+  /**
+   * Asynchronous RecvWrite messages are never delivered to the plugin
+   * immediately, because that may be in the midst of an unexpected RPC
+   * stack frame. It instead posts a runnable using this tracker to cancel
+   * in case we are destroyed.
+   */
+  ScopedRunnableMethodFactory<BrowserStreamChild> mDeliverDataTracker;
   base::RepeatingTimer<BrowserStreamChild> mSuspendedTimer;
 };
 
 } // namespace plugins
 } // namespace mozilla
 
 #endif /* mozilla_plugins_BrowserStreamChild_h */
--- a/dom/plugins/PluginModuleChild.cpp
+++ b/dom/plugins/PluginModuleChild.cpp
@@ -893,17 +893,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);
+        bs->NPN_DestroyStream(aReason);
     }
     else {
         PluginStreamChild* ps = static_cast<PluginStreamChild*>(s);
         ps->EnsureCorrectInstance(p);
         PPluginStreamChild::Call__delete__(ps, aReason, false);
     }
     return NPERR_NO_ERROR;
 }