1747494 - Multiple image requests aren't coalesced for image cache case

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

[undeaD_D]

Attachment: image_2021-12-23_22-24-52.png

Steps to reproduce:

Step 1: Open Link
https://undeadd.github.io/JavaCtf/test.html
(Sry for the weird url -> its a quick poc)

Step 2: Open Developer Network Tool
(Look out for Image loads)

Step 3: Click Link: "Test"
(Image gets loaded once)

Step 4: Click same Link again
(suddenly all the images get loaded seperately even if they are all the same src)

Step 5: Compare Network loads from Step 3 & 4

Actual results:

Firefox v95 and higher suddenly loads all the images seperately (in Step 4).
Larger quantities of images would lead to huge lagspikes and delay.

Expected results:

Firefox v94 and lower dont have this problem.
Step 4 should only trigger one image load operation similar to Step 3.

Comment 1

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

The Bugbug bot thinks this bug should belong to the 'Core::JavaScript Engine' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 2

[Tooru Fujisawa [:arai]]

bug 1722759 changed the Netmonitor behavior to log image requests loaded from image cache.

So, items logged at step 4 are image requests loaded from cache (see "Aus Cache" in "Übertragen" column), and no network requests happens at step 4.
But this behavior is inconsistent with the step 3 where image cache is not used and only one item is logged for multiple <img>s.

Comment 3

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

Set release status flags based on info from the regressing bug 1722759

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thank you for the report.

I can reproduce the issue using STRs in comment #0

@Bomsy: could we show just one "cached" request? (just like e.g. Chrome Net panel does)

Comment 5

[Hubert Boma Manilla (:bomsy)]

Hi Honza,

@Bomsy: could we show just one "cached" request? (just like e.g. Chrome Net panel does)

Yes we can just load one cache request. Should be straightforward to fix. I'll take a look.

Comment 6

[Hubert Boma Manilla (:bomsy)]

Attachment: Bug 1747494 - [devtools] Small cleanups r=ochameau

Depends on D134617

Comment 7

[Hubert Boma Manilla (:bomsy)]

Attachment: Bug 1747494 - [devtools] Should display only one request per URL for images from the cache r=ochameau

Depends on D135037

Comment 8

[Alexandre Poirot [:ochameau]]

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #4)

@Bomsy: could we show just one "cached" request? (just like e.g. Chrome Net panel does)

Note that chrome isn't as good as Firefox to displayed cached image request.
It does show a cached image request if you click on Test the first time, after having loaded the image in a previous page load.
Otherwise if you click Test again, Chrome won't show any request.

As Arai said in comment 2, Firefox doesn't fetch the images over the network except for the very first image loaded.
All subsequent requests displayed in the network monitor are fetched from the memory cache and not from the network. May be we should make that even clearer in the UI?
Now we have some discrepancy between the very first click on Test and all the subsequent ones.
We do not show all the images fetched from the cache on the very first click.
May be we should? Any idea why we aren't getting http-on-image-cache-response notifications?

Showing only the first one may be misleading?
Why would fetch("https://avatars.githubusercontent.com/u/8116188https://avatars.githubusercontent.com/u/8116188") display a request each time we call it?
Whereas var img = new Image(); img.src = "https://avatars.githubusercontent.com/u/8116188"; would only display a request once?

Comment 9

[Alexandre Poirot [:ochameau]]

Sean, any opinion about this?
Do you have any idea why http-on-image-cache-response isn't always fired? (Step 3 of comment 0's STR)

Comment 10

[Sean Feng [:sefeng]]

I was able to reproduce it locally.

We didn't fire it for the rest of the images because the cache entry exited but the request wasn't completed yet, so we exited early at https://searchfox.org/mozilla-central/rev/253ae246f642fe9619597f44de3b087f94e45a2d/image/imgLoader.cpp#2061-2063

I agree the behaviour should be consistent. I think if we only always want to get 1 image request in the devtool, then bomsy's fix is correct. However, if we want to always get multiple requests in the devtool, then we probably need to fix how we fire http-on-image-cache-response, perhaps it should be moved to imgRequestProxy::OnLoadComplete.

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Why "Disable Cache" (a checkbox within the Network panel) doesn't have an impact and the second click is showing cached images?

Comment 12

[Sean Feng [:sefeng]]

Because that's for HTTP cache? Images are loaded from the in-memory image cache for the second click.

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Sean Feng [:sefeng] from comment #12)

Because that's for HTTP cache? Images are loaded from the in-memory image cache for the second click.

I see, that make sense, thank you.
Are there any APIs that would allow DevTools to disable also the in-memory image cache?

Comment 15

[Sean Feng [:sefeng]]

I don't know anything about that.

Comment 16

[Alexandre Poirot [:ochameau]]

(In reply to Sean Feng [:sefeng] from comment #10)

I was able to reproduce it locally.

We didn't fire it for the rest of the images because the cache entry exited but the request wasn't completed yet, so we exited early at https://searchfox.org/mozilla-central/rev/253ae246f642fe9619597f44de3b087f94e45a2d/image/imgLoader.cpp#2061-2063

Ah ok, that's good to know!

I agree the behaviour should be consistent. I think if we only always want to get 1 image request in the devtool, then bomsy's fix is correct. However, if we want to always get multiple requests in the devtool, then we probably need to fix how we fire http-on-image-cache-response, perhaps it should be moved to imgRequestProxy::OnLoadComplete.

It sounds like you don't have a strong opinion here? I imagine that platform developers may have different point of view than web developers.
Showing absolutely all image loads may be useful to debug platform issues, while it might be confusing for web developers who have more interest about actual network requests.
Given that, the current fix is probably our best option, which we can easily revisit later if we want to display all image usages.

Comment 17

[Hubert Boma Manilla (:bomsy)]

I agree the behaviour should be consistent. I think if we only always want to get 1 image request in the devtool, then bomsy's fix is correct. However, if we want to always get multiple requests in the devtool, then we probably need to fix how we fire http-on-image-cache-response, perhaps it should be moved to imgRequestProxy::OnLoadComplete.

It sounds like you don't have a strong opinion here? I imagine that platform developers may have different point of view than web developers.
Showing absolutely all image loads may be useful to debug platform issues, while it might be confusing for web developers who have more interest about actual network requests.
Given that, the current fix is probably our best option, which we can easily revisit later if we want to display all image usages.

Maybe a good tradeoff might be showing some information indicating that this request is one of a number of requests (maybe some information in the tooltip). This might help platform developers.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/061ada982fc2
[devtools] Small  cleanups r=ochameau
https://hg.mozilla.org/integration/autoland/rev/bde2d5a50f7d
[devtools] Should display only one request per URL for images from the cache r=ochameau

Comment 19

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/061ada982fc2
https://hg.mozilla.org/mozilla-central/rev/bde2d5a50f7d