479328 - imglib redirects don't handle internal redirects properly, are incorrect

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P2)

Description

[Joe Drew (not getting mail)]

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!

Comment 1

[Joe Drew (not getting mail)]

Attachment: Hand-roll the cache removal

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 2

[Boris Zbarsky [:bzbarsky]]

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 3

[Joe Drew (not getting mail)]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

Comment 5

[Joe Drew (not getting mail)]

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.)

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

Comment 7

[Joe Drew (not getting mail)]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

Comment 9

[Joe Drew (not getting mail)]

Attachment: Revise and complete previous patch

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 10

[Joe Drew (not getting mail)]

Attachment: Update for bug 481553

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 365597 [details] [diff] [review]
Update for bug 481553

sr=bzbarsky.  Sorry for the lag...

Comment 12

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/d3c2bf145fb8

Comment 13

[Joe Drew (not getting mail)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3bdd315ea739