514605 - Possible for concurrent reloads of images to return old/different results

Status: RESOLVED FIXED

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

Description

[Joe Drew (not getting mail)]

Attachment: set load ID only if we get a valid cache hit

Because we set the load ID/context of images unconditionally if we get a cache hit, if we reload a page with two of the same image on it, we will actually give out a proxy to the old image on the second load (because the load ID has been updated to the current context).

Instead, if we're trying to validate an image over the network, we should only set the load ID once we know whether it's valid or not. The mechanics to share a new request already exist; this patch simply enables them.

This patch is required for the testcase in bug 497665 to work, so I'm marking this bug blocking-1.9.2? as well.

Comment 1

[Bobby Holley (:bholley)]

I don't know this code. To give a meaningful review I either need to spend some time internalizing imgLoader (might be a while) or we need better docs. I think the latter is probably more useful in the long run.

Things that should be documented:
 * What the heck is aCX? The documentation for it is literally "some random data"
 * What defines a "context"?
 * What exactly does ValidateEntry do? Under what conditions does it return true/false? Under what conditions does it set aProxyRequest? Given that this isn't an idl method, why do we have this crappy dual-rv call signature?
 * What is a caching channel?

In general I think LoadImage could really benefit from being split up into nice helper functions - the multi-page if/else logic is really difficult to follow. Ah, imagelib cleanup...

Comment 2

[Joe Drew (not getting mail)]

aCX is a pointer to some object that's loading the image. If that same object comes along and tries to load the image again, we will do none of the work to validate the image that we did initially; instead, we'll just hand back the image. A context is therefore an object that needs to validate an image; the optimization is entirely imglib-side.

ValidateEntry does all the necessary work to ensure a given image is still OK to use. It will set up a separate network load if it's needed to validate an image has not expired. This implicitly includes a new imgIRequest (implemented by imgRequestProxy). Every time you load an image, you get back an imgRequestProxy; whether that proxy points directly to your imgRequest or to an intermediate object that does the network work required to validate or invalidate your image is up to ValidateEntry. It sets aProxyRequest when the image needs to be validated, and if it is known to be valid (or we're ignoring validity), we don't bother to create a new one. That's why in LoadImage(), we check _retval to see if it's been created, and create a standard imgRequestProxy pointing to the underlying imgRequest in that case. That's why there are two return values - ValidateImage returns true if it's possible for the entry to be valid, which can be true regardless of whether you've got a new imgRequestProxy. (When it returns false, aProxyRequest is always NULL.)

A caching channel is a channel that can be backed by a cache. HTTP channels are caching channels, for example.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Some thoughts:

It would be nice if imgIRequest had better documentation describing what it does, how imgRequestProxy is the only implementer and how it's poorly named in relation to imgRequest.

It would be nice if we could create imgRequestProxies in one place and do it unconditionally because we always do this.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 398573 [details] [diff] [review]
set load ID only if we get a valid cache hit

This is insufficient for ensuring that all of the images with the same url on a page are the same. Discussed out loud.

Comment 5

[Joe Drew (not getting mail)]

Note: we are not going to fix this in the general case right now. I am going to add a comment saying it doesn't work in the general case.

Comment 6

[Joe Drew (not getting mail)]

Attachment: Add the comments Jeff wanted

Comment 7

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/917c34df349b

Comment 8

[Joe Drew (not getting mail)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/31c389dbefef