Followup to bug 548217 - Instead of using a bizarre dual-refcounting scheme, explicitly track when nsPluginStreamListenerPeer.mLocalCachedFile is shared and should be deleted, r=josh
authorBenjamin Smedberg <benjamin@smedbergs.us>
Sat, 06 Mar 2010 14:38:54 -0500
changeset 39119 e050d3eaf1dd2f6e56750ce0b6f589d0449e4a64
parent 39118 9c8823185edbccaa675f0b76b770be05bd8721f8
child 39120 a1eaad798c2a4d5629c75a3c4c5563e6c532b7cd
push id12022
push userbsmedberg@mozilla.com
push dateMon, 08 Mar 2010 18:22:57 +0000
treeherdermozilla-central@e050d3eaf1dd [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjosh
bugs548217
milestone1.9.3a3pre
Followup to bug 548217 - Instead of using a bizarre dual-refcounting scheme, explicitly track when nsPluginStreamListenerPeer.mLocalCachedFile is shared and should be deleted, r=josh
modules/plugin/base/src/nsPluginHost.cpp
modules/plugin/base/src/nsPluginTags.h
modules/plugin/test/mochitest/Makefile.in
modules/plugin/test/mochitest/loremipsum_nocache.txt
modules/plugin/test/mochitest/loremipsum_nocache.txt^headers^
modules/plugin/test/mochitest/test_twostreams.html
--- a/modules/plugin/base/src/nsPluginHost.cpp
+++ b/modules/plugin/base/src/nsPluginHost.cpp
@@ -340,16 +340,38 @@ nsresult nsPluginHost::PostPluginUnloadE
     return NS_OK;
 
   // failure case
   NS_TRY_SAFE_CALL_VOID(PR_UnloadLibrary(aLibrary), nsnull, nsnull);
 
   return NS_ERROR_FAILURE;
 }
 
+/**
+ * When a plugin requests opens multiple requests to the same URL and
+ * the request must be satified by saving a file to disk, each stream
+ * listener holds a reference to the backing file: the file is only removed
+ * when all the listeners are done.
+ */
+class CachedFileHolder
+{
+public:
+  CachedFileHolder(nsIFile* cacheFile);
+  ~CachedFileHolder();
+
+  void AddRef();
+  void Release();
+
+  nsIFile* file() const { return mFile; }
+
+private:
+  nsAutoRefCnt mRefCnt;
+  nsCOMPtr<nsIFile> mFile;
+};
+
 class nsPluginStreamListenerPeer : public nsIStreamListener,
                                    public nsIProgressEventSink,
                                    public nsIHttpHeaderVisitor,
                                    public nsSupportsWeakReference,
                                    public nsINPAPIPluginStreamInfo
 {
 public:
   nsPluginStreamListenerPeer();
@@ -410,17 +432,17 @@ private:
   PRPackedBool      mStartBinding;
   PRPackedBool      mHaveFiredOnStartRequest;
   // these get passed to the plugin stream listener
   PRUint32                mLength;
   PRInt32                 mStreamType;
 
   // local cached file, we save the content into local cache if browser cache is not available,
   // or plugin asks stream as file and it expects file extension until bug 90558 got fixed
-  nsCOMPtr<nsIFile> mLocalCachedFile;
+  nsRefPtr<CachedFileHolder> mLocalCachedFileHolder;
   nsCOMPtr<nsIOutputStream> mFileCacheOutputStream;
   nsHashtable             *mDataForwardToRequest;
 
   nsCString mContentType;
   PRBool mSeekable;
   PRUint32 mModified;
   nsCOMPtr<nsIPluginInstance> mPluginInstance;
   PRInt32 mStreamOffset;
@@ -450,16 +472,43 @@ private:
 
 NS_IMETHODIMP
 nsPluginStreamListenerPeer::GetContentType(char** result)
 {
   *result = const_cast<char*>(mContentType.get());
   return NS_OK;
 }
 
+CachedFileHolder::CachedFileHolder(nsIFile* cacheFile)
+  : mFile(cacheFile)
+{
+  NS_ASSERTION(mFile, "Empty CachedFileHolder");
+}
+
+CachedFileHolder::~CachedFileHolder()
+{
+  mFile->Remove(PR_FALSE);
+}
+
+void
+CachedFileHolder::AddRef()
+{
+  ++mRefCnt;
+  NS_LOG_ADDREF(this, mRefCnt, "CachedFileHolder", sizeof(*this));
+}
+
+void
+CachedFileHolder::Release()
+{
+  --mRefCnt;
+  NS_LOG_RELEASE(this, mRefCnt, "CachedFileHolder");
+  if (0 == mRefCnt)
+    delete this;
+}
+
 NS_IMETHODIMP
 nsPluginStreamListenerPeer::IsSeekable(PRBool* result)
 {
   *result = mSeekable;
   return NS_OK;
 }
 
 NS_IMETHODIMP
@@ -673,43 +722,24 @@ nsPluginStreamListenerPeer::nsPluginStre
   mStreamOffset = 0;
   mStreamComplete = 0;
 }
 
 nsPluginStreamListenerPeer::~nsPluginStreamListenerPeer()
 {
 #ifdef PLUGIN_LOGGING
   PR_LOG(nsPluginLogging::gPluginLog, PLUGIN_LOG_NORMAL,
-    ("nsPluginStreamListenerPeer::dtor this=%p, url=%s%c",this, mURLSpec.get(), mLocalCachedFile?',':'\n'));
+    ("nsPluginStreamListenerPeer::dtor this=%p, url=%s\n",this, mURLSpec.get()));
 #endif
 
   // close FD of mFileCacheOutputStream if it's still open
   // or we won't be able to remove the cache file
   if (mFileCacheOutputStream)
     mFileCacheOutputStream = nsnull;
 
