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

@@ -57,8 +57,8 @@ public:
     nsJARInputThunk(nsIZipReader *zipReader,
                     nsIURI* fullJarURI,
                     const nsACString &jarEntry,
-                    nsIZipReaderCache *jarCache)
+                    bool usingJarCache)
-        : mJarCache(jarCache)
+        : mUsingJarCache(usingJarCache)
         , mJarReader(zipReader)
         , mJarEntry(jarEntry)
         , mContentLength(-1)
@@ -74,13 +74,7 @@ public:
 
     virtual ~nsJARInputThunk()
     {
-        if (!mJarCache && mJarReader)
+        Close();
-            mJarReader->Close();
-    }
-
-    void GetJarReader(nsIZipReader **result)
-    {
-        NS_IF_ADDREF(*result = mJarReader);
     }
 
     int64_t GetContentLength()
@@ -88,11 +82,11 @@ public:
         return mContentLength;
     }
 
-    nsresult EnsureJarStream();
+    nsresult Init();
 
 private:
 
-    nsCOMPtr<nsIZipReaderCache> mJarCache;
+    bool                        mUsingJarCache;
     nsCOMPtr<nsIZipReader>      mJarReader;
     nsCString                   mJarDirSpec;
     nsCOMPtr<nsIInputStream>    mJarStream;
@@ -103,11 +97,8 @@ private:
 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
@@ -144,27 +135,28 @@ nsJARInputThunk::EnsureJarStream()
 NS_IMETHODIMP
 nsJARInputThunk::Close()
 {
-    if (mJarStream)
+    nsresult rv = NS_OK;
-        return mJarStream->Close();
 
-    return NS_OK;
+    if (mJarStream)
+        rv = mJarStream->Close();
+
+    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);
 }
 
@@ -196,7 +188,6 @@ nsJARChannel::nsJARChannel()
     , mStatus(NS_OK)
     , mIsPending(false)
     , mIsUnsafe(true)
-    , mJarInput(nullptr)
 {
 #if defined(PR_LOGGING)
     if (!gJarProtocolLog)
@@ -209,9 +200,6 @@ nsJARChannel::nsJARChannel()
 
 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
@@ -261,8 +249,10 @@ nsJARChannel::Init(nsIURI *uri)
 }
 
 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;
@@ -274,7 +264,7 @@ nsJARChannel::CreateJarInput(nsIZipReaderCache *jarCache)
     if (jarCache) {
         if (mInnerJarEntry.IsEmpty())
             rv = jarCache->GetZip(mJarFile, getter_AddRefs(reader));
-        else 
+        else
             rv = jarCache->GetInnerZip(mJarFile, mInnerJarEntry,
                                        getter_AddRefs(reader));
     } else {
@@ -300,26 +290,37 @@ nsJARChannel::CreateJarInput(nsIZipReaderCache *jarCache)
     if (NS_FAILED(rv))
         return rv;
 
-    mJarInput = new nsJARInputThunk(reader, mJarURI, mJarEntry, jarCache);
+    nsRefPtr<nsJARInputThunk> input = new nsJARInputThunk(reader,
-    if (!mJarInput)
+                                                          mJarURI,
-        return NS_ERROR_OUT_OF_MEMORY;
+                                                          mJarEntry,
-    NS_ADDREF(mJarInput);
+                                                          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
@@ -349,27 +350,7 @@ nsJARChannel::EnsureJarInput(bool blocking)
         }
     }
 
-    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;
-
 }
 
 //-----------------------------------------------------------------------------
@@ -641,10 +622,6 @@ nsJARChannel::GetContentDispositionHeader(nsACString &aContentDispositionHeader)
 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;
 }
@@ -662,26 +639,31 @@ 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);
+    nsresult rv = LookupFile();
-    if (NS_FAILED(rv)) return rv;
+    if (NS_FAILED(rv))
+        return rv;
 
-    if (!mJarInput)
+    // If mJarInput was not set by LookupFile, the JAR is a remote jar.
-        return NS_ERROR_UNEXPECTED;
+    if (!mJarFile) {
+        NS_NOTREACHED("need sync downloader");
+        return NS_ERROR_NOT_IMPLEMENTED;
+    }
 
-    // force load the jar file now so GetContentLength will return a
+    nsCOMPtr<nsJARInputThunk> input;
-    // meaningful value once we return.
+    rv = CreateJarInput(gJarHandler->JarCache(), getter_AddRefs(input));
-    rv = mJarInput->EnsureJarStream();
+    if (NS_FAILED(rv))
-    if (NS_FAILED(rv)) return rv;
+        return rv;
-
-    NS_ADDREF(*stream = mJarInput);
 
+    input.forget(stream);
     mOpened = true;
+    // local files are always considered safe
+    mIsUnsafe = false;
     return NS_OK;
 }
 
@@ -691,6 +673,7 @@ 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;
@@ -699,30 +682,49 @@ nsJARChannel::AsyncOpen(nsIStreamListener *listener, nsISupports *ctx)
     // Initialize mProgressSink
     NS_QueryNotificationCallbacks(mCallbacks, mLoadGroup, mProgressSink);
 
-    nsresult rv = EnsureJarInput(false);
+    nsresult rv = LookupFile();
-    if (NS_FAILED(rv)) return rv;
+    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,
+    if (!mJarFile) {
-        // then we need to clear these variables.
+        // Not a local file...
-        if (NS_FAILED(rv)) {
+        // kick off an async download of the base URI...
-            mIsPending = false;
+        rv = NS_NewDownloader(getter_AddRefs(mDownloader), this);
-            mListenerContext = nullptr;
+        if (NS_SUCCEEDED(rv))
-            mListener = nullptr;
+            rv = NS_OpenURI(mDownloader, nullptr, mJarBaseURI, nullptr,
-            return rv;
+                            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);
 
@@ -842,11 +844,12 @@ nsJARChannel::OnDownloadComplete(nsIDownloader *downloader,
 
     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);
         }
@@ -893,7 +896,6 @@ nsJARChannel::OnStopRequest(nsIRequest *req, nsISupports *ctx, nsresult status)
         mLoadGroup->RemoveRequest(this, nullptr, status);
 
     mPump = 0;
-    NS_IF_RELEASE(mJarInput);
     mIsPending = false;
     mDownloader = 0; // this may delete the underlying jar file
 

modules/libjar/nsJARChannel.h

@@ -46,8 +46,8 @@ public:
     nsresult Init(nsIURI *uri);
 
 private:
-    nsresult CreateJarInput(nsIZipReaderCache *);
+    nsresult CreateJarInput(nsIZipReaderCache *, nsJARInputThunk **);
-    nsresult EnsureJarInput(bool blocking);
+    nsresult LookupFile();
 
 #if defined(PR_LOGGING)
     nsCString                       mSpec;
@@ -77,7 +77,6 @@ private:
     bool                            mIsPending;
     bool                            mIsUnsafe;
 
-    nsJARInputThunk                *mJarInput;
     nsCOMPtr<nsIStreamListener>     mDownloader;
     nsCOMPtr<nsIInputStreamPump>    mPump;
     nsCOMPtr<nsIFile>               mJarFile;