Mirrors/servo
1
0
Fork 0
mirror of https://github.com/servo/servo.git synced 2025-06-09 00:53:26 +00:00
Code Issues Projects Releases Packages Wiki Activity

Fix GC borrow hazards triggered by LoadBlocker::terminate (#34122)

Browse source 
* Fix GC borrow hazards triggered by LoadBlocker::terminate

Signed-off-by: taniishkaaa <tanishkasingh2004@gmail.com>

* Fix clippy warnings

Signed-off-by: taniishkaaa <tanishkasingh2004@gmail.com>

* Use borrow_mut()

Signed-off-by: taniishkaaa <tanishkasingh2004@gmail.com>

* Revert to previous code due to crown unrooted error

Signed-off-by: taniishkaaa <tanishkasingh2004@gmail.com>

* Update test expectations

Signed-off-by: taniishkaaa <tanishkasingh2004@gmail.com>

---------

Signed-off-by: taniishkaaa <tanishkasingh2004@gmail.com>
This commit is contained in:
tanishka 2024-11-05 03:57:41 +05:30 • committed by GitHub
parent 072ff302d2
commit cc6163dcdd
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
10 changed files with 34 additions and 40 deletions
Download patch file Download diff file Expand all files Collapse all files

10
components/script/document_loader.rs
View file

@ -11,6 +11,7 @@ use net_traits::request::RequestBuilder;
use net_traits::{fetch_async, BoxedFetchCallback, ResourceThreads}; use net_traits::{fetch_async, BoxedFetchCallback, ResourceThreads};
use servo_url::ServoUrl; use servo_url::ServoUrl;
use crate::dom::bindings::cell::DomRefCell;
use crate::dom::bindings::root::Dom; use crate::dom::bindings::root::Dom;
use crate::dom::document::Document; use crate::dom::document::Document;
use crate::fetch::FetchCanceller; use crate::fetch::FetchCanceller;
@ -49,11 +50,12 @@ impl LoadBlocker {
} }
/// Remove this load from the associated document's list of blocking loads. /// Remove this load from the associated document's list of blocking loads.
pub fn terminate(blocker: &mut Option<LoadBlocker>, can_gc: CanGc) { pub fn terminate(blocker: &DomRefCell<Option<LoadBlocker>>, can_gc: CanGc) {
if let Some(this) = blocker.as_mut() { if let Some(this) = blocker.borrow().as_ref() {
this.doc.finish_load(this.load.take().unwrap(), can_gc); let load_data = this.load.clone().unwrap();
this.doc.finish_load(load_data, can_gc);
} }
*blocker = None; *blocker.borrow_mut() = None;
} }
} }

16
components/script/dom/htmliframeelement.rs
View file