-  // if we have mLocalCachedFile lets release it
-  // and it'll be fiscally remove if refcnt == 1
-  if (mLocalCachedFile) {
-    nsrefcnt refcnt;
-    NS_RELEASE2(mLocalCachedFile, refcnt);
-
-#ifdef PLUGIN_LOGGING
-    nsCAutoString filePath;
-    mLocalCachedFile->GetNativePath(filePath);
-
-    PR_LOG(nsPluginLogging::gPluginLog, PLUGIN_LOG_NORMAL,
-      ("LocalyCachedFile=%s has %d refcnt and will %s be deleted now\n",filePath.get(),refcnt,refcnt==1?"":"NOT"));
-#endif
-
-    if (refcnt == 1) {
-      mLocalCachedFile->Remove(PR_FALSE);
-    }
-  }
-
   delete mDataForwardToRequest;
 }
 
 NS_IMPL_ISUPPORTS6(nsPluginStreamListenerPeer,
                    nsIStreamListener,
                    nsIRequestObserver,
                    nsIHttpHeaderVisitor,
                    nsISupportsWeakReference,
@@ -812,30 +842,26 @@ nsPluginStreamListenerPeer::SetupPluginC
   nsresult rv = NS_OK;
 
   PRBool useExistingCacheFile = PR_FALSE;
 
   nsRefPtr<nsPluginHost> pluginHost = dont_AddRef(nsPluginHost::GetInst());
   nsTArray< nsAutoPtr<nsPluginInstanceTag> > *instanceTags = pluginHost->InstanceTagArray();
   for (PRUint32 i = 0; i < instanceTags->Length(); i++) {
     nsPluginInstanceTag *instanceTag = (*instanceTags)[i];
-    if (instanceTag->mStreams) {
-      // most recent streams are at the end of list
-      PRInt32 cnt;
-      instanceTag->mStreams->Count((PRUint32*)&cnt);
-      while (--cnt >= 0) {
-        nsPluginStreamListenerPeer *lp =
-          reinterpret_cast<nsPluginStreamListenerPeer*>(instanceTag->mStreams->ElementAt(cnt));
-        if (lp && lp->mLocalCachedFile) {
-          useExistingCacheFile = lp->UseExistingPluginCacheFile(this);
-          if (useExistingCacheFile) {
-            mLocalCachedFile = lp->mLocalCachedFile;
-            break;
-          }
-          NS_RELEASE(lp);
+
+    // most recent streams are at the end of list
+    for (PRInt32 i = instanceTag->mStreams.Count() - 1; i >= 0; --i) {
+      nsPluginStreamListenerPeer *lp =
+        static_cast<nsPluginStreamListenerPeer*>(instanceTag->mStreams[i]);
+      if (lp && lp->mLocalCachedFileHolder) {
+        useExistingCacheFile = lp->UseExistingPluginCacheFile(this);
+        if (useExistingCacheFile) {
+          mLocalCachedFileHolder = lp->mLocalCachedFileHolder;
+          break;
         }
       }
       if (useExistingCacheFile)
         break;
     }
   }
 
   if (!useExistingCacheFile) {
@@ -873,35 +899,25 @@ nsPluginStreamListenerPeer::SetupPluginC
     // create a file output stream to write to...
     nsCOMPtr<nsIOutputStream> outstream;
     rv = NS_NewLocalFileOutputStream(getter_AddRefs(mFileCacheOutputStream), pluginTmp, -1, 00600);
     if (NS_FAILED(rv))
       return rv;
 
     // save the file.
     
-    mLocalCachedFile = pluginTmp;
-    // Addref to add one extra refcnt, we can use NS_RELEASE2(mLocalCachedFile...) in dtor
-    // to remove this file when refcnt == 1
-    NS_ADDREF(mLocalCachedFile);
+    mLocalCachedFileHolder = new CachedFileHolder(pluginTmp);
   }
 
   // add this listenerPeer to list of stream peers for this instance
   // it'll delay release of listenerPeer until nsPluginInstanceTag::~nsPluginInstanceTag
   // and the temp file is going to stay alive until then
   nsPluginInstanceTag *instanceTag = pluginHost->FindInstanceTag(mInstance);
-  if (instanceTag) {
-    if (!instanceTag->mStreams &&
-        (NS_FAILED(rv = NS_NewISupportsArray(getter_AddRefs(instanceTag->mStreams))))) {
-      return rv;
-    }
-
-    nsISupports* supports = static_cast<nsISupports*>((static_cast<nsIStreamListener*>(this)));
-    instanceTag->mStreams->AppendElement(supports);
-  }
+  if (instanceTag)
+    instanceTag->mStreams.AppendObject(this);
 
   return rv;
 }
 
 NS_IMETHODIMP
 nsPluginStreamListenerPeer::OnStartRequest(nsIRequest *request,
                                            nsISupports* aContext)
 {
@@ -1269,18 +1285,20 @@ NS_IMETHODIMP nsPluginStreamListenerPeer
     // on error status cleanup the stream
     // and return w/o OnFileAvailable()
     mPStreamListener->OnStopBinding(this, aStatus);
     return NS_OK;
   }
 
   // call OnFileAvailable if plugin requests stream type StreamType_AsFile or StreamType_AsFileOnly
   if (mStreamType >= NP_ASFILE) {
-    nsCOMPtr<nsIFile> localFile = mLocalCachedFile;
-    if (!localFile) {
+    nsCOMPtr<nsIFile> localFile;
+    if (mLocalCachedFileHolder)
+      localFile = mLocalCachedFileHolder->file();
+    else {
       nsCOMPtr<nsICachingChannel> cacheChannel = do_QueryInterface(request);
       if (cacheChannel) {
         cacheChannel->GetCacheFile(getter_AddRefs(localFile));
       } else {
         // see if it is a file channel.
         nsCOMPtr<nsIFileChannel> fileChannel = do_QueryInterface(request);
         if (fileChannel) {
           fileChannel->GetFile(getter_AddRefs(localFile));
@@ -4490,21 +4508,18 @@ nsresult nsPluginHost::NewFullPagePlugin
 
   rv = listener->InitializeFullPage(aInstance);
 
   aStreamListener = listener;
   NS_ADDREF(listener);
 
   // add peer to list of stream peers for this instance
   nsPluginInstanceTag * p = FindInstanceTag(aInstance);
-  if (p) {
-    if (!p->mStreams && (NS_FAILED(rv = NS_NewISupportsArray(getter_AddRefs(p->mStreams)))))
-      return rv;
-    p->mStreams->AppendElement(aStreamListener);
-  }
+  if (p)
+    p->mStreams.AppendObject(listener);
 
   return rv;
 }
 
 NS_IMETHODIMP nsPluginHost::Observe(nsISupports *aSubject,
                                     const char *aTopic,
                                     const PRUnichar *someData)
 {
--- a/modules/plugin/base/src/nsPluginTags.h
+++ b/modules/plugin/base/src/nsPluginTags.h
@@ -38,16 +38,17 @@
 
 #ifndef nsPluginTags_h_
 #define nsPluginTags_h_
 
 #include "nscore.h"
 #include "prtypes.h"
 #include "nsAutoPtr.h"
 #include "nsCOMPtr.h"
+#include "nsCOMArray.h"
 #include "nsIPluginTag.h"
 #include "nsIPlugin.h"
 #include "nsNPAPIPluginInstance.h"
 #include "nsISupportsArray.h"
 
 class nsPluginHost;
 struct PRLibrary;
 struct nsPluginInfo;
@@ -125,17 +126,17 @@ private:
 
 struct nsPluginInstanceTag
 {
   char*                  mURL;
   nsRefPtr<nsPluginTag>  mPluginTag;
   nsNPAPIPluginInstance* mInstance; // this must always be valid
   PRBool                 mDefaultPlugin;
   // Array holding all opened stream listeners for this entry
-  nsCOMPtr <nsISupportsArray> mStreams; 
+  nsCOMArray<nsIPluginStreamInfo> mStreams; 
   
   nsPluginInstanceTag(nsPluginTag* aPluginTag,
                       nsIPluginInstance* aInstance, 
                       const char * url,
                       PRBool aDefaultPlugin);
   ~nsPluginInstanceTag();
 };
 
--- a/modules/plugin/test/mochitest/Makefile.in
+++ b/modules/plugin/test/mochitest/Makefile.in
@@ -47,16 +47,18 @@ include $(topsrcdir)/config/rules.mk
 _MOCHITEST_FILES = \
   test_getauthenticationinfo.html \
   test_npobject_getters.html \
   test_npruntime_npnevaluate.html \
   test_npruntime_npninvoke.html \
   test_npruntime_npninvokedefault.html \
   loremipsum.txt \
   loremipsum_file.txt \
+  loremipsum_nocache.txt \
+  loremipsum_nocache.txt^headers^ \
   post.sjs \
   pluginstream.js \
   plugin_window.html \
   test_painting.html \
   test_pluginstream_err.html \
   test_pluginstream_src.html \
   test_pluginstream_geturl.html \
   test_pluginstream_geturlnotify.html \
@@ -69,16 +71,17 @@ include $(topsrcdir)/config/rules.mk
   test_multipleinstanceobjects.html \
   test_streamNotify.html \
   test_instantiation.html \
   test_cookies.html \
   test_npn_timers.html \
   test_npn_asynccall.html \
   test_bug532208.html \
   large-pic.jpg \
+  test_twostreams.html \
   $(NULL)
 
 #  test_npruntime_npnsetexception.html \ Disabled for e10s
 
 ifdef MOZ_IPC
 _MOCHITEST_FILES += \
   test_crashing.html \
   test_crashing2.html \
new file mode 100644
--- /dev/null
+++ b/modules/plugin/test/mochitest/loremipsum_nocache.txt
@@ -0,0 +1,11 @@
+Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet.
+
+Duis autem vel eum iriure dolor in hendrerit in vulputate velit esse molestie consequat, vel illum dolore eu feugiat nulla facilisis at vero eros et accumsan et iusto odio dignissim qui blandit praesent luptatum zzril delenit augue duis dolore te feugait nulla facilisi. Lorem ipsum dolor sit amet, consectetuer adipiscing elit, sed diam nonummy nibh euismod tincidunt ut laoreet dolore magna aliquam erat volutpat.
+
+Ut wisi enim ad minim veniam, quis nostrud exerci tation ullamcorper suscipit lobortis nisl ut aliquip ex ea commodo consequat. Duis autem vel eum iriure dolor in hendrerit in vulputate velit esse molestie consequat, vel illum dolore eu feugiat nulla facilisis at vero eros et accumsan et iusto odio dignissim qui blandit praesent luptatum zzril delenit augue duis dolore te feugait nulla facilisi.
+
+Nam liber tempor cum soluta nobis eleifend option congue nihil imperdiet doming id quod mazim placerat facer possim assum. Lorem ipsum dolor sit amet, consectetuer adipiscing elit, sed diam nonummy nibh euismod tincidunt ut laoreet dolore magna aliquam erat volutpat. Ut wisi enim ad minim veniam, quis nostrud exerci tation ullamcorper suscipit lobortis nisl ut aliquip ex ea commodo consequat.
+
+Duis autem vel eum iriure dolor in hendrerit in vulputate velit esse molestie consequat, vel illum dolore eu feugiat nulla facilisis. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, At accusam aliquyam diam diam dolore dolores duo eirmod eos erat, et nonumy sed tempor et et invidunt justo labore Stet clita ea et gubergren, kasd magna no rebum. sanctus sea sed takimata ut vero voluptua. est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut laboreet dolore magna aliquyam erat.
+
+Consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. 
\ No newline at end of file
new file mode 100644
--- /dev/null
+++ b/modules/plugin/test/mochitest/loremipsum_nocache.txt^headers^
@@ -0,0 +1,1 @@
+Cache-Control: no-store
new file mode 100644
--- /dev/null
+++ b/modules/plugin/test/mochitest/test_twostreams.html
@@ -0,0 +1,46 @@
+<html>
+<head>
+  <title>Dual NPAPI NP_ASFILEONLY NPStream Test</title>
+  <script type="text/javascript" 
+          src="/MochiKit/packed.js"></script>
+  <script type="text/javascript" 
+          src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" 
+        href="/tests/SimpleTest/test.css" />
+</head>
+<body>
+  <p id="display"></p>
+
+  <iframe id="testframe1" name="testframe1" onload="frameLoaded(1)"></iframe>
+  <iframe id="testframe2" name="testframe2" onload="frameLoaded(2)"></iframe>
+
+  <embed src="loremipsum_nocache.txt" streammode="asfileonly"
+         frame="testframe1"
+         id="embedtest" style="width: 400px; height: 100px;"
+         type="application/x-test"></embed>
+  <embed src="loremipsum_nocache.txt" streammode="asfileonly"
+	 frame="testframe2"
+	 id="embedtest2" style="width: 400px; height: 100px;"
+	 type="application/x-test"></embed>
+
+  <script type="text/javascript">
+  SimpleTest.waitForExplicitFinish();
+
+  var framesToLoad = 2;
+  function frameLoaded(id) {
+    var frame = document.getElementById('testframe' + id);
+    if (!frame.contentDocument.body.innerHTML.length)
+      return;
+
+    --framesToLoad;
+    if (0 == framesToLoad) {
+      is(document.getElementById('testframe1').contentDocument.body.innerHTML,
+         document.getElementById('testframe2').contentDocument.body.innerHTML,
+         "Frame contents should match");
+      SimpleTest.finish();
+    }
+  }
+  </script>
+
+ </body>
+ </html>