Skip to content
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

Remove network requests from image cache thread #14962

Merged
merged 10 commits into from Feb 23, 2017

Conversation

jdm
Copy link
Member

@jdm jdm commented Jan 11, 2017

The design of initiating network requests from the image cache thread was simple, but it makes it difficult to implement image loading that conforms to the HTML specification. These changes make the implementation of HTMLImageElement responsible for network requests for <img> elements, and CSS-based images (background-image, bullets, etc.) are requested by the script thread to ensure that the layout thread does not attempt to retain unsafe pointers to DOM nodes during asynchronous operations.



This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/layout_image.rs, components/script/dom/htmlcanvaselement.rs, components/script/dom/window.rs, components/script/lib.rs, components/net_traits/lib.rs, components/net_traits/lib.rs, components/net_traits/image_cache_thread.rs, components/net_traits/image_cache_thread.rs, components/script/document_loader.rs, components/net/image_cache_thread.rs, components/script/dom/htmlimageelement.rs, components/script_layout_interface/lib.rs, components/script/script_thread.rs, components/script_layout_interface/rpc.rs, components/script/dom/canvasrenderingcontext2d.rs, components/script/dom/bindings/trace.rs
  • @fitzgen: components/script/layout_image.rs, components/script/dom/htmlcanvaselement.rs, components/script/dom/window.rs, components/script/lib.rs, components/script/document_loader.rs, components/script/dom/htmlimageelement.rs, components/script_layout_interface/lib.rs, components/script/script_thread.rs, components/script_layout_interface/rpc.rs, components/script/dom/canvasrenderingcontext2d.rs, components/script/dom/bindings/trace.rs
  • @emilio: components/layout/display_list_builder.rs, components/layout/context.rs, components/layout/construct.rs, components/layout/fragment.rs, components/layout/query.rs

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jan 11, 2017
@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #13972) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Jan 11, 2017
@jdm
Copy link
Member Author

jdm commented Jan 11, 2017

@bors-servo: try

@bors-servo
Copy link
Contributor

⌛ Trying commit f2cda69 with merge 4f42fe3...

bors-servo pushed a commit that referenced this pull request Jan 11, 2017
Remove network requests from image cache thread

The design of initiating network requests from the image cache thread was simple, but it makes it difficult to implement image loading that conforms to the HTML specification. These changes make the implementation of HTMLImageElement responsible for network requests for `<img>` elements, and CSS-based images (background-image, bullets, etc.) are requested by the script thread to ensure that the layout thread does not attempt to retain unsafe pointers to DOM nodes during asynchronous operations.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #7708
- [X] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14962)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel-wpt

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Jan 11, 2017
@jdm
Copy link
Member Author

jdm commented Jan 11, 2017