@ -146,10 +146,10 @@ impl HTMLIFrameElement {
let document = document_from_node(self); let document = document_from_node(self);
{ {
let mut load_blocker = self.load_blocker.borrow_mut(); let load_blocker = &self.load_blocker;
// Any oustanding load is finished from the point of view of the blocked // Any oustanding load is finished from the point of view of the blocked
// document; the new navigation will continue blocking it. // document; the new navigation will continue blocking it.
LoadBlocker::terminate(&mut load_blocker, can_gc); LoadBlocker::terminate(load_blocker, can_gc);
} }
if load_data.url.scheme() == "javascript" { if load_data.url.scheme() == "javascript" {
@ -422,8 +422,8 @@ impl HTMLIFrameElement {
// Only terminate the load blocker if the pipeline id was updated due to a traversal. // Only terminate the load blocker if the pipeline id was updated due to a traversal.
// The load blocker will be terminated for a navigation in iframe_load_event_steps. // The load blocker will be terminated for a navigation in iframe_load_event_steps.
if reason == UpdatePipelineIdReason::Traversal { if reason == UpdatePipelineIdReason::Traversal {
let mut blocker = self.load_blocker.borrow_mut(); let blocker = &self.load_blocker;
LoadBlocker::terminate(&mut blocker, can_gc); LoadBlocker::terminate(blocker, can_gc);
} }
self.upcast::<Node>().dirty(NodeDamage::OtherNodeDamage); self.upcast::<Node>().dirty(NodeDamage::OtherNodeDamage);
@ -507,8 +507,8 @@ impl HTMLIFrameElement {
self.upcast::<EventTarget>() self.upcast::<EventTarget>()
.fire_event(atom!("load"), can_gc); .fire_event(atom!("load"), can_gc);
let mut blocker = self.load_blocker.borrow_mut(); let blocker = &self.load_blocker;
LoadBlocker::terminate(&mut blocker, can_gc); LoadBlocker::terminate(blocker, can_gc);
// TODO Step 5 - unset child document `mut iframe load` flag // TODO Step 5 - unset child document `mut iframe load` flag
} }
@ -740,8 +740,8 @@ impl VirtualMethods for HTMLIFrameElement {
fn unbind_from_tree(&self, context: &UnbindContext) { fn unbind_from_tree(&self, context: &UnbindContext) {
self.super_type().unwrap().unbind_from_tree(context); self.super_type().unwrap().unbind_from_tree(context);
let mut blocker = self.load_blocker.borrow_mut(); let blocker = &self.load_blocker;
LoadBlocker::terminate(&mut blocker, CanGc::note()); LoadBlocker::terminate(blocker, CanGc::note());
// https://html.spec.whatwg.org/multipage/#a-browsing-context-is-discarded // https://html.spec.whatwg.org/multipage/#a-browsing-context-is-discarded
let window = window_from_node(self); let window = window_from_node(self);

17
components/script/dom/htmlimageelement.rs
View file

@ -154,7 +154,7 @@ struct ImageRequest {
#[no_trace] #[no_trace]
parsed_url: Option<ServoUrl>, parsed_url: Option<ServoUrl>,
source_url: Option<USVString>, source_url: Option<USVString>,
blocker: Option<LoadBlocker>, blocker: DomRefCell<Option<LoadBlocker>>,
#[ignore_malloc_size_of = "Arc"] #[ignore_malloc_size_of = "Arc"]
#[no_trace] #[no_trace]
image: Option<Arc<Image>>, image: Option<Arc<Image>>,
@ -431,7 +431,7 @@ impl HTMLImageElement {
self.current_request.borrow_mut().final_url = Some(url); self.current_request.borrow_mut().final_url = Some(url);
self.current_request.borrow_mut().image = Some(image); self.current_request.borrow_mut().image = Some(image);
self.current_request.borrow_mut().state = State::CompletelyAvailable; self.current_request.borrow_mut().state = State::CompletelyAvailable;
LoadBlocker::terminate(&mut self.current_request.borrow_mut().blocker, can_gc); LoadBlocker::terminate(&self.current_request.borrow().blocker, can_gc);
// Mark the node dirty // Mark the node dirty
self.upcast::<Node>().dirty(NodeDamage::OtherNodeDamage); self.upcast::<Node>().dirty(NodeDamage::OtherNodeDamage);
self.resolve_image_decode_promises(); self.resolve_image_decode_promises();
@ -537,7 +537,7 @@ impl HTMLImageElement {
ImageRequestPhase::Current => self.current_request.borrow_mut(), ImageRequestPhase::Current => self.current_request.borrow_mut(),
ImageRequestPhase::Pending => self.pending_request.borrow_mut(), ImageRequestPhase::Pending => self.pending_request.borrow_mut(),
}; };
LoadBlocker::terminate(&mut request.blocker, can_gc); LoadBlocker::terminate(&request.blocker, can_gc);
request.state = state; request.state = state;
request.image = None; request.image = None;
request.metadata = None; request.metadata = None;
@ -821,8 +821,9 @@ impl HTMLImageElement {
request.image = None; request.image = None;
request.metadata = None; request.metadata = None;
let document = document_from_node(self); let document = document_from_node(self);
LoadBlocker::terminate(&mut request.blocker, can_gc); LoadBlocker::terminate(&request.blocker, can_gc);
request.blocker = Some(LoadBlocker::new(&document, LoadType::Image(url.clone()))); *request.blocker.borrow_mut() =
Some(LoadBlocker::new(&document, LoadType::Image(url.clone())));
} }
/// Step 13-17 of html.spec.whatwg.org/multipage/#update-the-image-data /// Step 13-17 of html.spec.whatwg.org/multipage/#update-the-image-data
@ -853,7 +854,7 @@ impl HTMLImageElement {
// Step 15 abort pending request // Step 15 abort pending request
pending_request.image = None; pending_request.image = None;
pending_request.parsed_url = None; pending_request.parsed_url = None;
LoadBlocker::terminate(&mut pending_request.blocker, can_gc); LoadBlocker::terminate(&pending_request.blocker, can_gc);
// TODO: queue a task to restart animation, if restart-animation is set // TODO: queue a task to restart animation, if restart-animation is set
return; return;
} }
@ -1311,7 +1312,7 @@ impl HTMLImageElement {
source_url: None, source_url: None,
image: None, image: None,
metadata: None, metadata: None,
blocker: None, blocker: DomRefCell::new(None),
final_url: None, final_url: None,
current_pixel_density: None, current_pixel_density: None,
}), }),
@ -1321,7 +1322,7 @@ impl HTMLImageElement {
source_url: None, source_url: None,
image: None, image: None,
metadata: None, metadata: None,
blocker: None, blocker: DomRefCell::new(None),
final_url: None, final_url: None,
current_pixel_density: None, current_pixel_density: None,
}), }),

