The redirect handling added in bug 89419 a) doesn't handle internal redirects (which should be mostly ignored, except for noting the new channel, as are used in the PAC patch), but also b) doesn't cache the new location, because when OnChannelRedirect calls RemoveFromCache, mCacheEntry is nulled out and thus we don't re-add the cache entry. This doesn't cause incorrect behaviour in the 302 case because the important part is that the image is removed from the cache, but it *does* break bug 466586 because every image gets a redirect and isn't cached. This is bad!

Let's hand-roll our cache removal to ensure we don't null out the cache pointer. This is ugly but necessary. Also, imgCacheEntry doesn't use its mKeyURI member, so let's remove it.

Comment on attachment 363211 [details] [diff] [review] Hand-roll the cache removal Um... I can probably sr this, but whoever reviewed the cache-removal stuff should probably review it. :(

Comment on attachment 363211 [details] [diff] [review] Hand-roll the cache removal Should I explicitly add handling of INTERNAL redirects? We can't actually special-case for them unless the URI is also the same.

I'm not sure what comment 3 is asking. Explain?

If the flag in OnChannelRedirect is set to nsIChannelEventSink::REDIRECT_INTERNAL, should I do anything to special-case that? Alternately, should I just special-case newURI == oldURI to avoid things bouncing in and out of the cache? (With the PAC patch, redirects to the same URI happen on every load.)

Special-casing URI equality might be the way to go, assuming there's a drawback to bouncing in and out... what's the drawback?

Extra work done on every load, basically. It might not be important.

Is it more extra work than a URI equality comparison? If so, special-casing the URI equality case might be worth it.

This revision of the patch makes sure we resurrect image cache entries correctly (marking them as not evicted, and adding them back in to the queue and expiration tracker if necessary). It also special-cases "same URI spec" (not nsIURI.equals() because that's not guaranteed to have the same hash), and doesn't bounce the cache entry in and out of the cache in that case.

Comment on attachment 365597 [details] [diff] [review] Update for bug 481553 sr=bzbarsky. Sorry for the lag...