Skip to content

/ servo

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Images loaded from file:// urls are not meaningfully cached #24720

Closed
jdm opened this issue Nov 13, 2019 · 7 comments
Closed

Images loaded from file:// urls are not meaningfully cached #24720

jdm opened this issue Nov 13, 2019 · 7 comments
Labels
A-content/images C-assigned C-has-manual-testcase E-candidate-for-mentoring

Comments

@jdm
Copy link
Member

@jdm jdm commented Nov 13, 2019

RUST_LOG=net ./mach run tests/wpt/mozilla/tests/css/background_size_zero_a.html shows an endless stream of image loads happening because there's a new opaque origin provided every time. This is fallout from #24340, presumably because file:// urls generate a new opaque origin. This happens because when we create a LayoutContext in layout_thread/lib.rs, we pass self.url.origin() as the origin that gets used in get_or_request_image_or_meta, when we should be obtaining it from the window when kicking off reflow instead.
@nmvk
Copy link
Contributor

@nmvk nmvk commented Nov 14, 2019

@highfive assign me
@highfive highfive added the C-assigned label Nov 14, 2019
@highfive
Copy link

@highfive highfive commented Nov 14, 2019

Hey @nmvk! Thanks for your interest in working on this issue. It's now assigned to you!
@kunalmohan
Copy link
Collaborator

@kunalmohan kunalmohan commented Feb 13, 2020

@nmvk are you still working on this issue? If not I'd like to give it a try.
@jdm
Copy link
Member Author

@jdm jdm commented Feb 13, 2020

@kunalmohan It's been several months with no update; I think you can start working on it :)
@kunalmohan
Copy link
Collaborator

@kunalmohan kunalmohan commented Feb 13, 2020

Thanks @jdm! I'll get started with it right away.
@kunalmohan
Copy link
Collaborator

@kunalmohan kunalmohan commented Feb 14, 2020

@jdm I am going to need some help here. I am trying to understand the flow of things that are happening here. Here's in brief of what I have understood-

  1. The get_or_request_image_or_meta function calls find_image_or_metadata function under net/image_cache.rs.
  2. find_image_or_metadata calls get_cache fn which should return CacheResult::Hit (in case of reflow) but instead returns CacheResult::Miss (Miss should be returned just for the first time).
  3. This causes the image image to be listed as a pending image.

Am I right here?
@jdm
Copy link
Member Author

@jdm jdm commented Feb 14, 2020

Yes, that is correct.
@kunalmohan kunalmohan mentioned this issue Feb 15, 2020
Merged
4 of 4 tasks complete
bors-servo added a commit that referenced this issue Feb 17, 2020
@bors-servo
Auto merge of #25777 - kunalmohan:24720-ImageCache, r=<try>
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
e9b0982 
Take origin from current window instead of creating a new one in event of reflow

Everytime a new `LayoutContext` was created, it created a new origin which
caused endless stream of image loads to occur in case of reflow. The reason
for this was that the existing image, although cached successfully, was not
used because the entry in hashmap did not match because of different(new)
origin.
This is solved by storing the origin of a window in enum `ScriptReflow` and
used in creating new `LayoutContext` in case of reflow.

<!-- Please describe your changes on the following line: -->
r?@jdm

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #24720  (GitHub issue number if applicable)

<!-- Either: -->
- [X] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Feb 19, 2020
@bors-servo
Auto merge of #25777 - kunalmohan:24720-ImageCache, r=jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
d719b38 
Take origin from current window instead of creating a new one in event of reflow

Everytime a new `LayoutContext` was created, it created a new origin which
caused endless stream of image loads to occur in case of reflow. The reason
for this was that the existing image, although cached successfully, was not
used because the entry in hashmap did not match because of different(new)
origin.
This is solved by storing the origin of a window in enum `ScriptReflow` and
used in creating new `LayoutContext` in case of reflow.

<!-- Please describe your changes on the following line: -->
r?@jdm

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #24720  (GitHub issue number if applicable)

<!-- Either: -->
- [X] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Feb 19, 2020
@bors-servo
Auto merge of #25777 - kunalmohan:24720-ImageCache, r=jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
6cb3f73 
Take origin from current window instead of creating a new one in event of reflow

Everytime a new `LayoutContext` was created, it created a new origin which
caused endless stream of image loads to occur in case of reflow. The reason
for this was that the existing image, although cached successfully, was not
used because the entry in hashmap did not match because of different(new)
origin.
This is solved by storing the origin of a window in enum `ScriptReflow` and
used in creating new `LayoutContext` in case of reflow.

<!-- Please describe your changes on the following line: -->
r?@jdm

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #24720  (GitHub issue number if applicable)

<!-- Either: -->
- [X] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-content/images C-assigned C-has-manual-testcase E-candidate-for-mentoring
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

Take origin from current window instead of creating a new one in event of reflow
4 participants
@jdm @nmvk @highfive @kunalmohan
You can’t perform that action at this time.