1760532 - Images are not cached correctly if the img crossOrigin property is set after the src property

Status: RESOLVED DUPLICATE of bug 1832528

(Core :: Graphics: ImageLib, defect)

Description

[Brian Peiris]

Attachment: crossorigin-img.html

If an img element is added to the DOM with its crossOrigin property set to "anonymous", after its src property is set, Firefox will always make a network request to the image even if it is cacheable.

See the simplified test case that is attached.

Steps to reproduce:

1. Open the attached test case.
2. Refresh the page with Ctrl+F5 or Ctrl+Shift+R, to ensure your cache is cleared.
3. Click on the "toggle image" button.
4. See that the image takes a while to load. This is expected.
5. Click the "toggle image" to remove the image.
6. Click the "toggle image" button again.
7. See that the image takes a while to load again, even though it should be cached.

The test case loads an image from a server that intentionally responds slowly (with a 0.5s delay), to demonstrate the issue.

This issue does not occur if the crossOrigin property is set before the src property. Chrome does not re-request the image with the same test case either way.

Comment 1

[Andreas Farre [:farre] (PTO -> Dec 7th)]

I'm a bit uncertain of which component this should be in, and it's fully possible that it's a DOM Core issue. But I thought I'd ask if it fits better in either Content Security due to CORS, or Necko due to caching.

Comment 2

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Andreas Farre [:farre] from comment #1)

I'm a bit uncertain of which component this should be in, and it's fully possible that it's a DOM Core issue. But I thought I'd ask if it fits better in either Content Security due to CORS, or Necko due to caching.

I am keeping my ni? for now so it remains on my radar.

Dan can you take a look please? Maybe something to talk about during the next dom:sec triage meeting?

Comment 3

[Daniel Veditz [:dveditz]]

Putting Christoph's ni? back as seemed to be the intent of comment 2 (victim of the "redirect needinfo request to" default setting)

Comment 4

[Emilio Cobos Álvarez [:emilio]]

I think this bug 1076583: Right now we're loading on .src assignment synchronously, while per spec it ought to be async. .srcset should work as the reporter expects, btw.

