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
Merged

Conversation

@jdm
Copy link
Member

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.


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

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

highfive commented Jan 11, 2017

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
@jdm jdm force-pushed the jdm:image_script_load branch from 63e9dbc to 15905b2 Jan 11, 2017
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 11, 2017

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

@jdm jdm force-pushed the jdm:image_script_load branch from 15905b2 to f2cda69 Jan 11, 2017
@jdm

This comment has been minimized.

Copy link
Member Author

jdm commented Jan 11, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 11, 2017

⌛️ Trying commit f2cda69 with merge 4f42fe3...

bors-servo added 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

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 11, 2017

💔 Test failed - linux-rel-wpt

@jdm

This comment has been minimized.

Copy link
Member Author

jdm commented Jan 11, 2017

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

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 11, 2017

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

@wafflespeanut

This comment has been minimized.

Copy link
Member

wafflespeanut commented Jan 19, 2017

r? @Ms2ger @nox or someone else

@highfive highfive assigned Ms2ger and unassigned wafflespeanut Jan 19, 2017
@jdm

This comment has been minimized.

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
@jdm jdm force-pushed the jdm:image_script_load branch from f2cda69 to e5f95a4 Jan 24, 2017
@jdm

This comment has been minimized.

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

This comment has been minimized.

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 label Jan 24, 2017
Copy link
Contributor

Ms2ger left a comment

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.

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Jan 25, 2017

Contributor

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());

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Jan 25, 2017

Contributor

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

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Jan 25, 2017

Contributor

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 {

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Jan 25, 2017

Contributor

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();

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Jan 25, 2017

Contributor

:/

This comment has been minimized.

Copy link
@jdm

jdm Jan 25, 2017

Author Member

This gets addressed in a later commit.


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

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Jan 25, 2017

Contributor

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);

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Jan 25, 2017

Contributor

How do we know these are still valid?

This comment has been minimized.

Copy link
@jdm

jdm Jan 25, 2017

Author Member

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() {

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Jan 25, 2017

Contributor

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()));

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Jan 25, 2017

Contributor

Do layout-only images block onload?

This comment has been minimized.

Copy link
@jdm

jdm Jan 25, 2017

Author Member

That gets addressed in a later commit.

}));

let document = document_from_node(node);
let window = window_from_node(node);

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Jan 25, 2017

Contributor

document.window()

@@ -16,12 +16,17 @@
}
}
var t = async_test();

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Jan 26, 2017

Contributor

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,

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Jan 26, 2017

Contributor

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.

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Jan 26, 2017

Contributor

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) {

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Jan 26, 2017

Contributor

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 =

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Jan 26, 2017

Contributor

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

@jdm

This comment has been minimized.

Copy link
Member Author

jdm commented Feb 1, 2017

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

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Feb 22, 2017

⌛️ Testing commit e99cba6 with merge 1e6810f...

bors-servo added 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

This comment has been minimized.

Copy link
Contributor

bors-servo commented Feb 22, 2017

💔 Test failed - linux-rel-wpt

@jdm

This comment has been minimized.

Copy link
Member Author

jdm commented Feb 23, 2017

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

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Feb 23, 2017

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

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Feb 23, 2017

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

bors-servo added 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

This comment has been minimized.

Copy link
Contributor

bors-servo commented Feb 23, 2017

☀️ 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
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
jdm added a commit to web-platform-tests/wpt that referenced this pull request Apr 11, 2017
…ronous drawImage tests.

Upstreamed from servo/servo#14962 [ci skip]
jdm added a commit to web-platform-tests/wpt that referenced this pull request Apr 17, 2017
Upstreamed from servo/servo#14962 [ci skip]
bors-servo added 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
Projects
None yet
8 participants
You can’t perform that action at this time.