Gah, the gl-teximage.html failures are back again :(

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #14291) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Jan 11, 2017
@wafflespeanut
Copy link
Contributor

r? @Ms2ger @nox or someone else

@highfive highfive assigned Ms2ger and unassigned wafflespeanut Jan 19, 2017
@jdm
Copy link
Member Author

jdm commented Jan 19, 2017

I'm going to pursue a different design after discussion with @jrmuizel.

@jdm jdm closed this Jan 19, 2017
@jdm jdm reopened this Jan 24, 2017
@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Jan 24, 2017
@jdm
Copy link
Member Author

jdm commented Jan 24, 2017

I changed my mind - the changes I discussed with @jrmuizel are more or less orthogonal to these changes, and this PR is already complex enough as it is.

@jdm
Copy link
Member Author

jdm commented Jan 24, 2017

@Ms2ger Since nobody volunteered in response to my mailing list post, I choose you to review!

@jdm jdm removed the S-needs-rebase There are merge conflict errors. label Jan 24, 2017
Copy link
Contributor

@Ms2ger Ms2ger left a comment

Choose a reason for hiding this comment

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

Some comments from looking at the first commit.

/// Interface to the font cache thread.
pub font_cache_thread: Mutex<FontCacheThread>,

/// A cache of WebRender image info.
pub webrender_image_cache: Arc<RwLock<HashMap<(ServoUrl, UsePlaceholder),
WebRenderImageInfo,
BuildHasherDefault<FnvHasher>>>>,

/// A list of in-progress image loads to be shared with the script thread.
/// A None value means that this layout was not initiated by the script thread.
Copy link
Contributor

Choose a reason for hiding this comment

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

You're only wrapping this in Option later on.


impl Drop for SharedLayoutContext {
fn drop(&mut self) {
assert!(self.pending_images.lock().unwrap().is_empty());
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's gate this on panicking?

@@ -203,19 +171,31 @@ impl SharedLayoutContext {
// Image failed to load, so just return nothing
Err(ImageState::LoadError) => None,
// Not yet requested, async mode - request image or metadata from the cache
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove references to "async mode"

CompletedLoad {
image_response: image_response,
id: id,
}
}
}

/// Stores information to notify a client when the state
/// of an image changes.
struct ImageListener {
Copy link
Contributor

Choose a reason for hiding this comment

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

This struct should be removed, either in this PR or a followup

use servo_url::ServoUrl;

pub fn request_image_from_cache(window: &Window, url: ServoUrl) -> ImageResponse {
let image_cache = window.image_cache_thread();
panic!()
/*let image_cache = window.image_cache_thread();
Copy link
Contributor

Choose a reason for hiding this comment

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

:/

Copy link
Member Author

Choose a reason for hiding this comment

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

This gets addressed in a later commit.


let context = Arc::new(Mutex::new(ImageContext {
elem: trusted_node,
data: vec!(),
Copy link
Contributor

Choose a reason for hiding this comment

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

vec![]

let id = image.id;
let js_runtime = self.js_runtime.borrow();
let js_runtime = js_runtime.as_ref().unwrap();
let node = from_untrusted_node_address(js_runtime.rt(), image.node);
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we know these are still valid?

Copy link
Member Author

Choose a reason for hiding this comment

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

Script is paused during layout, and this executes immediately after the script thread resumes.


let mut images = self.pending_layout_images.borrow_mut();
let nodes = images.entry(id).or_insert(vec![]);
if nodes.iter().find(|n| **n == JS::from_ref(&*node)).is_none() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why create a JS here?

let window = document.window();
let image_cache = window.image_cache_thread();
image_cache.store_complete_image_bytes(self.id, self.data.clone());
document.finish_load(LoadType::Image(self.url.clone()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Do layout-only images block onload?

Copy link
Member Author

Choose a reason for hiding this comment

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

That gets addressed in a later commit.

}));

let document = document_from_node(node);
let window = window_from_node(node);
Copy link
Contributor

Choose a reason for hiding this comment

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

document.window()

@@ -16,12 +16,17 @@
}
}

var t = async_test();
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrap everything in the async_test, please.

@@ -495,51 +479,76 @@ impl ImageCache {

/// Return a completed image if it exists, or None if there is no complete load
/// of the complete load is not fully decoded or is unavailable.
fn get_completed_image_if_available(&self,
url: &ServoUrl,
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation

@@ -495,51 +479,76 @@ impl ImageCache {

/// Return a completed image if it exists, or None if there is no complete load
/// of the complete load is not fully decoded or is unavailable.
Copy link
Contributor

Choose a reason for hiding this comment

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

Pre-existing: of -> or?

placeholder: UsePlaceholder)
-> Option<Result<ImageOrMetadataAvailable, ImageState>> {
self.completed_loads.get(url).map(|completed_load| {
match (completed_load.image_response.clone(), placeholder) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's only clone if needed?


LoadBlocker::terminate(&mut self.current_request.borrow_mut().blocker);
let document = document_from_node(self);
self.current_request.borrow_mut().blocker =
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not borrow this as many times as possible :)

@jdm
Copy link
Member Author

jdm commented Feb 1, 2017

@glennw Want to look at the image cache changes in this PR?

@bors-servo
Copy link
Contributor

⌛ Testing commit e99cba6 with merge 1e6810f...

bors-servo pushed a commit that referenced this pull request Feb 22, 2017
Remove network requests from image cache thread

The design of initiating network requests from the image cache thread was simple, but it makes it difficult to implement image loading that conforms to the HTML specification. These changes make the implementation of HTMLImageElement responsible for network requests for `<img>` elements, and CSS-based images (background-image, bullets, etc.) are requested by the script thread to ensure that the layout thread does not attempt to retain unsafe pointers to DOM nodes during asynchronous operations.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #7708
- [X] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14962)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel-wpt

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Feb 22, 2017
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Feb 23, 2017
@jdm
Copy link
Member Author

jdm commented Feb 23, 2017

@bors-servo: r=Ms2ger,glennw,emilio

@bors-servo
Copy link
Contributor

📌 Commit 5af2603 has been approved by Ms2ger,glennw,emilio

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Feb 23, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit 5af2603 with merge 854d720...

bors-servo pushed a commit that referenced this pull request Feb 23, 2017
Remove network requests from image cache thread

The design of initiating network requests from the image cache thread was simple, but it makes it difficult to implement image loading that conforms to the HTML specification. These changes make the implementation of HTMLImageElement responsible for network requests for `<img>` elements, and CSS-based images (background-image, bullets, etc.) are requested by the script thread to ensure that the layout thread does not attempt to retain unsafe pointers to DOM nodes during asynchronous operations.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #7708
- [X] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14962)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
Approved by: Ms2ger,glennw,emilio
Pushing 854d720 to master...

@bors-servo bors-servo merged commit 5af2603 into servo:master Feb 23, 2017
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Feb 23, 2017
jdm added a commit to web-platform-tests/wpt that referenced this pull request Apr 11, 2017
jdm added a commit to web-platform-tests/wpt that referenced this pull request Apr 17, 2017
bors-servo pushed a commit that referenced this pull request Apr 21, 2017
Revert unnecessary changes to canvas image drawing tests.

These changes were made as part of #14962 under the assumption that images not in the document do not delay the load event. That assumption is incorrect.

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16492)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move HTTP requests out of image cache logic
8 participants