Closed Bug 479328 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: ImageLib, defect, P2)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1b4

People

(Reporter: joe, Assigned: joe)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 2 obsolete files)

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!
Flags: blocking1.9.1?
Attached patch Hand-roll the cache removal (obsolete) — Splinter Review
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.
Assignee: nobody → joe
Attachment #363211 - Flags: superreview?(vladimir)
Attachment #363211 - Flags: review?(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 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.
Attachment #363211 - Flags: superreview?(vladimir)
Attachment #363211 - Flags: superreview?(bzbarsky)
Attachment #363211 - Flags: review?(vladimir)
Attachment #363211 - Flags: review?(bzbarsky)
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
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.
Attachment #363211 - Attachment is obsolete: true
Attachment #364148 - Flags: superreview?(bzbarsky)
Attachment #364148 - Flags: review?(vladimir)
Attachment #363211 - Flags: superreview?(bzbarsky)
Attachment #364148 - Attachment is obsolete: true
Attachment #365597 - Flags: superreview?(bzbarsky)
Attachment #365597 - Flags: review+
Attachment #364148 - Flags: superreview?(bzbarsky)
Comment on attachment 365597 [details] [diff] [review]
Update for bug 481553

sr=bzbarsky.  Sorry for the lag...
Attachment #365597 - Flags: superreview?(bzbarsky) → superreview+
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3bdd315ea739
Keywords: fixed1.9.1
Target Milestone: --- → mozilla1.9.1b4
You need to log in before you can comment on or make changes to this bug.