Bug 719180: Part 1 - Correct jar channel stream ownership; r=taras
authorNils Maier <maierman@web.de>
Wed, 28 Nov 2012 13:12:56 -0500
changeset 114377 736e9f98d97a1c9c4824a7d064ce28b050b4eb2f
parent 114376 123b02c5b57911c36fcfc720100f310a53a31ee9
child 114378 0e5ca55005ae8ef9002ac12472740104fb79fd47
push id18733
push usereakhgari@mozilla.com
push dateWed, 28 Nov 2012 18:14:10 +0000
treeherdermozilla-inbound@0e5ca55005ae [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstaras
bugs719180
milestone20.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 719180: Part 1 - Correct jar channel stream ownership; r=taras Avoid potential file locking issues by not keeping references to the input stream in the first place, analog to how nsBaseChannel/nsFileChannel are implemented. The file locks will now be released as soon as either the stream is explictly closed or the stream instance gets destroyed. This means that a synchronous ::Open() will transfer the input stream ownership to the caller, while asynchronous ::AsyncOpen() will transfer it to the pump handling the asynchronous transfer to the caller's listener.
modules/libjar/nsJARChannel.cpp
modules/libjar/nsJARChannel.h
--- a/modules/libjar/nsJARChannel.cpp
+++ b/modules/libjar/nsJARChannel.cpp
@@ -52,67 +52,58 @@ class nsJARInputThunk : public nsIInputS
 {
 public:
     NS_DECL_ISUPPORTS
     NS_DECL_NSIINPUTSTREAM
 
     nsJARInputThunk(nsIZipReader *zipReader,
                     nsIURI* fullJarURI,
                     const nsACString &jarEntry,
-                    nsIZipReaderCache *jarCache)
-        : mJarCache(jarCache)
+                    bool usingJarCache)
+        : mUsingJarCache(usingJarCache)
         , mJarReader(zipReader)
         , mJarEntry(jarEntry)
         , mContentLength(-1)
     {
         if (fullJarURI) {
 #ifdef DEBUG
             nsresult rv =
 #endif
                 fullJarURI->GetAsciiSpec(mJarDirSpec);
             NS_ASSERTION(NS_SUCCEEDED(rv), "this shouldn't fail");
         }
     }
 
     virtual ~nsJARInputThunk()
     {
-        if (!mJarCache && mJarReader)
-            mJarReader->Close();
-    }
-
-    void GetJarReader(nsIZipReader **result)
-    {
-        NS_IF_ADDREF(*result = mJarReader);
+        Close();
     }
 
     int64_t GetContentLength()
     {
         return mContentLength;
     }
 
-    nsresult EnsureJarStream();
+    nsresult Init();
 
 private:
 
-    nsCOMPtr<nsIZipReaderCache> mJarCache;
+    bool                        mUsingJarCache;
     nsCOMPtr<nsIZipReader>      mJarReader;
     nsCString                   mJarDirSpec;
     nsCOMPtr<nsIInputStream>    mJarStream;
     nsCString                   mJarEntry;
     int64_t                     mContentLength;
 };
 
 NS_IMPL_THREADSAFE_ISUPPORTS1(nsJARInputThunk, nsIInputStream)
 
 nsresult
