2010968 - Problem with CompressionDictionaries when a load is replacing a dictionary and using itself as the dictionary

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect, P2)

Description

[Randell Jesup [:jesup] (needinfo me)]

HttpChannel was calling InitCacheEntry right before ParseDictionary(), which is when it would realize it was replacing an old dictionary and might Suspend() the new request until the old dictionary had loaded the cache contents (for use in decoding active requests, including the one for the replacement). InitCacheEntry will see we're overwriting a cache entry and will truncate it, and also will notice there's a dictionary associated and tell it to remove itself. This happens before we can suspend and wait.

Moving InitCacheEntry below ParseDictionary, and putting a split inbetween where we check for Suspend lets the old dictionary load the cache contents before removing itself and Resume()ing the replacement load (which will be using the old dictionary to decompress the new one).

This is a weird edgecase in general, where we're loading the same URI even though it's cached, and using itself for compression - which will result in 99.999% compression (i.e. it will just dump out a copy of itself unless the contents of that URI on the server have changed, which is possible not isn't happening in this case. This is happening a LOT in ChatGPT (something like 30-some replacements during a load IIRC), and at least once in Shopify (some css file).

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 2010968: InitCacheEntry after ParseDictiony for Dictionaries r=#necko-reviewers!

Move InitCacheEntry to after parsing dictionaries and possibly suspending
the request.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Issues found with this case:

• InitCacheEntry called before ParseDictionary can suspend the replacement
• Content-Length needs to get removed/blanked/fixed if we decompress before storing in the cache (avoids thinking it's partial, though this seems to not hurt much -- so far)
• RemoveFromVary() needs to use case-insensitive comparisons (we need to remove available-dictionary and accept-encoding for dictionaries and dictionary-compressed responses)
• (not specific to this issue: GetMetaDataElement() should be case-insensitive for keys, or perhaps we should flatten header name case (not values) for request/response headers in the metadata, and flatten the lookup values for headers)
• In case we do end up revalidating anyways, we need to not let the dictionary load stall like normal ones do. Add a Cache load flag to set this

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 2010968: Clear Content-Length when we decompress to store in the cache r=#necko-reviewers!

The Content-Length in the saved cache headers needs to not conflict with
size of data in the cache

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 2010968: Dictionary cleanup, and handle errors better r=#necko-reviewers!

In particular, if we Suspend for 5+ seconds and timeout

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 2010968: Add OPEN_ALWAYS flag to open even if revalidating for Dictionary loads r=#necko-reviewers!

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 2010968: RemoveFromVary needs to be case-insensitive when removing entries r=#necko-reviewers!

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 2010968: test changes to RemoveFromVary r=#necko-reviewers!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/e349dcc7a981
https://hg.mozilla.org/integration/autoland/rev/71879ab12afc
InitCacheEntry after ParseDictiony for Dictionaries r=necko-reviewers,valentin
https://github.com/mozilla-firefox/firefox/commit/67cd89cd4fa5
https://hg.mozilla.org/integration/autoland/rev/d7d876d135ab
Clear Content-Length when we decompress to store in the cache r=necko-reviewers,valentin
https://github.com/mozilla-firefox/firefox/commit/e6baf9d9a8e8
https://hg.mozilla.org/integration/autoland/rev/a79807f3659f
Dictionary cleanup, and handle errors better r=necko-reviewers,valentin
https://github.com/mozilla-firefox/firefox/commit/d7c84fc09682
https://hg.mozilla.org/integration/autoland/rev/a017057df408
Add OPEN_ALWAYS flag to open even if revalidating for Dictionary loads r=necko-reviewers,valentin
https://github.com/mozilla-firefox/firefox/commit/b1159d787670
https://hg.mozilla.org/integration/autoland/rev/b5defdd1e162
RemoveFromVary needs to be case-insensitive when removing entries r=necko-reviewers,valentin
https://github.com/mozilla-firefox/firefox/commit/81cf99cbdbb0
https://hg.mozilla.org/integration/autoland/rev/c7dcd1cbf56c
test changes to RemoveFromVary r=necko-reviewers,valentin

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/71879ab12afc
https://hg.mozilla.org/mozilla-central/rev/d7d876d135ab
https://hg.mozilla.org/mozilla-central/rev/a79807f3659f
https://hg.mozilla.org/mozilla-central/rev/a017057df408
https://hg.mozilla.org/mozilla-central/rev/b5defdd1e162
https://hg.mozilla.org/mozilla-central/rev/c7dcd1cbf56c