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

@@ -57,6 +57,8 @@ BrowserStreamChild::BrowserStreamChild(PluginInstanceChild* instance,
   , 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,
@@ -127,7 +129,9 @@ BrowserStreamChild::RecvWrite(const int32_t& offset,
   newdata->data = data;
   newdata->curpos = 0;
 
-  DeliverData();
+  if (mDeliverDataTracker.empty())
+    MessageLoop::current()->PostTask(FROM_HERE,
+      mDeliverDataTracker.NewRunnableMethod(&BrowserStreamChild::DeliverData));
 
   return true;
 }
@@ -159,12 +163,11 @@ BrowserStreamChild::RecvNPP_DestroyStream(const NPReason& reason)
     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;
 }
@@ -201,47 +204,88 @@ BrowserStreamChild::NPN_RequestRead(NPByteRange* aRangeList)
   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()
 {

dom/plugins/BrowserStreamChild.h

@@ -93,13 +93,17 @@ public:
   }
 
   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();
 
@@ -115,6 +119,16 @@ private:
   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;
@@ -123,6 +137,13 @@ private:
   };
   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;
 };
 

dom/plugins/PluginModuleChild.cpp

@@ -898,7 +898,7 @@ _destroystream(NPP aNPP,
     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);