-nsJARInputThunk::EnsureJarStream()
+nsJARInputThunk::Init()
 {
-    if (mJarStream)
-        return NS_OK;
-
     nsresult rv;
     if (ENTRY_IS_DIRECTORY(mJarEntry)) {
         // A directory stream also needs the Spec of the FullJarURI
         // because is included in the stream data itself.
 
         NS_ENSURE_STATE(!mJarDirSpec.IsEmpty());
 
         rv = mJarReader->GetInputStreamWithSpec(mJarDirSpec,
@@ -139,37 +130,38 @@ nsJARInputThunk::EnsureJarStream()
     mContentLength = avail < INT64_MAX ? (int64_t) avail : -1;
 
     return NS_OK;
 }
 
 NS_IMETHODIMP
 nsJARInputThunk::Close()
 {
+    nsresult rv = NS_OK;
+
     if (mJarStream)
-        return mJarStream->Close();
+        rv = mJarStream->Close();
 
-    return NS_OK;
+    if (!mUsingJarCache && mJarReader)
+        mJarReader->Close();
+
+    mJarReader = nullptr;
+
+    return rv;
 }
 
 NS_IMETHODIMP
 nsJARInputThunk::Available(uint64_t *avail)
 {
-    nsresult rv = EnsureJarStream();
-    if (NS_FAILED(rv)) return rv;
-
     return mJarStream->Available(avail);
 }
 
 NS_IMETHODIMP
 nsJARInputThunk::Read(char *buf, uint32_t count, uint32_t *countRead)
 {
-    nsresult rv = EnsureJarStream();
-    if (NS_FAILED(rv)) return rv;
-
     return mJarStream->Read(buf, count, countRead);
 }
 
 NS_IMETHODIMP
 nsJARInputThunk::ReadSegments(nsWriteSegmentFun writer, void *closure,
                               uint32_t count, uint32_t *countRead)
 {
     // stream transport does only calls Read()
@@ -191,32 +183,28 @@ nsJARInputThunk::IsNonBlocking(bool *non
 nsJARChannel::nsJARChannel()
     : mOpened(false)
     , mAppURI(nullptr)
     , mContentLength(-1)
     , mLoadFlags(LOAD_NORMAL)
     , mStatus(NS_OK)
     , mIsPending(false)
     , mIsUnsafe(true)
-    , mJarInput(nullptr)
 {
 #if defined(PR_LOGGING)
     if (!gJarProtocolLog)
         gJarProtocolLog = PR_NewLogModule("nsJarProtocol");
 #endif
 
     // hold an owning reference to the jar handler
     NS_ADDREF(gJarHandler);
 }
 
 nsJARChannel::~nsJARChannel()
 {
-    // with the exception of certain error cases mJarInput will already be null.
-    NS_IF_RELEASE(mJarInput);
-
     // release owning reference to the jar handler
     nsJARProtocolHandler *handler = gJarHandler;
     NS_RELEASE(handler); // NULL parameter
 }
 
 NS_IMPL_ISUPPORTS_INHERITED6(nsJARChannel,
                              nsHashPropertyBag,
                              nsIRequest,
@@ -256,30 +244,32 @@ nsJARChannel::Init(nsIURI *uri)
 
 #if defined(PR_LOGGING)
     mJarURI->GetSpec(mSpec);
 #endif
     return rv;
 }
 
 nsresult
-nsJARChannel::CreateJarInput(nsIZipReaderCache *jarCache)
+nsJARChannel::CreateJarInput(nsIZipReaderCache *jarCache, nsJARInputThunk **resultInput)
 {
+    MOZ_ASSERT(resultInput);
+
     // important to pass a clone of the file since the nsIFile impl is not
     // necessarily MT-safe
     nsCOMPtr<nsIFile> clonedFile;
     nsresult rv = mJarFile->Clone(getter_AddRefs(clonedFile));
     if (NS_FAILED(rv))
         return rv;
 
     nsCOMPtr<nsIZipReader> reader;
     if (jarCache) {
         if (mInnerJarEntry.IsEmpty())
             rv = jarCache->GetZip(mJarFile, getter_AddRefs(reader));
-        else 
+        else
             rv = jarCache->GetInnerZip(mJarFile, mInnerJarEntry,
                                        getter_AddRefs(reader));
     } else {
         // create an uncached jar reader
         nsCOMPtr<nsIZipReader> outerReader = do_CreateInstance(kZipReaderCID, &rv);
         if (NS_FAILED(rv))
             return rv;
 
@@ -295,36 +285,47 @@ nsJARChannel::CreateJarInput(nsIZipReade
                 return rv;
 
             rv = reader->OpenInner(outerReader, mInnerJarEntry);
         }
     }
     if (NS_FAILED(rv))
         return rv;
 
-    mJarInput = new nsJARInputThunk(reader, mJarURI, mJarEntry, jarCache);
-    if (!mJarInput)
-        return NS_ERROR_OUT_OF_MEMORY;
-    NS_ADDREF(mJarInput);
+    nsRefPtr<nsJARInputThunk> input = new nsJARInputThunk(reader,
+                                                          mJarURI,
+                                                          mJarEntry,
+                                                          jarCache != nullptr
+                                                          );
+    rv = input->Init();
+    if (NS_FAILED(rv))
+        return rv;
+
+    // Make GetContentLength meaningful
+    mContentLength = input->GetContentLength();
+
+    input.forget(resultInput);
     return NS_OK;
 }
 
 nsresult
-nsJARChannel::EnsureJarInput(bool blocking)
+nsJARChannel::LookupFile()
 {
-    LOG(("nsJARChannel::EnsureJarInput [this=%x %s]\n", this, mSpec.get()));
+    LOG(("nsJARChannel::LookupFile [this=%x %s]\n", this, mSpec.get()));
 
     nsresult rv;
     nsCOMPtr<nsIURI> uri;
 
     rv = mJarURI->GetJARFile(getter_AddRefs(mJarBaseURI));
-    if (NS_FAILED(rv)) return rv;
+    if (NS_FAILED(rv))
+        return rv;
 
     rv = mJarURI->GetJAREntry(mJarEntry);
-    if (NS_FAILED(rv)) return rv;
+    if (NS_FAILED(rv))
+        return rv;
 
     // The name of the JAR entry must not contain URL-escaped characters:
     // we're moving from URL domain to a filename domain here. nsStandardURL
     // does basic escaping by default, which breaks reading zipped files which
     // have e.g. spaces in their filenames.
     NS_UnescapeURL(mJarEntry);
 
     // try to get a nsIFile directly from the url, which will often succeed.
@@ -344,37 +345,17 @@ nsJARChannel::EnsureJarInput(bool blocki
                 fileURL = do_QueryInterface(innerJarURI);
             if (fileURL) {
                 fileURL->GetFile(getter_AddRefs(mJarFile));
                 jarURI->GetJAREntry(mInnerJarEntry);
             }
         }
     }
 
-    if (mJarFile) {
-        mIsUnsafe = false;
-
-        // NOTE: we do not need to deal with mSecurityInfo here,
-        // because we're loading from a local file
-        rv = CreateJarInput(gJarHandler->JarCache());
-    }
-    else if (blocking) {
-        NS_NOTREACHED("need sync downloader");
-        rv = NS_ERROR_NOT_IMPLEMENTED;
-    }
-    else {
-        // kick off an async download of the base URI...
-        rv = NS_NewDownloader(getter_AddRefs(mDownloader), this);
-        if (NS_SUCCEEDED(rv))
-            rv = NS_OpenURI(mDownloader, nullptr, mJarBaseURI, nullptr,
-                            mLoadGroup, mCallbacks,
-                            mLoadFlags & ~(LOAD_DOCUMENT_URI | LOAD_CALL_CONTENT_SNIFFERS));
-    }
     return rv;
-
 }
 
 //-----------------------------------------------------------------------------
 // nsIRequest
 //-----------------------------------------------------------------------------
 
 NS_IMETHODIMP
 nsJARChannel::GetName(nsACString &result)
@@ -636,20 +617,16 @@ nsJARChannel::GetContentDispositionHeade
 
     aContentDispositionHeader = mContentDispositionHeader;
     return NS_OK;
 }
 
 NS_IMETHODIMP
 nsJARChannel::GetContentLength(int64_t *result)
 {
-    // if content length is unknown, query mJarInput...
-    if (mContentLength < 0 && mJarInput)
-        mContentLength = mJarInput->GetContentLength();
-
     *result = mContentLength;
     return NS_OK;
 }
 
 NS_IMETHODIMP
 nsJARChannel::SetContentLength(int64_t aContentLength)
 {
     // XXX does this really make any sense at all?
@@ -657,77 +634,102 @@ nsJARChannel::SetContentLength(int64_t a
     return NS_OK;
 }
 
 NS_IMETHODIMP
 nsJARChannel::Open(nsIInputStream **stream)
 {
     LOG(("nsJARChannel::Open [this=%x]\n", this));
 
-    NS_ENSURE_TRUE(!mJarInput, NS_ERROR_IN_PROGRESS);
+    NS_ENSURE_TRUE(!mOpened, NS_ERROR_IN_PROGRESS);
     NS_ENSURE_TRUE(!mIsPending, NS_ERROR_IN_PROGRESS);
 
     mJarFile = nullptr;
     mIsUnsafe = true;
 
-    nsresult rv = EnsureJarInput(true);
-    if (NS_FAILED(rv)) return rv;
+    nsresult rv = LookupFile();
+    if (NS_FAILED(rv))
+        return rv;
 
-    if (!mJarInput)
-        return NS_ERROR_UNEXPECTED;
+    // If mJarInput was not set by LookupFile, the JAR is a remote jar.
+    if (!mJarFile) {
+        NS_NOTREACHED("need sync downloader");
+        return NS_ERROR_NOT_IMPLEMENTED;
+    }
 
-    // force load the jar file now so GetContentLength will return a
-    // meaningful value once we return.
-    rv = mJarInput->EnsureJarStream();
-    if (NS_FAILED(rv)) return rv;
+    nsCOMPtr<nsJARInputThunk> input;
+    rv = CreateJarInput(gJarHandler->JarCache(), getter_AddRefs(input));
+    if (NS_FAILED(rv))
+        return rv;
 
-    NS_ADDREF(*stream = mJarInput);
-
+    input.forget(stream);
     mOpened = true;
+    // local files are always considered safe
+    mIsUnsafe = false;
     return NS_OK;
 }
 
 NS_IMETHODIMP
 nsJARChannel::AsyncOpen(nsIStreamListener *listener, nsISupports *ctx)
 {
     LOG(("nsJARChannel::AsyncOpen [this=%x]\n", this));
 
     NS_ENSURE_ARG_POINTER(listener);
+    NS_ENSURE_TRUE(!mOpened, NS_ERROR_IN_PROGRESS);
     NS_ENSURE_TRUE(!mIsPending, NS_ERROR_IN_PROGRESS);
 
     mJarFile = nullptr;
     mIsUnsafe = true;
 
     // Initialize mProgressSink
     NS_QueryNotificationCallbacks(mCallbacks, mLoadGroup, mProgressSink);
 
-    nsresult rv = EnsureJarInput(false);
-    if (NS_FAILED(rv)) return rv;
+    nsresult rv = LookupFile();
+    if (NS_FAILED(rv))
+        return rv;
 
     // These variables must only be set if we're going to trigger an
     // OnStartRequest, either from AsyncRead or OnDownloadComplete.
+    // 
+    // That means: Do not add early return statements beyond this point!
     mListener = listener;
     mListenerContext = ctx;
     mIsPending = true;
-    if (mJarInput) {
-        // create input stream pump and call AsyncRead as a block
-        rv = NS_NewInputStreamPump(getter_AddRefs(mPump), mJarInput);
-        if (NS_SUCCEEDED(rv))
-            rv = mPump->AsyncRead(this, nullptr);
 
-        // If we failed to create the pump or initiate the AsyncRead,
-        // then we need to clear these variables.
-        if (NS_FAILED(rv)) {
-            mIsPending = false;
-            mListenerContext = nullptr;
-            mListener = nullptr;
-            return rv;
+    if (!mJarFile) {
+        // Not a local file...
+        // kick off an async download of the base URI...
+        rv = NS_NewDownloader(getter_AddRefs(mDownloader), this);
+        if (NS_SUCCEEDED(rv))
+            rv = NS_OpenURI(mDownloader, nullptr, mJarBaseURI, nullptr,
+                            mLoadGroup, mCallbacks,
+                            mLoadFlags & ~(LOAD_DOCUMENT_URI | LOAD_CALL_CONTENT_SNIFFERS));
+    }
+    else {
+        // local files are always considered safe
+        mIsUnsafe = false;
+
+        nsCOMPtr<nsJARInputThunk> input;
+        rv = CreateJarInput(gJarHandler->JarCache(), getter_AddRefs(input));
+        if (NS_SUCCEEDED(rv)) {
+            // create input stream pump and call AsyncRead as a block
+            rv = NS_NewInputStreamPump(getter_AddRefs(mPump), input);
+            if (NS_SUCCEEDED(rv))
+                rv = mPump->AsyncRead(this, nullptr);
         }
     }
 
+    if (NS_FAILED(rv)) {
+        mIsPending = false;
+        mListenerContext = nullptr;
+        mListener = nullptr;
+        return rv;
+    }
+
+
     if (mLoadGroup)
         mLoadGroup->AddRequest(this, nullptr);
 
     mOpened = true;
     return NS_OK;
 }
 
 //-----------------------------------------------------------------------------
@@ -837,21 +839,22 @@ nsJARChannel::OnDownloadComplete(nsIDown
         nsCOMPtr<nsIViewSourceChannel> viewSource = do_QueryInterface(channel);
         if (viewSource) {
             status = NS_ERROR_UNSAFE_CONTENT_TYPE;
         }
     }
 
     if (NS_SUCCEEDED(status)) {
         mJarFile = file;
-    
-        rv = CreateJarInput(nullptr);
+
+        nsCOMPtr<nsJARInputThunk> input;
+        rv = CreateJarInput(nullptr, getter_AddRefs(input));
         if (NS_SUCCEEDED(rv)) {
             // create input stream pump
-            rv = NS_NewInputStreamPump(getter_AddRefs(mPump), mJarInput);
+            rv = NS_NewInputStreamPump(getter_AddRefs(mPump), input);
             if (NS_SUCCEEDED(rv))
                 rv = mPump->AsyncRead(this, nullptr);
         }
         status = rv;
     }
 
     if (NS_FAILED(status)) {
         mStatus = status;
@@ -888,17 +891,16 @@ nsJARChannel::OnStopRequest(nsIRequest *
         mListener = 0;
         mListenerContext = 0;
     }
 
     if (mLoadGroup)
         mLoadGroup->RemoveRequest(this, nullptr, status);
 
     mPump = 0;
-    NS_IF_RELEASE(mJarInput);
     mIsPending = false;
     mDownloader = 0; // this may delete the underlying jar file
 
     // Drop notification callbacks to prevent cycles.
     mCallbacks = 0;
     mProgressSink = 0;
 
     return NS_OK;
--- a/modules/libjar/nsJARChannel.h
+++ b/modules/libjar/nsJARChannel.h
@@ -41,18 +41,18 @@ public:
     NS_DECL_NSISTREAMLISTENER
 
     nsJARChannel();
     virtual ~nsJARChannel();
 
     nsresult Init(nsIURI *uri);
 
 private:
-    nsresult CreateJarInput(nsIZipReaderCache *);
-    nsresult EnsureJarInput(bool blocking);
+    nsresult CreateJarInput(nsIZipReaderCache *, nsJARInputThunk **);
+    nsresult LookupFile();
 
 #if defined(PR_LOGGING)
     nsCString                       mSpec;
 #endif
 
     bool                            mOpened;
 
     nsCOMPtr<nsIJARURI>             mJarURI;
@@ -72,17 +72,16 @@ private:
      * empty */
     uint32_t                        mContentDisposition;
     int64_t                         mContentLength;
     uint32_t                        mLoadFlags;
     nsresult                        mStatus;
     bool                            mIsPending;
     bool                            mIsUnsafe;
 
-    nsJARInputThunk                *mJarInput;
     nsCOMPtr<nsIStreamListener>     mDownloader;
     nsCOMPtr<nsIInputStreamPump>    mPump;
     nsCOMPtr<nsIFile>               mJarFile;
     nsCOMPtr<nsIURI>                mJarBaseURI;
     nsCString                       mJarEntry;
     nsCString                       mInnerJarEntry;
 };