Repository: servo/servo (language: python)

Issue:
LoadBlocker::terminate is a common trigger for GC borrow hazards 
The load blocker API requires a mutable borrow, which usually means the caller needs to mutably borrow a RefCell to obtain it. Since terminate also can trigger a GC in certain infrequent cases, this is both risky and over-cautious.


Discussion / hints:
I think we need to change [LoadBlocker::terminate](https://github.com/servo/servo/blob/a3fda3893f1c48ba857e1fa9da0102632db66119/components/script/document_loader.rs#L52) from accepting `&mut Option<LoadBlocker>` to `&DomRefCell<Option<LoadBlocker>>`. This will allow us to extract the actual load blocker value before using it to call [finish_load](https://github.com/servo/servo/blob/a3fda3893f1c48ba857e1fa9da0102632db66119/components/script/document_loader.rs#L54), avoiding the need to keep the mutable borrow active during that call.
An alternative: add a RefCell wrapper to https://github.com/servo/servo/blob/main/components/script/document_loader.rs#L38 so that we no longer need a mutable borrow at all in LoadBlocker::terminate. I think I still prefer the previous solution, though.
I can work on this!
are there any tests for this?
`./mach test-wpt tests/wpt/tests/html/semantics/embedded-content/the-img-element` should be good!

Fix patch:
diff --git a/components/script/document_loader.rs b/components/script/document_loader.rs
index dd9d74403ef73..5d07cea093050 100644
--- a/components/script/document_loader.rs
+++ b/components/script/document_loader.rs
@@ -11,6 +11,7 @@ use net_traits::request::RequestBuilder;
 use net_traits::{fetch_async, BoxedFetchCallback, ResourceThreads};
 use servo_url::ServoUrl;
 
+use crate::dom::bindings::cell::DomRefCell;
 use crate::dom::bindings::root::Dom;
 use crate::dom::document::Document;
 use crate::fetch::FetchCanceller;
@@ -49,11 +50,12 @@ impl LoadBlocker {
     }
 
     /// Remove this load from the associated document's list of blocking loads.
-    pub fn terminate(blocker: &mut Option<LoadBlocker>, can_gc: CanGc) {
-        if let Some(this) = blocker.as_mut() {
-            this.doc.finish_load(this.load.take().unwrap(), can_gc);
+    pub fn terminate(blocker: &DomRefCell<Option<LoadBlocker>>, can_gc: CanGc) {
+        if let Some(this) = blocker.borrow().as_ref() {
+            let load_data = this.load.clone().unwrap();
+            this.doc.finish_load(load_data, can_gc);
         }
-        *blocker = None;
+        *blocker.borrow_mut() = None;
     }
 }
 
diff --git a/components/script/dom/htmliframeelement.rs b/components/script/dom/htmliframeelement.rs
index bf0f6de5b0d86..d26cf6297f2b4 100644
--- a/components/script/dom/htmliframeelement.rs
+++ b/components/script/dom/htmliframeelement.rs
@@ -146,10 +146,10 @@ impl HTMLIFrameElement {
         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
             // 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" {
@@ -422,8 +422,8 @@ impl HTMLIFrameElement {
         // 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.
         if reason == UpdatePipelineIdReason::Traversal {
-            let mut blocker = self.load_blocker.borrow_mut();
-            LoadBlocker::terminate(&mut blocker, can_gc);
+            let blocker = &self.load_blocker;
+            LoadBlocker::terminate(blocker, can_gc);
         }
 
         self.upcast::<Node>().dirty(NodeDamage::OtherNodeDamage);
@@ -507,8 +507,8 @@ impl HTMLIFrameElement {
         self.upcast::<EventTarget>()
             .fire_event(atom!("load"), can_gc);
 
-        let mut blocker = self.load_blocker.borrow_mut();
-        LoadBlocker::terminate(&mut blocker, can_gc);
+        let blocker = &self.load_blocker;
+        LoadBlocker::terminate(blocker, can_gc);
 
         // 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) {
         self.super_type().unwrap().unbind_from_tree(context);
 
-        let mut blocker = self.load_blocker.borrow_mut();
-        LoadBlocker::terminate(&mut blocker, CanGc::note());
+        let blocker = &self.load_blocker;
+        LoadBlocker::terminate(blocker, CanGc::note());
 
         // https://html.spec.whatwg.org/multipage/#a-browsing-context-is-discarded
         let window = window_from_node(self);
diff --git a/components/script/dom/htmlimageelement.rs b/components/script/dom/htmlimageelement.rs
index f238d777558da..d26557985e1e3 100644
--- a/components/script/dom/htmlimageelement.rs
+++ b/components/script/dom/htmlimageelement.rs
@@ -154,7 +154,7 @@ struct ImageRequest {
     #[no_trace]
     parsed_url: Option<ServoUrl>,
     source_url: Option<USVString>,
-    blocker: Option<LoadBlocker>,
+    blocker: DomRefCell<Option<LoadBlocker>>,
     #[ignore_malloc_size_of = "Arc"]
     #[no_trace]
     image: Option<Arc<Image>>,
@@ -431,7 +431,7 @@ impl HTMLImageElement {
         self.current_request.borrow_mut().final_url = Some(url);
         self.current_request.borrow_mut().image = Some(image);
         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
         self.upcast::<Node>().dirty(NodeDamage::OtherNodeDamage);
         self.resolve_image_decode_promises();
@@ -537,7 +537,7 @@ impl HTMLImageElement {
             ImageRequestPhase::Current => self.current_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.image = None;
         request.metadata = None;
@@ -821,8 +821,9 @@ impl HTMLImageElement {
         request.image = None;
         request.metadata = None;
         let document = document_from_node(self);
-        LoadBlocker::terminate(&mut request.blocker, can_gc);
-        request.blocker = Some(LoadBlocker::new(&document, LoadType::Image(url.clone())));
+        LoadBlocker::terminate(&request.blocker, can_gc);
+        *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
@@ -853,7 +854,7 @@ impl HTMLImageElement {
                             // Step 15 abort pending request
                             pending_request.image = 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
                             return;
                         }
@@ -1311,7 +1312,7 @@ impl HTMLImageElement {
                 source_url: None,
                 image: None,
                 metadata: None,
-                blocker: None,
+                blocker: DomRefCell::new(None),
                 final_url: None,
                 current_pixel_density: None,
             }),
@@ -1321,7 +1322,7 @@ impl HTMLImageElement {
                 source_url: None,
                 image: None,
                 metadata: None,
-                blocker: None,
+                blocker: DomRefCell::new(None),
                 final_url: None,
                 current_pixel_density: None,
             }),
diff --git a/components/script/dom/htmlmediaelement.rs b/components/script/dom/htmlmediaelement.rs
index 8a9d095dcf727..e76b57e9b8b60 100644
--- a/components/script/dom/htmlmediaelement.rs
+++ b/components/script/dom/htmlmediaelement.rs
@@ -511,11 +511,12 @@ impl HTMLMediaElement {
     ///
     /// <https://html.spec.whatwg.org/multipage/#delaying-the-load-event-flag>
     pub fn delay_load_event(&self, delay: bool, can_gc: CanGc) {
-        let mut blocker = self.delaying_the_load_event_flag.borrow_mut();
-        if delay && blocker.is_none() {
-            *blocker = Some(LoadBlocker::new(&document_from_node(self), LoadType::Media));
-        } else if !delay && blocker.is_some() {
-            LoadBlocker::terminate(&mut blocker, can_gc);
+        let blocker = &self.delaying_the_load_event_flag;
+        if delay && blocker.borrow().is_none() {
+            *blocker.borrow_mut() =
+                Some(LoadBlocker::new(&document_from_node(self), LoadType::Media));
+        } else if !delay && blocker.borrow().is_some() {
+            LoadBlocker::terminate(blocker, can_gc);
         }
     }
 
diff --git a/components/script/dom/htmlvideoelement.rs b/components/script/dom/htmlvideoelement.rs
index 24614f5b8a0c9..58981651a0e6c 100644
--- a/components/script/dom/htmlvideoelement.rs
+++ b/components/script/dom/htmlvideoelement.rs
@@ -225,9 +225,9 @@ impl HTMLVideoElement {
         // <video poster="poster.png"></video>
         // (which triggers no media load algorithm unless a explicit call to .load() is done)
         // will block the document's load event forever.
-        let mut blocker = self.load_blocker.borrow_mut();
-        LoadBlocker::terminate(&mut blocker, can_gc);
-        *blocker = Some(LoadBlocker::new(
+        let blocker = &self.load_blocker;
+        LoadBlocker::terminate(blocker, can_gc);
+        *blocker.borrow_mut() = Some(LoadBlocker::new(
             &document_from_node(self),
             LoadType::Image(poster_url.clone()),
         ));
@@ -309,13 +309,13 @@ impl ImageCacheListener for HTMLVideoElement {
             ImageResponse::Loaded(image, url) => {
                 debug!("Loaded poster image for video element: {:?}", url);
                 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(..) => {},
             // The image cache may have loaded a placeholder for an invalid poster url
             ImageResponse::PlaceholderLoaded(..) | ImageResponse::None => {
                 // 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);
             },
         }
     }


Test patch:
diff --git a/tests/wpt/meta/dom/ranges/Range-cloneContents.html.ini b/tests/wpt/meta/dom/ranges/Range-cloneContents.html.ini
deleted file mode 100644
index e9dec4e7d3fd1..0000000000000
--- a/tests/wpt/meta/dom/ranges/Range-cloneContents.html.ini
+++ /dev/null
@@ -1,2 +0,0 @@
-[Range-cloneContents.html]
-  expected: ERROR
diff --git a/tests/wpt/meta/dom/ranges/Range-deleteContents.html.ini b/tests/wpt/meta/dom/ranges/Range-deleteContents.html.ini
deleted file mode 100644
index fb9580441139c..0000000000000
--- a/tests/wpt/meta/dom/ranges/Range-deleteContents.html.ini
+++ /dev/null
@@ -1,2 +0,0 @@
-[Range-deleteContents.html]
-  expected: ERROR
diff --git a/tests/wpt/meta/dom/ranges/Range-extractContents.html.ini b/tests/wpt/meta/dom/ranges/Range-extractContents.html.ini
deleted file mode 100644
index 5ecae010acf49..0000000000000
--- a/tests/wpt/meta/dom/ranges/Range-extractContents.html.ini
+++ /dev/null
@@ -1,2 +0,0 @@
-[Range-extractContents.html]
-  expected: ERROR
diff --git a/tests/wpt/meta/dom/ranges/Range-insertNode.html.ini b/tests/wpt/meta/dom/ranges/Range-insertNode.html.ini
deleted file mode 100644
index ca10364a1526a..0000000000000
--- a/tests/wpt/meta/dom/ranges/Range-insertNode.html.ini
+++ /dev/null
@@ -1,2 +0,0 @@
-[Range-insertNode.html]
-  expected: ERROR
diff --git a/tests/wpt/meta/dom/ranges/Range-surroundContents.html.ini b/tests/wpt/meta/dom/ranges/Range-surroundContents.html.ini
deleted file mode 100644
index 5d76192b3444b..0000000000000
--- a/tests/wpt/meta/dom/ranges/Range-surroundContents.html.ini
+++ /dev/null
@@ -1,2 +0,0 @@
-[Range-surroundContents.html]
-  expected: ERROR