Bug 596443, SVG embedded by reference is not loading. r=bzbarsky@mit.edu, a=bzbarsky@mit.edu
authorBjarne Herland <bjarne@runitsoft.com>
Mon, 25 Oct 2010 15:47:40 -0700
changeset 56468 47bdba5b3d87573a9d955953eb720a07336562c8
parent 56467 290ad164b6562b44be4028905031d3e2a6b22b48
child 56470 b3e46c883256a95b716a3558f0d08abd5909aaa8
push idunknown
push userunknown
push dateunknown
reviewersbzbarsky, bzbarsky
bugs596443
milestone2.0b8pre
Bug 596443, SVG embedded by reference is not loading. r=bzbarsky@mit.edu, a=bzbarsky@mit.edu
netwerk/cache/nsCacheEntry.cpp
netwerk/cache/nsCacheService.cpp
netwerk/cache/nsCacheService.h
--- a/netwerk/cache/nsCacheEntry.cpp
+++ b/netwerk/cache/nsCacheEntry.cpp
@@ -162,30 +162,30 @@ nsCacheEntry::TouchMetaData()
  *      n descriptors (existing, bound entry) valid
  *      n descriptors (existing, bound entry) not valid (wait until valid or doomed)
  */
 
 nsresult
 nsCacheEntry::RequestAccess(nsCacheRequest * request, nsCacheAccessMode *accessGranted)
 {
     nsresult  rv = NS_OK;
-    
+
+    if (IsDoomed()) return NS_ERROR_CACHE_ENTRY_DOOMED;
+
     if (!IsInitialized()) {
         // brand new, unbound entry
         request->mKey = nsnull;  // steal ownership of the key string
         if (request->IsStreamBased())  MarkStreamBased();
         MarkInitialized();
 
         *accessGranted = request->AccessRequested() & nsICache::ACCESS_WRITE;
         NS_ASSERTION(*accessGranted, "new cache entry for READ-ONLY request");
         PR_APPEND_LINK(request, &mRequestQ);
         return rv;
     }
-    
-    if (IsDoomed()) return NS_ERROR_CACHE_ENTRY_DOOMED;
 
     if (IsStreamData() != request->IsStreamBased()) {
         *accessGranted = nsICache::ACCESS_NONE;
         return request->IsStreamBased() ?
             NS_ERROR_CACHE_DATA_IS_NOT_STREAM : NS_ERROR_CACHE_DATA_IS_STREAM;
     }
 
     if (PR_CLIST_IS_EMPTY(&mDescriptorQ)) {
@@ -221,16 +221,19 @@ nsCacheEntry::CreateDescriptor(nsCacheRe
     // XXX check request is on q
     PR_REMOVE_AND_INIT_LINK(request); // remove request regardless of success
 
     if (descriptor == nsnull)
         return NS_ERROR_OUT_OF_MEMORY;
 
     PR_APPEND_LINK(descriptor, &mDescriptorQ);
 
+    CACHE_LOG_DEBUG(("  descriptor %p created for request %p on entry %p\n",
+                    descriptor, request, this));
+
     NS_ADDREF(*result = descriptor);
     return NS_OK;
 }
 
 
 PRBool
 nsCacheEntry::RemoveRequest(nsCacheRequest * request)
 {
--- a/netwerk/cache/nsCacheService.cpp
+++ b/netwerk/cache/nsCacheService.cpp
@@ -1501,21 +1501,22 @@ nsCacheService::NotifyListener(nsCacheRe
 nsresult
 nsCacheService::ProcessRequest(nsCacheRequest *           request,
                                PRBool                     calledFromOpenCacheEntry,
                                nsICacheEntryDescriptor ** result)
 {
     // !!! must be called with mLock held !!!
     nsresult           rv;
     nsCacheEntry *     entry = nsnull;
+    nsCacheEntry *     doomedEntry = nsnull;
     nsCacheAccessMode  accessGranted = nsICache::ACCESS_NONE;
     if (result) *result = nsnull;
 
     while(1) {  // Activate entry loop
-        rv = ActivateEntry(request, &entry);  // get the entry for this request
+        rv = ActivateEntry(request, &entry, &doomedEntry);  // get the entry for this request
         if (NS_FAILED(rv))  break;
 
         while(1) { // Request Access loop
             NS_ASSERTION(entry, "no entry in Request Access loop!");
             // entry->RequestAccess queues request on entry
             rv = entry->RequestAccess(request, &accessGranted);
             if (rv != NS_ERROR_CACHE_WAIT_FOR_VALIDATION) break;
             
@@ -1542,16 +1543,34 @@ nsCacheService::ProcessRequest(nsCacheRe
         // loop back around to look for another entry
     }
 
     nsICacheEntryDescriptor *descriptor = nsnull;
     
     if (NS_SUCCEEDED(rv))
         rv = entry->CreateDescriptor(request, accessGranted, &descriptor);
 
+    // If doomedEntry is set, ActivatEntry() doomed an existing entry and
+    // created a new one for that cache-key. However, any pending requests
+    // on the doomed entry were not processed and we need to do that here.
+    // This must be done after adding the created entry to list of active
+    // entries (which is done in ActivateEntry()) otherwise the hashkeys crash
+    // (see bug ##561313). It is also important to do this after creating a
+    // descriptor for this request, or some other request may end up being
+    // executed first for the newly created entry.
+    // Finally, it is worth to emphasize that if doomedEntry is set,
+    // ActivateEntry() created a new entry for the request, which will be
+    // initialized by RequestAccess() and they both should have returned NS_OK.
+    if (doomedEntry) {
+        (void) ProcessPendingRequests(doomedEntry);
+        if (doomedEntry->IsNotInUse())
+            DeactivateEntry(doomedEntry);
+        doomedEntry = nsnull;
+    }
+
     if (request->mListener) {  // Asynchronous
     
         if (NS_FAILED(rv) && calledFromOpenCacheEntry)
             return rv;  // skip notifying listener, just return rv to caller
             
         // call listener to report error or descriptor
         nsresult rv2 = NotifyListener(request, descriptor, accessGranted, rv);
         if (NS_FAILED(rv2) && NS_SUCCEEDED(rv)) {
@@ -1619,25 +1638,28 @@ nsCacheService::OpenCacheEntry(nsCacheSe
     }
 
     return rv;
 }
 
 
 nsresult
 nsCacheService::ActivateEntry(nsCacheRequest * request, 
-                              nsCacheEntry ** result)
+                              nsCacheEntry ** result,
+                              nsCacheEntry ** doomedEntry)
 {
     CACHE_LOG_DEBUG(("Activate entry for request %p\n", request));
     
     nsresult        rv = NS_OK;
 
     NS_ASSERTION(request != nsnull, "ActivateEntry called with no request");
     if (result) *result = nsnull;
-    if ((!request) || (!result))  return NS_ERROR_NULL_POINTER;
+    if (doomedEntry) *doomedEntry = nsnull;
+    if ((!request) || (!result) || (!doomedEntry))
+        return NS_ERROR_NULL_POINTER;
 
     // check if the request can be satisfied
     if (!mEnableMemoryDevice && !request->IsStreamBased())
         return NS_ERROR_FAILURE;
     if (!IsStorageEnabledForPolicy_Locked(request->StoragePolicy()))
         return NS_ERROR_FAILURE;
 
     // search active entries (including those not bound to device)
@@ -1649,16 +1671,18 @@ nsCacheService::ActivateEntry(nsCacheReq
         PRBool collision = PR_FALSE;
         entry = SearchCacheDevices(request->mKey, request->StoragePolicy(), &collision);
         CACHE_LOG_DEBUG(("Device search for request %p returned %p\n",
                          request, entry));
         // When there is a hashkey collision just refuse to cache it...
         if (collision) return NS_ERROR_CACHE_IN_USE;
 
         if (entry)  entry->MarkInitialized();
+    } else {
+        NS_ASSERTION(entry->IsActive(), "Inactive entry found in mActiveEntries!");
     }
 
     if (entry) {
         ++mCacheHits;
         entry->Fetched();
     } else {
         ++mCacheMisses;
     }
@@ -1666,17 +1690,20 @@ nsCacheService::ActivateEntry(nsCacheReq
     if (entry &&
         ((request->AccessRequested() == nsICache::ACCESS_WRITE) ||
          ((request->StoragePolicy() != nsICache::STORE_OFFLINE) &&
           (entry->mExpirationTime <= SecondsFromPRTime(PR_Now()) &&
            request->WillDoomEntriesIfExpired()))))
 
     {
         // this is FORCE-WRITE request or the entry has expired
-        rv = DoomEntry_Internal(entry);
+        // we doom entry without processing pending requests, but store it in
+        // doomedEntry which causes pending requests to be processed below
+        rv = DoomEntry_Internal(entry, false);
+        *doomedEntry = entry;
         if (NS_FAILED(rv)) {
             // XXX what to do?  Increment FailedDooms counter?
         }
         entry = nsnull;
     }
 
     if (!entry) {
         if (! (request->AccessRequested() & nsICache::ACCESS_WRITE)) {
@@ -1688,17 +1715,17 @@ nsCacheService::ActivateEntry(nsCacheReq
         entry = new nsCacheEntry(request->mKey,
                                  request->IsStreamBased(),
                                  request->StoragePolicy());
         if (!entry)
             return NS_ERROR_OUT_OF_MEMORY;
         
         entry->Fetched();
         ++mTotalEntries;
-        
+
         // XXX  we could perform an early bind in some cases based on storage policy
     }
 
     if (!entry->IsActive()) {
         rv = mActiveEntries.AddEntry(entry);
         if (NS_FAILED(rv)) goto error;
         CACHE_LOG_DEBUG(("Added entry %p to mActiveEntries\n", entry));
         entry->MarkActive();  // mark entry active, because it's now in mActiveEntries
@@ -1768,17 +1795,19 @@ nsCacheService::SearchCacheDevices(nsCSt
     return entry;
 }
 
 
 nsCacheDevice *
 nsCacheService::EnsureEntryHasDevice(nsCacheEntry * entry)
 {
     nsCacheDevice * device = entry->CacheDevice();
-    if (device)  return device;
+    // return device if found, possibly null if the entry is doomed i.e prevent
+    // doomed entries to bind to a device (see e.g. bugs #548406 and #596443)
+    if (device || entry->IsDoomed())  return device;
 
     PRInt64 predictedDataSize = entry->PredictedDataSize();
 #ifdef NECKO_DISK_CACHE
     if (entry->IsStreamData() && entry->IsAllowedOnDisk() && mEnableDiskDevice) {
         // this is the default
         if (!mDiskDevice) {
             (void)CreateDiskDevice();  // ignore the error (check for mDiskDevice instead)
         }
@@ -1844,22 +1873,23 @@ nsCacheService::EnsureEntryHasDevice(nsC
         entry->SetCacheDevice(device);
     return device;
 }
 
 
 nsresult
 nsCacheService::DoomEntry(nsCacheEntry * entry)
 {
-    return gService->DoomEntry_Internal(entry);
+    return gService->DoomEntry_Internal(entry, true);
 }
 
 
 nsresult
-nsCacheService::DoomEntry_Internal(nsCacheEntry * entry)
+nsCacheService::DoomEntry_Internal(nsCacheEntry * entry,
+                                   PRBool doProcessPendingRequests)
 {
     if (entry->IsDoomed())  return NS_OK;
     
     CACHE_LOG_DEBUG(("Dooming entry %p\n", entry));
     nsresult  rv = NS_OK;
     entry->MarkDoomed();
     
     NS_ASSERTION(!entry->IsBinding(), "Dooming entry while binding device.");
@@ -1872,22 +1902,25 @@ nsCacheService::DoomEntry_Internal(nsCac
         CACHE_LOG_DEBUG(("Removed entry %p from mActiveEntries\n", entry));
         entry->MarkInactive();
      }
 
     // put on doom list to wait for descriptors to close
     NS_ASSERTION(PR_CLIST_IS_EMPTY(entry), "doomed entry still on device list");
     PR_APPEND_LINK(entry, &mDoomedEntries);
 
-    // tell pending requests to get on with their lives...
-    rv = ProcessPendingRequests(entry);
-    
-    // All requests have been removed, but there may still be open descriptors
-    if (entry->IsNotInUse()) {
-        DeactivateEntry(entry); // tell device to get rid of it
+    // handle pending requests only if we're supposed to
+    if (doProcessPendingRequests) {
+        // tell pending requests to get on with their lives...
+        rv = ProcessPendingRequests(entry);
+
+        // All requests have been removed, but there may still be open descriptors
+        if (entry->IsNotInUse()) {
+            DeactivateEntry(entry); // tell device to get rid of it
+        }
     }
     return rv;
 }
 
 
 void
 nsCacheService::OnProfileShutdown(PRBool cleanse)
 {
@@ -2264,16 +2297,21 @@ nsCacheService::DeactivateEntry(nsCacheE
 nsresult
 nsCacheService::ProcessPendingRequests(nsCacheEntry * entry)
 {
     nsresult            rv = NS_OK;
     nsCacheRequest *    request = (nsCacheRequest *)PR_LIST_HEAD(&entry->mRequestQ);
     nsCacheRequest *    nextRequest;
     PRBool              newWriter = PR_FALSE;
     
+    CACHE_LOG_DEBUG(("ProcessPendingRequests for %sinitialized %s %salid entry %p\n",
+                    (entry->IsInitialized()?"" : "Un"),
+                    (entry->IsDoomed()?"DOOMED" : ""),
+                    (entry->IsValid()? "V":"Inv"), entry));
+
     if (request == &entry->mRequestQ)  return NS_OK;    // no queued requests
 
     if (!entry->IsDoomed() && entry->IsInvalid()) {
         // 1st descriptor closed w/o MarkValid()
         NS_ASSERTION(PR_CLIST_IS_EMPTY(&entry->mDescriptorQ), "shouldn't be here with open descriptors");
 
 #if DEBUG
         // verify no ACCESS_WRITE requests(shouldn't have any of these)
@@ -2283,16 +2321,17 @@ nsCacheService::ProcessPendingRequests(n
             request = (nsCacheRequest *)PR_NEXT_LINK(request);
         }
         request = (nsCacheRequest *)PR_LIST_HEAD(&entry->mRequestQ);        
 #endif
         // find first request with ACCESS_READ_WRITE (if any) and promote it to 1st writer
         while (request != &entry->mRequestQ) {
             if (request->AccessRequested() == nsICache::ACCESS_READ_WRITE) {
                 newWriter = PR_TRUE;
+                CACHE_LOG_DEBUG(("  promoting request %p to 1st writer\n", request));
                 break;
             }
 
             request = (nsCacheRequest *)PR_NEXT_LINK(request);
         }
         
         if (request == &entry->mRequestQ)   // no requests asked for ACCESS_READ_WRITE, back to top
             request = (nsCacheRequest *)PR_LIST_HEAD(&entry->mRequestQ);
@@ -2301,16 +2340,18 @@ nsCacheService::ProcessPendingRequests(n
         // XXX serialize their accesses, give them only read access, but force them to check validate flag?
         // XXX or do readers simply presume the entry is valid
     }
 
     nsCacheAccessMode  accessGranted = nsICache::ACCESS_NONE;
 
     while (request != &entry->mRequestQ) {
         nextRequest = (nsCacheRequest *)PR_NEXT_LINK(request);
+        CACHE_LOG_DEBUG(("  %sync request %p for %p\n",
+                        (request->mListener?"As":"S"), request, entry));
 
         if (request->mListener) {
 
             // Async request
             PR_REMOVE_AND_INIT_LINK(request);
 
             if (entry->IsDoomed()) {
                 rv = ProcessRequest(request, PR_FALSE, nsnull);
@@ -2328,17 +2369,17 @@ nsCacheService::ProcessPendingRequests(n
                              "if entry is valid, RequestAccess must succeed.");
                 // XXX if (newWriter)  NS_ASSERTION( accessGranted == request->AccessRequested(), "why not?");
 
                 // entry->CreateDescriptor dequeues request, and queues descriptor
                 nsICacheEntryDescriptor *descriptor = nsnull;
                 rv = entry->CreateDescriptor(request,
                                              accessGranted,
                                              &descriptor);
-                
+
                 // post call to listener to report error or descriptor
                 rv = NotifyListener(request, descriptor, accessGranted, rv);
                 delete request;
                 if (NS_FAILED(rv)) {
                     // XXX what to do?
                 }
                 
             } else {
@@ -2399,16 +2440,17 @@ nsCacheService::ClearActiveEntries()
 PLDHashOperator
 nsCacheService::DeactivateAndClearEntry(PLDHashTable *    table,
                                         PLDHashEntryHdr * hdr,
                                         PRUint32          number,
                                         void *            arg)
 {
     nsCacheEntry * entry = ((nsCacheEntryHashTableEntry *)hdr)->cacheEntry;
     NS_ASSERTION(entry, "### active entry = nsnull!");
+    // only called from Shutdown() so we don't worry about pending requests
     gService->ClearPendingRequests(entry);
     entry->DetachDescriptors();
     
     entry->MarkInactive();  // so we don't call Remove() while we're enumerating
     gService->DeactivateEntry(entry);
     
     return PL_DHASH_REMOVE; // and continue enumerating
 }
@@ -2418,17 +2460,17 @@ void
 nsCacheService::DoomActiveEntries()
 {
     nsAutoTArray<nsCacheEntry*, 8> array;
 
     mActiveEntries.VisitEntries(RemoveActiveEntry, &array);
 
     PRUint32 count = array.Length();
     for (PRUint32 i=0; i < count; ++i)
-        DoomEntry_Internal(array[i]);
+        DoomEntry_Internal(array[i], true);
 }
 
 
 PLDHashOperator
 nsCacheService::RemoveActiveEntry(PLDHashTable *    table,
                                   PLDHashEntryHdr * hdr,
                                   PRUint32          number,
                                   void *            arg)
--- a/netwerk/cache/nsCacheService.h
+++ b/netwerk/cache/nsCacheService.h
@@ -185,30 +185,33 @@ private:
 
     nsresult         CreateRequest(nsCacheSession *   session,
                                    const nsACString & clientKey,
                                    nsCacheAccessMode  accessRequested,
                                    PRBool             blockingMode,
                                    nsICacheListener * listener,
                                    nsCacheRequest **  request);
 
-    nsresult         DoomEntry_Internal(nsCacheEntry * entry);
+    nsresult         DoomEntry_Internal(nsCacheEntry * entry,
+                                        PRBool doProcessPendingRequests);
 
     nsresult         EvictEntriesForClient(const char *          clientID,
                                            nsCacheStoragePolicy  storagePolicy);
 
     // Notifies request listener asynchronously on the request's thread, and
     // releases the descriptor on the request's thread.  If this method fails,
     // the descriptor is not released.
     nsresult         NotifyListener(nsCacheRequest *          request,
                                     nsICacheEntryDescriptor * descriptor,
                                     nsCacheAccessMode         accessGranted,
                                     nsresult                  error);
 
-    nsresult         ActivateEntry(nsCacheRequest * request, nsCacheEntry ** entry);
+    nsresult         ActivateEntry(nsCacheRequest * request,
+                                   nsCacheEntry ** entry,
+                                   nsCacheEntry ** doomedEntry);
 
     nsCacheDevice *  EnsureEntryHasDevice(nsCacheEntry * entry);
 
     nsCacheEntry *   SearchCacheDevices(nsCString * key, nsCacheStoragePolicy policy, PRBool *collision);
 
     void             DeactivateEntry(nsCacheEntry * entry);
 
     nsresult         ProcessRequest(nsCacheRequest *           request,