Since setting the crossOrigin attribute is a "relevant mutation" (https://html.spec.whatwg.org/#relevant-mutations) then we trigger another load. Should happen with any of those attributes, not just crossorigin.

Comment 5

[Daniel Veditz [:dveditz]]

I recommend setting the onload handler first. You'll see that the image is requested as soon as you set the src property. Then when you set the crossOrigin property to "anonymous" the image needs to be re-fetched because there's no guarantee an anonymous request will get the same response as the original one (for one thing, no cookies are sent for anonymous requests, and the value of Sec-Fetch-Mode: is "no-cors" rather than "cors").

If you open about:cache?storage=disk you'll see we store the image twice, with slightly different information under the URL. The anonymous request will have an extra a, at the end, after the other originAttributes.

I don't know why Chrome is different and only requests the image once, but clearly either one of the browsers is wrong or the spec is ambiguous. We've re-loaded the image so the data matches the value of the crossOrigin property, and maybe Chrome uses whatever the crossOrigin property happens to be when you set the .src property and doesn't care if you lie to yourself by setting crossOrigin after (but would respect it if you otherwise caused that image object to load again.

I'm having trouble interpreting the spec. https://html.spec.whatwg.org/#reacting-to-dom-mutations says that changing the crossorigin attribute is a "relevant mutation", but it's not clear to me what that means. The height and width are also "relevant mutations" and I don't think those are supposed to trigger reloads. On the other hand, https://html.spec.whatwg.org/#link-type-modulepreload has a note that says

Unlike some other link relations, changing the relevant attributes (such as as, crossorigin, and referrerpolicy) of such a link does not trigger a new fetch.

...which seems to be saying changing crossorigin everywhere else (like on an IMG) should in fact do a new fetch -- which we're doing. Or maybe Mozilla has interpreted it that way, and Chrome has interpreted that to only apply to literal <link> relations and not other kinds of elements that have a src instead.

I think I need to bounce this to Anne

Comment 6

[Brian Peiris]

Attachment: crossorigin-img-enhanced.html

Comment 7

[Brian Peiris]

Thanks for the added detail. I did have the misconception that the image would only be fetched when the element was added to the page, but you're right, if there weren't additional blocking javascript to run, the fetch would happen immediately after setting either src or crossOrigin.

It's certainly understandable that the browser would have to make a new request when crossOrigin changes, since as you mentioned, the parameters of the request have changed.

However, there is still a caching bug here. Even if you did set crossOrigin after setting src, I would expect the request to be fetched from cache on subsequent requests.

In other words, here's what I would expect:

1. I create an image element, ImageA
2. I set src [A fetch happens here, the response is cached]
3. I set crossOrigin [A different fetch happens here, the response is cached with different attributes]
4. Without refreshing the page, or clearing cache, I create a new image element, ImageB
5. I set src [A fetch should not happen here, since it should be satisfied by the cached response from #2]
6. I set crossOrigin [A fetch should not happen here, since it should be satisfied by the cache response from #3]

Instead, what we see is that Firefox will fetch the image from the server in step #6, instead of using the cached response from #3.

I've enhanced my test case and attached it here. https://bug1760532.bmoattachments.org/attachment.cgi?id=9269206
It now handles onload before setting src or crossOrigin, as you suggested.

The default options in the test will demonstrate the issue more clearly.

1. Click the "toggle image" button multiple times to see the broken cache behavior
2. Uncheck "set cross origin after src"
3. Click the "toggle image" button multiple times to see the expected cache behavior

I've added options to test different server responses as well. One with no cache-control header, one with a cache-control header of 1 year (the default), and one with a cache-control header of one year, with the immutable attribute. Interestingly, the caching behavior is correct with the immutable attribute.

I've also added options to add async sleeps between actions, so that you can see the fetches happen in between, instead of being blocked by the javascript.

It might also be helpful to watch the devtools network tab when testing the different test configurations, since the behavior differs in some scenarios.

Comment 8

[Emilio Cobos Álvarez [:emilio]]

We don't cache requests if the CORSMode doesn't match, so that explains what you're seeing: https://searchfox.org/mozilla-central/rev/be4604e4be8c71b3c1dbff2398a5b05f15411673/image/imgLoader.cpp#803-812

Comment 9

[Anne (:annevk)]

I suspect dveditz accidentally removed this dependency. I think comment 4 and comment 8 are spot on with regards to what is going on here. (And Component-wise I think we are good too.)

Comment 10

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

The severity field is not set for this bug.
:mccr8, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 11

[Kris Maglione [:kmag]]

(In reply to Daniel Veditz [:dveditz] from comment #5)

I'm having trouble interpreting the spec. https://html.spec.whatwg.org/#reacting-to-dom-mutations says that changing the crossorigin attribute is a "relevant mutation", but it's not clear to me what that means. The height and width are also "relevant mutations" and I don't think those are supposed to trigger reloads.

The width attribute affects the image source if we have a srcset, so we do need to trigger a new load if it's changed in a way that would require a different source. We just don't have to necessarily make any changes when the mutation doesn't affect the source unless the src attribute is explicitly set to its current value.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #8)

We don't cache requests if the CORSMode doesn't match, so that explains what you're seeing: https://searchfox.org/mozilla-central/rev/be4604e4be8c71b3c1dbff2398a5b05f15411673/image/imgLoader.cpp#803-812

Just to be clear, when the first load for the image is triggered, the request goes into the image cache keyed on the URI, the triggering origin attributes, and the document. It isn't keyed on the CORSMode, so two requests with the same URI but different CORSModes can't be in the cache at the same time. When the second request is made with a different CORSMode, the entry with the original CORSMode prevents it from being cached, so the second <img> with the crossorigin attribute doesn't have access to it.

I'm not sure if we want to fix that, since it would make the image cache somewhat more complicated and expensive.

That said, I think the network cache should still work for the requests as expected, but may require revalidation depending on cache headers.

Comment 12

[Bob Hood [:bhood]]

Tim, any insights you might be able to provide here?

Comment 13

[Timothy Nikkel (:tnikkel)]

I'll hopefully have more to say here, but I don't think this is S2 severity level. Happy to consider other thoughts if I've missed something.

Comment 14

[Timothy Nikkel (:tnikkel)]

I think bug 1832528 should fix this.

Comment 15

[Brian Peiris]

Looks like this is fixed. The attached test case no longer reproduces the issue in Firefox 120.
I don't have access to the Mozilla employee account I used to create this report, so please go ahead and close this for me.
Thanks!