Handle cancelAnimationFrame() when called within a requestAnimationFrame() callback

[Manishearth]

Spec change: immersive-web/webxr#1005 (comment)

Relevant code:

servo/components/script/dom/xrsession.rs

Lines 398 to 414 in b60c70b

	let mut callbacks = mem::replace(&mut *self.raf_callback_list.borrow_mut(), vec![]);
	let start = self.global().as_window().get_navigation_start();
	let time = reduce_timing_resolution((frame.time_ns - start).to_ms());
	let frame = XRFrame::new(&self.global(), self, frame);
	// Step 6,7
	frame.set_active(true);
	frame.set_animation_frame(true);
	// Step 8
	self.outside_raf.set(false);
	for (_, callback) in callbacks.drain(..) {
	if let Some(callback) = callback {
	let _ = callback.Call__(time, &frame, ExceptionHandling::Report);
	}
	}
	self.outside_raf.set(true);

Currently we take self.raf_callback_list out and put it on the stack, and then iterate through it.

Instead, we should have a second current_raf_callback_list list on the object, move the vector there, and use indexed iteration. Then, in CancelAnimationFrame, we check both lists.

[highfive]

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in Matrix.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

[tigleym]

I'd like to give this a try. @highfive assign me

[highfive]

Hey @tigleym! Thanks for your interest in working on this issue. It's now assigned to you!

[tigleym]

@Manishearth I'll need some help finding a good way to execute each callback for current_raf_callback_list while borrowing the value wrapped with it. Right now the CancelAnimationFrame callback will panic because we also try to mutably borrow current_raf_callback_list there.

So far, this is what I'm trying to do:

Move the vector from self.raf_callback_list to self.current_raf_callback_list using mem::replace

mem::replace(
  &mut *self.current_raf_callback_list.borrow_mut(),
  mem::replace(&mut *self.raf_callback_list.borrow_mut(), vec![])
 );

Iterate over the callbacks stored on self.current_raf_callback_list:

for (_, callback) in self.current_raf_callback_list.borrow().iter() {
  if let Some(callback) = callback {
    let _ = callback.Call__(time, &frame, ExceptionHandling::Report);
   }
}

In CancelAnimationFrame (panic happens here):

let mut list = self.raf_callback_list.borrow_mut();

// Find the entry in raf_callback_list, otherwise try finding it in current_raf_callback_list
if let Some(pair) = list.iter_mut().find(|pair| pair.0 == frame) {
  pair.1 = None;
} else {
// Panic happens here because current_raf_callback_list is already borrowed.
  if let Some(pair) = self.current_raf_callback_list.borrow_mut().iter_mut().find(|pair| pair.0 == frame) {
    pair.1 = None;
  }
}

Perhaps what I can try doing is putting self.current_raf_callback_list on the stack and iterate that instead, similar to what is previously done with self.raf_callback_list. But I wanted to get your input on this, and if I'm going in the right direction.

[Manishearth]

Perhaps what I can try doing is putting self.current_raf_callback_list on the stack and iterate that instead,

No, because then CancelAnimationFrame can't clear it.

What needs to be done instead is that the loop for (_, callback) in self.current_raf_callback_list.borrow().iter() { should borrow on each frame, using for i in 0..len and doing self.current_raf_callback_list.borrow()[i] in each iteration. We guarantee that the array does not change size during this loop, so this should work.

[tigleym]

@Manishearth thanks for the info! Unfortunately, I don't have a lot of time to work on this right now. So feel free to unassign me if you or someone else would like to finish it off. Thanks!

[huangjiahua]

Can I work on this? @highfive: assign me

[highfive]

Hey @huangjiahua! Thanks for your interest in working on this issue. It's now assigned to you!

[huangjiahua]

@Manishearth This is my approach right now

let len = self.current_raf_callback_list.borrow().len();
for i in 0..len {
    let _ = self.current_raf_callback_list.borrow()[i]
        .1
        .as_ref()
        .and_then(|callback| {
            let _ = callback.Call__(time, &frame, ExceptionHandling::Report);
            Some(())
        });
}

Will this work? And I'm trying to run tests right now, it seems that running all the tests is time consuming for my PC, so is there any subset of tests I should run to check this?

[Manishearth]

Yes. ./mach test-wpt webxr though I don't think there are tests for this yet. You can write one!