11
components/script/dom/htmlmediaelement.rs
View file

@ -511,11 +511,12 @@ impl HTMLMediaElement {
/// ///
/// <https://html.spec.whatwg.org/multipage/#delaying-the-load-event-flag> /// <https://html.spec.whatwg.org/multipage/#delaying-the-load-event-flag>
pub fn delay_load_event(&self, delay: bool, can_gc: CanGc) { pub fn delay_load_event(&self, delay: bool, can_gc: CanGc) {
let mut blocker = self.delaying_the_load_event_flag.borrow_mut(); let blocker = &self.delaying_the_load_event_flag;
if delay && blocker.is_none() { if delay && blocker.borrow().is_none() {
*blocker = Some(LoadBlocker::new(&document_from_node(self), LoadType::Media)); *blocker.borrow_mut() =
} else if !delay && blocker.is_some() { Some(LoadBlocker::new(&document_from_node(self), LoadType::Media));
LoadBlocker::terminate(&mut blocker, can_gc); } else if !delay && blocker.borrow().is_some() {
LoadBlocker::terminate(blocker, can_gc);
} }
} }

10
components/script/dom/htmlvideoelement.rs
View file

@ -225,9 +225,9 @@ impl HTMLVideoElement {
// <video poster="poster.png"></video> // <video poster="poster.png"></video>
// (which triggers no media load algorithm unless a explicit call to .load() is done) // (which triggers no media load algorithm unless a explicit call to .load() is done)
// will block the document's load event forever. // will block the document's load event forever.
let mut blocker = self.load_blocker.borrow_mut(); let blocker = &self.load_blocker;
LoadBlocker::terminate(&mut blocker, can_gc); LoadBlocker::terminate(blocker, can_gc);
*blocker = Some(LoadBlocker::new( *blocker.borrow_mut() = Some(LoadBlocker::new(
&document_from_node(self), &document_from_node(self),
LoadType::Image(poster_url.clone()), LoadType::Image(poster_url.clone()),
)); ));
@ -309,13 +309,13 @@ impl ImageCacheListener for HTMLVideoElement {
ImageResponse::Loaded(image, url) => { ImageResponse::Loaded(image, url) => {
debug!("Loaded poster image for video element: {:?}", url); debug!("Loaded poster image for video element: {:?}", url);
self.htmlmediaelement.process_poster_image_loaded(image); self.htmlmediaelement.process_poster_image_loaded(image);
LoadBlocker::terminate(&mut self.load_blocker.borrow_mut(), can_gc); LoadBlocker::terminate(&self.load_blocker, can_gc);
}, },
ImageResponse::MetadataLoaded(..) => {}, ImageResponse::MetadataLoaded(..) => {},
// The image cache may have loaded a placeholder for an invalid poster url // The image cache may have loaded a placeholder for an invalid poster url
ImageResponse::PlaceholderLoaded(..) | ImageResponse::None => { ImageResponse::PlaceholderLoaded(..) | ImageResponse::None => {
// A failed load should unblock the document load. // A failed load should unblock the document load.
LoadBlocker::terminate(&mut self.load_blocker.borrow_mut(), can_gc); LoadBlocker::terminate(&self.load_blocker, can_gc);
}, },
} }
} }

2
tests/wpt/meta/dom/ranges/Range-cloneContents.html.ini vendored
View file

@ -1,2 +0,0 @@
[Range-cloneContents.html]
expected: ERROR

2
tests/wpt/meta/dom/ranges/Range-deleteContents.html.ini vendored
View file

@ -1,2 +0,0 @@
[Range-deleteContents.html]
expected: ERROR

2
tests/wpt/meta/dom/ranges/Range-extractContents.html.ini vendored
View file

@ -1,2 +0,0 @@
[Range-extractContents.html]
expected: ERROR

2
tests/wpt/meta/dom/ranges/Range-insertNode.html.ini vendored
View file

@ -1,2 +0,0 @@
[Range-insertNode.html]
expected: ERROR

2
tests/wpt/meta/dom/ranges/Range-surroundContents.html.ini vendored
View file

@ -1,2 +0,0 @@
[Range-surroundContents.html]
expected: ERROR