811129 - Instead of storing imgIContainer state on imgRequest/imgRequestProxy until the image is created, use a temporary image object and transfer the state

Status: NEW

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

Right now we allow callers who have access to an imgRequest or imgRequestProxy to call methods that would mutate state on the image (for example lock/unlock). These classes store that state until the image becomes available, at which point they replay the state onto the image. If the image changes (for example, if new frames of an animated GIF become available), the state has to be read from the old image, the old image is discarded, and then the state is replayed onto the new image.

This code is rather complex and errorprone. A simpler solution would be to start with a special Image subclass that only exists to hold state, and add a method that transfers the state from one Image to another which is called when an image is first available and when things like new frames cause the image to change.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Talked about this IRL with Joe and Jeff and we're going to go for this change.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Unfortunately I'm not sure that this is going to work out at this point. The problem is imgRequestProxy::ChangeOwner(). When we point the imgRequestProxy at a different request, we are also pointing it at a different image. However, the old version of the image may still be in use. (AFAICT, for example, data URIs never pass the cache validation - which may in itself be unintended behavior, not sure - and imgCacheValidator will always end up calling ChangeOwner on its proxies.) This means that we can't just move the lock count, animation consumer count, etc. from the old version of the image to the new one. Unless nsDocument is changed to talk directly to images and not to imgRequestProxies when performing locking and unlocking and similar, I think this just won't work.

Comment 3

[Seth Fowler [:seth] [:s2h]]

So, I've now given this further thought, and I no longer think nsDocument talking directly to images is the right fix.

The problem is clearer when we look at the broader context. When we need to validate a cached image request over the network in ValidateRequestWithNewChannel(), we create an imgCacheValidator that performs the network request, and return an imgRequestProxy associated with that imgCacheValidator, but with an mOwner pointing to the unvalidated old request stored in the image cache entry. Then, if we find out that validation failed and we need to redownload the image, we use ChangeOwner() to point the imgRequestProxy's mOwner member at a new imgRequest representing the new image download.

The problem is that in the time when we are waiting for validation results to come back from the network, LockImage() and similar can be called on the imgRequestProxy. If we keep the lock count and similar on the image, as proposed in this patch, imgRequestProxy::LockImage() will just call Image::LockImage() and forget anything ever happened. This means that when we call ChangeOwner() later, we don't know how many locks are associated with the current imgRequestProxy and how many come from elsewhere, so we can't safely move the locks over to the new image being downloaded by the new imgRequest.

The solution is basically "don't do that". Specifically, don't return an imgRequestProxy pointing to the old imgRequest. Instead, ValidateRequestWithNewChannel() should return an imgRequestProxy whose mOwner is an imgRequest with an "empty" image. The "empty" image will remember the image belonging to the old imgRequest, so if validation succeeds we can just replace one with the other. If validation fails we download the new image and handle things as usual. The empty image has only the lock count, animation consumer count, etc. of the new imgRequestProxy that ValidateRequestWithNewChannel() returns - it's not shared - so we can safely move everything over to whichever image we finally end up with without a problem.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.