Bug 479328 and
bug 481753 - Ensure imgRequest always knows when it's in the cache, and doesn't try to do cache manipulation when it isn't. Also, fix redirect handling to not simply invalidate the cache entry. r=vlad, sr=bzbarsky
--- a/modules/libpr0n/src/imgLoader.cpp
+++ b/modules/libpr0n/src/imgLoader.cpp
@@ -123,18 +123,16 @@ static PRBool NewRequestAndEntry(nsIURI
if (!*entry) {
delete *request;
return PR_FALSE;
}
NS_ADDREF(*request);
NS_ADDREF(*entry);
- imgLoader::SetHasNoProxies(uri, *entry);
-
return PR_TRUE;
}
static PRBool ShouldRevalidateEntry(imgCacheEntry *aEntry,
nsLoadFlags aFlags,
PRBool aHasExpired)
{
PRBool bValidateEntry = PR_FALSE;
@@ -262,36 +260,32 @@ void imgCacheEntry::TouchWithSize(PRInt3
{
LOG_SCOPE(gImgLog, "imgCacheEntry::TouchWithSize");
mTouchedTime = SecondsFromPRTime(PR_Now());
// Don't update the cache if we've been removed from it or it doesn't care
// about our size or usage.
if (!Evicted() && HasNoProxies()) {
- // We can't use mKeyURI here, because we're not guaranteed to be updated if
- // the request has no observers and has thus dropped its reference to us.
nsCOMPtr<nsIURI> uri;
mRequest->GetKeyURI(getter_AddRefs(uri));
imgLoader::CacheEntriesChanged(uri, diff);
}
}
void imgCacheEntry::Touch(PRBool updateTime /* = PR_TRUE */)
{
LOG_SCOPE(gImgLog, "imgCacheEntry::Touch");
if (updateTime)
mTouchedTime = SecondsFromPRTime(PR_Now());
// Don't update the cache if we've been removed from it or it doesn't care
// about our size or usage.
if (!Evicted() && HasNoProxies()) {
- // We can't use mKeyURI here, because we're not guaranteed to be updated if
- // the request has no observers and has thus dropped its reference to us.
nsCOMPtr<nsIURI> uri;
mRequest->GetKeyURI(getter_AddRefs(uri));
imgLoader::CacheEntriesChanged(uri);
}
}
void imgCacheEntry::SetHasNoProxies(PRBool hasNoProxies)
{
@@ -706,16 +700,37 @@ PRBool imgLoader::PutIntoCache(nsIURI *k
} else {
PR_LOG(gImgLog, PR_LOG_DEBUG,
("[this=%p] imgLoader::PutIntoCache -- Element NOT already in the cache", nsnull));
}
if (!cache.Put(spec, entry))
return PR_FALSE;
+ // We can be called to resurrect an evicted entry.
+ if (entry->Evicted())
+ entry->SetEvicted(PR_FALSE);
+
+ // If we're resurrecting an entry with no proxies, put it back in the
+ // tracker and queue.
+ if (entry->HasNoProxies()) {
+ nsresult addrv = NS_OK;
+
+ if (gCacheTracker)
+ addrv = gCacheTracker->AddObject(entry);
+
+ if (NS_SUCCEEDED(addrv)) {
+ imgCacheQueue &queue = GetCacheQueue(key);
+ queue.Push(entry);
+ }
+ }
+
+ nsRefPtr<imgRequest> request(getter_AddRefs(entry->GetRequest()));
+ request->SetIsInCache(PR_TRUE);
+
return PR_TRUE;
}
PRBool imgLoader::SetHasNoProxies(nsIURI *key, imgCacheEntry *entry)
{
#if defined(PR_LOGGING)
nsCAutoString spec;
key->GetSpec(spec);
@@ -1034,16 +1049,19 @@ PRBool imgLoader::RemoveFromCache(nsIURI
if (entry->HasNoProxies()) {
if (gCacheTracker)
gCacheTracker->RemoveObject(entry);
queue.Remove(entry);
}
entry->SetEvicted(PR_TRUE);
+ nsRefPtr<imgRequest> request(getter_AddRefs(entry->GetRequest()));
+ request->SetIsInCache(PR_FALSE);
+
return PR_TRUE;
}
else
return PR_FALSE;
}
PRBool imgLoader::RemoveFromCache(imgCacheEntry *entry)
{
@@ -1065,16 +1083,17 @@ PRBool imgLoader::RemoveFromCache(imgCac
if (entry->HasNoProxies()) {
LOG_STATIC_FUNC(gImgLog, "imgLoader::RemoveFromCache removing from tracker");
if (gCacheTracker)
gCacheTracker->RemoveObject(entry);
queue.Remove(entry);
}
entry->SetEvicted(PR_TRUE);
+ request->SetIsInCache(PR_FALSE);
return PR_TRUE;
}
}
return PR_FALSE;
}
@@ -1264,18 +1283,17 @@ NS_IMETHODIMP imgLoader::LoadImage(nsIUR
PR_LOG(gImgLog, PR_LOG_DEBUG,
("[this=%p] imgLoader::LoadImage -- AsyncOpen() failed: 0x%x\n",
this, openRes));
request->CancelAndAbort(openRes);
return openRes;
}
// Try to add the new request into the cache.
- if (!PutIntoCache(aURI, entry))
- request->SetCacheable(PR_FALSE);
+ PutIntoCache(aURI, entry);
// If we did get a cache hit, use it.
} else {
// XXX: Should this be executed if an expired cache entry does not have a caching channel??
LOG_MSG_WITH_PARAM(gImgLog,
"imgLoader::LoadImage |cache hit|", "request", request);
// Update the request's LoadId
@@ -1401,18 +1419,17 @@ NS_IMETHODIMP imgLoader::LoadImageWithCh
NS_ADDREF(pl);
*listener = static_cast<nsIStreamListener*>(pl);
NS_ADDREF(*listener);
NS_RELEASE(pl);
// Try to add the new request into the cache.
- if (!PutIntoCache(uri, entry))
- request->SetCacheable(PR_FALSE);
+ PutIntoCache(uri, entry);
}
// XXX: It looks like the wrong load flags are being passed in...
requestFlags &= 0xFFFF;
rv = CreateNewProxyForRequest(request, loadGroup, aObserver,
requestFlags, nsnull, _retval);
request->NotifyProxyListener(static_cast<imgRequestProxy*>(*_retval));
@@ -1681,18 +1698,17 @@ NS_IMETHODIMP imgCacheValidator::OnStart
return NS_ERROR_OUT_OF_MEMORY;
}
mDestListener = static_cast<nsIStreamListener*>(pl);
// Try to add the new request into the cache. Note that the entry must be in
// the cache before the proxies' ownership changes, because adding a proxy
// changes the caching behaviour for imgRequests.
- if (!sImgLoader.PutIntoCache(uri, entry))
- request->SetCacheable(PR_FALSE);
+ sImgLoader.PutIntoCache(uri, entry);
PRUint32 count = mProxies.Count();
for (PRInt32 i = count-1; i>=0; i--) {
imgRequestProxy *proxy = static_cast<imgRequestProxy *>(mProxies[i]);
proxy->ChangeOwner(request);
request->NotifyProxyListener(proxy);
}
--- a/modules/libpr0n/src/imgLoader.h
+++ b/modules/libpr0n/src/imgLoader.h
@@ -163,17 +163,16 @@ private: // methods
// Private, unimplemented copy constructor.
imgCacheEntry(const imgCacheEntry &);
private: // data
nsAutoRefCnt mRefCnt;
NS_DECL_OWNINGTHREAD
nsRefPtr<imgRequest> mRequest;
- nsCOMPtr<nsIURI> mKeyURI;
PRUint32 mDataSize;
PRInt32 mTouchedTime;
PRInt32 mExpiryTime;
nsExpirationState mExpirationState;
PRPackedBool mMustValidateIfExpired : 1;
PRPackedBool mEvicted : 1;
PRPackedBool mHasNoProxies : 1;
};
--- a/modules/libpr0n/src/imgRequest.cpp
+++ b/modules/libpr0n/src/imgRequest.cpp
@@ -80,17 +80,17 @@ NS_IMPL_ISUPPORTS8(imgRequest, imgILoad,
nsISupportsWeakReference,
nsIChannelEventSink,
nsIInterfaceRequestor)
imgRequest::imgRequest() :
mImageStatus(imgIRequest::STATUS_NONE), mState(0), mCacheId(0),
mValidator(nsnull), mImageSniffers("image-sniffing-services"),
mIsMultiPartChannel(PR_FALSE), mLoading(PR_FALSE), mProcessing(PR_FALSE),
- mHadLastPart(PR_FALSE), mGotData(PR_FALSE), mIsCacheable(PR_TRUE)
+ mHadLastPart(PR_FALSE), mGotData(PR_FALSE), mIsInCache(PR_FALSE)
{
/* member initializers and constructor code */
}
imgRequest::~imgRequest()
{
if (mKeyURI) {
nsCAutoString spec;
@@ -414,24 +414,25 @@ nsresult imgRequest::GetSecurityInfo(nsI
NS_IF_ADDREF(*aSecurityInfo = mSecurityInfo);
return NS_OK;
}
void imgRequest::RemoveFromCache()
{
LOG_SCOPE(gImgLog, "imgRequest::RemoveFromCache");
- if (mIsCacheable) {
+ if (mIsInCache) {
+ // mCacheEntry is nulled out when we have no more observers.
if (mCacheEntry)
imgLoader::RemoveFromCache(mCacheEntry);
else
imgLoader::RemoveFromCache(mKeyURI);
+ }
- mCacheEntry = nsnull;
- }
+ mCacheEntry = nsnull;
}
PRBool imgRequest::HaveProxyWithObserver(imgRequestProxy* aProxyToIgnore) const
{
nsTObserverArray<imgRequestProxy*>::ForwardIterator iter(mObservers);
imgRequestProxy* proxy;
while (iter.HasMore()) {
proxy = iter.GetNext();
@@ -468,23 +469,20 @@ void imgRequest::AdjustPriority(imgReque
if (mObservers.SafeElementAt(0, nsnull) != proxy)
return;
nsCOMPtr<nsISupportsPriority> p = do_QueryInterface(mRequest);
if (p)
p->AdjustPriority(delta);
}
-void imgRequest::SetCacheable(PRBool cacheable)
+void imgRequest::SetIsInCache(PRBool incache)
{
- LOG_FUNC_WITH_PARAM(gImgLog, "imgRequest::SetIsCacheable", "cacheable", cacheable);
- mIsCacheable = cacheable;
-
- if (!mIsCacheable)
- mCacheEntry = nsnull;
+ LOG_FUNC_WITH_PARAM(gImgLog, "imgRequest::SetIsCacheable", "incache", incache);
+ mIsInCache = incache;
}
/** imgILoad methods **/
NS_IMETHODIMP imgRequest::SetImage(imgIContainer *aImage)
{
LOG_FUNC(gImgLog, "imgRequest::SetImage");
@@ -1074,35 +1072,48 @@ imgRequest::OnChannelRedirect(nsIChannel
nsresult rv = NS_OK;
nsCOMPtr<nsIChannelEventSink> sink(do_GetInterface(mPrevChannelSink));
if (sink) {
rv = sink->OnChannelRedirect(oldChannel, newChannel, flags);
if (NS_FAILED(rv))
return rv;
}
-#if defined(PR_LOGGING)
- nsCAutoString spec;
- mKeyURI->GetSpec(spec);
-
- LOG_MSG_WITH_PARAM(gImgLog, "imgRequest::OnChannelRedirect", "old", spec.get());
-#endif
-
- RemoveFromCache();
-
mChannel = newChannel;
- newChannel->GetOriginalURI(getter_AddRefs(mKeyURI));
+ // Don't make any cache changes if we're going to point to the same thing. We
+ // compare specs and not just URIs here because URIs that compare as
+ // .Equals() might have different hashes.
+ nsCAutoString oldspec;
+ if (mKeyURI)
+ mKeyURI->GetSpec(oldspec);
+ LOG_MSG_WITH_PARAM(gImgLog, "imgRequest::OnChannelRedirect", "old", oldspec.get());
-#if defined(PR_LOGGING)
- mKeyURI->GetSpec(spec);
+ nsCOMPtr<nsIURI> newURI;
+ newChannel->GetOriginalURI(getter_AddRefs(newURI));
+ nsCAutoString newspec;
+ if (newURI)
+ newURI->GetSpec(newspec);
+ LOG_MSG_WITH_PARAM(gImgLog, "imgRequest::OnChannelRedirect", "new", newspec.get());
- LOG_MSG_WITH_PARAM(gImgLog, "imgRequest::OnChannelRedirect", "new", spec.get());
-#endif
+ if (oldspec != newspec) {
+ if (mIsInCache) {
+ // Remove the cache entry from the cache, but don't null out mCacheEntry
+ // (as imgRequest::RemoveFromCache() does), because we need it to put
+ // ourselves back in the cache.
+ if (mCacheEntry)
+ imgLoader::RemoveFromCache(mCacheEntry);
+ else
+ imgLoader::RemoveFromCache(mKeyURI);
+ }
- if (mIsCacheable) {
- // If we don't still have a cache entry, we don't want to refresh the cache.
- if (mKeyURI && mCacheEntry)
- imgLoader::PutIntoCache(mKeyURI, mCacheEntry);
+ mKeyURI = newURI;
+
+ if (mIsInCache) {
+ // If we don't still have a URI or cache entry, we don't want to put
+ // ourselves back into the cache.
+ if (mKeyURI && mCacheEntry)
+ imgLoader::PutIntoCache(mKeyURI, mCacheEntry);
+ }
}
return rv;
}
--- a/modules/libpr0n/src/imgRequest.h
+++ b/modules/libpr0n/src/imgRequest.h
@@ -159,20 +159,20 @@ private:
// Adjust the priority of the underlying network request by the given delta
// on behalf of the given proxy.
void AdjustPriority(imgRequestProxy *aProxy, PRInt32 aDelta);
// Return whether we've seen some data at this point
PRBool HasTransferredData() const { return mGotData; }
- // Set whether this request is cacheable. By default, all requests are
- // cacheable, but they might not be if there is already a request with this
- // key URI in the cache.
- void SetCacheable(PRBool cacheable);
+ // Set whether this request is stored in the cache. If it isn't, regardless
+ // of whether this request has a non-null mCacheEntry, this imgRequest won't
+ // try to update or modify the image cache.
+ void SetIsInCache(PRBool cacheable);
public:
NS_DECL_IMGILOAD
NS_DECL_IMGIDECODEROBSERVER
NS_DECL_IMGICONTAINEROBSERVER
NS_DECL_NSISTREAMLISTENER
NS_DECL_NSIREQUESTOBSERVER
NS_DECL_NSICHANNELEVENTSINK
@@ -208,12 +208,12 @@ private:
imgCacheValidator *mValidator;
nsCategoryCache<nsIContentSniffer> mImageSniffers;
PRPackedBool mIsMultiPartChannel : 1;
PRPackedBool mLoading : 1;
PRPackedBool mProcessing : 1;
PRPackedBool mHadLastPart : 1;
PRPackedBool mGotData : 1;
- PRPackedBool mIsCacheable : 1;
+ PRPackedBool mIsInCache : 1;
};
#endif
--- a/testing/mochitest/runtests.py.in
+++ b/testing/mochitest/runtests.py.in
@@ -462,16 +462,19 @@ Are you executing $objdir/_tests/testing
urlOpts.append("logFile=" + encodeURIComponent(options.logFile))
urlOpts.append("fileLevel=" + encodeURIComponent(options.fileLevel))
if options.consoleLevel:
urlOpts.append("consoleLevel=" + encodeURIComponent(options.consoleLevel))
if len(urlOpts) > 0:
testURL += "?" + "&".join(urlOpts)
browserEnv["XPCOM_MEM_BLOAT_LOG"] = LEAK_REPORT_FILE
+ if options.a11y:
+ browserEnv["XPCOM_MEM_REFCNT_LOG"] = PROFILE_DIRECTORY + "/refcnt.log"
+ browserEnv["XPCOM_MEM_LOG_CLASSES"] = "imgRequest,imgCacheEntry"
if options.fatalAssertions:
browserEnv["XPCOM_DEBUG_BREAK"] = "stack-and-abort"
status = automation.runApp(testURL, browserEnv, options.app,
PROFILE_DIRECTORY, options.browserArgs,
runSSLTunnel = True,
utilityPath = options.utilityPath,