Skip to content

Create new image cache per document#36832

Merged
jdm merged 1 commit intoservo:mainfrom
TimvdLippe:pr-timvdlippe-fix-image-cache-sharing
May 4, 2025
Merged

Create new image cache per document#36832
jdm merged 1 commit intoservo:mainfrom
TimvdLippe:pr-timvdlippe-fix-image-cache-sharing

Conversation

@TimvdLippe
Copy link
Contributor

Rather than sharing the full image cache in a script_thread, the image cache is now unique per document. This ensures that CSP factors no longer affect whether the image is retrieved from the cache incorrectly.

To do so, the thread_pool is shared across all caches, but the store is fresh. Except for the place_holder{image,url}, which are cloned. That's because the rippy_data is only available in the constellation and no longer accessible at the point that we need to create the document in the script_thread.

Contrary to the description in #36505, the script_thread still has an image_cache for this reason: so it has access to the store and thread_pool to clone it.

With these changes, the two CSP tests no longer flake. Confirmed with running the following commmand:

./mach test-wpt tests/wpt/tests/content-security-policy/generic/ --rerun=10

Fixes #36505

@TimvdLippe TimvdLippe requested a review from gterzian as a code owner May 4, 2025 10:38
@TimvdLippe
Copy link
Contributor Author

@jdm
Copy link
Member

jdm commented May 4, 2025

Contrary to the description in #36505, the script_thread still has an image_cache for this reason: so it has access to the store and thread_pool to clone it.

This makes sense; unfortunately I was counting on removing it in order to identify the other uses that need to be updated.

Rather than sharing the full image cache in a script_thread,
the image cache is now unique per document. This ensures that
CSP factors no longer affect whether the image is retrieved
from the cache incorrectly.

To do so, the thread_pool is shared across all caches, but the
store is fresh. Except for the place_holder{image,url}, which
are cloned. That's because the `rippy_data` is only available
in the constellation and no longer accessible at the point
that we need to create the document in the script_thread.

Contrary to the description in servo#36505, the script_thread still
has an image_cache for this reason: so it has access to
the store and thread_pool to clone it.

With these changes, the two CSP tests no longer flake. Confirmed
with running the following commmand:

```
./mach test-wpt tests/wpt/tests/content-security-policy/generic/ --rerun=10
```

Fixes servo#36505

Signed-off-by: Tim van der Lippe <tvanderlippe@gmail.com>
@TimvdLippe TimvdLippe force-pushed the pr-timvdlippe-fix-image-cache-sharing branch from e5fd0b3 to e6c9754 Compare May 4, 2025 19:49
@TimvdLippe
Copy link
Contributor Author

TimvdLippe commented May 4, 2025

Copy link
Member

@jdm jdm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@jdm jdm enabled auto-merge May 4, 2025 19:57
@jdm jdm added this pull request to the merge queue May 4, 2025
Merged via the queue into servo:main with commit 8a83777 May 4, 2025
21 checks passed
@TimvdLippe TimvdLippe deleted the pr-timvdlippe-fix-image-cache-sharing branch May 9, 2025 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants