Fix behavior when setting repeatCount, and improve tests

core/src/avm1/globals.rs

@@ -296,7 +296,7 @@ pub fn create_timer<'gc>(
         Some(Value::Object(o)) if o.as_executable().is_some() => (
             TimerCallback::Avm1Function {
                 func: *o,
-                params: args.get(2..).map(|s| s.to_vec()).unwrap_or_default(),
+                params: args.get(2..).unwrap_or_default().to_vec(),
             },
             args.get(1),
         ),

core/src/avm2/globals/flash/utils/Timer.as

@@ -43,6 +43,9 @@ package flash.utils {
 
 		public function set repeatCount(value:int): void {
 			this._repeatCount = value;
+			if (this._repeatCount != 0 && this._repeatCount <= this._currentCount) {
+				this.stop();
+			}
 		}
 
 		public function get running(): Boolean {

core/src/timer.rs

@@ -83,6 +83,7 @@ impl<'gc> Timers<'gc> {
 
             // TODO: Can we avoid these clones?
             let callback = timer.callback.clone();
+            let expected_id = timer.id;
 
             let cancel_timer = match callback {
                 TimerCallback::Avm1Function { func, params } => {
@@ -119,13 +120,22 @@ impl<'gc> Timers<'gc> {
             crate::player::Player::run_actions(&mut activation.context);
 
             let mut timer = activation.context.timers.peek_mut().unwrap();
+            // Our timer should still be on the top of the heap.
+            // The only way that this could fail is the timer callback
+            // added a new callback with an *earlier* tick time than our
+            // current one. Our current timer has a 'tick_time' less than
+            // 'cur_time', so this could only happen if a new timer was
+            // added with a negative interval (which is not allowed).
+            assert_eq!(
+                timer.id, expected_id,
+                "Running timer callback created timer in the past!"
+            );
             if timer.is_timeout || cancel_timer {
                 // Timeouts only fire once.
                 drop(timer);
                 activation.context.timers.pop();
             } else {
                 // Reset setInterval timers. `peek_mut` re-sorts the timer in the priority queue.
-                //timer.tick_time = new_cur_time.wrapping_add(timer.interval);
                 timer.tick_time = timer.tick_time.wrapping_add(timer.interval);
             }
         }

tests/tests/regression_tests.rs

@@ -49,7 +49,7 @@ macro_rules! val_or_false {
 // If 'img' is true, then we will render an image of the final frame
 // of the SWF, and compare it against a reference image on disk.
 macro_rules! swf_tests {
-    ($($(#[$attr:meta])* ($name:ident, $path:expr, $num_frames:literal $(, img = $img:literal)? ),)*) => {
+    ($($(#[$attr:meta])* ($name:ident, $path:expr, $num_frames:literal $(, img = $img:literal)? $(, frame_time_sleep = $frame_time_sleep:literal)? ),)*) => {
         $(
         #[test]
         $(#[$attr])*
@@ -60,7 +60,8 @@ macro_rules! swf_tests {
                 $num_frames,
                 concat!("tests/swfs/", $path, "/input.json"),
                 concat!("tests/swfs/", $path, "/output.txt"),
-                val_or_false!($($img)?)
+                val_or_false!($($img)?),
+                val_or_false!($($frame_time_sleep)?),
             )
         }
         )*
@@ -740,7 +741,7 @@ swf_tests! {
     (textfield_variable, "avm1/textfield_variable", 8),
     (this_scoping, "avm1/this_scoping", 1),
     (timeline_function_def, "avm1/timeline_function_def", 3),
-    (avm2_timer, "avm2/timer", 140),
+    (avm2_timer, "avm2/timer", 280, frame_time_sleep = true),
     (timer_run_actions, "avm1/timer_run_actions", 1),
     (trace, "avm1/trace", 1),
     (transform, "avm1/transform", 1),
@@ -882,6 +883,7 @@ fn external_interface_avm1() -> Result<(), Error> {
             Ok(())
         },
         false,
+        false,
     )
 }
 
@@ -929,6 +931,7 @@ fn external_interface_avm2() -> Result<(), Error> {
             Ok(())
         },
         false,
+        false,
     )
 }
 
@@ -954,6 +957,7 @@ fn shared_object_avm1() -> Result<(), Error> {
             Ok(())
         },
         false,
+        false,
     )?;
 
     // Verify that the flash cookie matches the expected one
@@ -979,6 +983,7 @@ fn shared_object_avm1() -> Result<(), Error> {
         },
         |_player| Ok(()),
         false,
+        false,
     )?;
 
     Ok(())
@@ -1001,6 +1006,7 @@ fn timeout_avm1() -> Result<(), Error> {
         },
         |_| Ok(()),
         false,
+        false,
     )
 }
 
@@ -1022,6 +1028,7 @@ fn stage_scale_mode() -> Result<(), Error> {
         },
         |_| Ok(()),
         false,
+        false,
     )
 }
 
@@ -1061,6 +1068,7 @@ fn test_swf(
     simulated_input_path: &str,
     expected_output_path: &str,
     check_img: bool,
+    frame_time_sleep: bool,
 ) -> Result<(), Error> {
     test_swf_with_hooks(
         swf_path,
@@ -1070,11 +1078,13 @@ fn test_swf(
         |_| Ok(()),
         |_| Ok(()),
         check_img,
+        frame_time_sleep,
     )
 }
 
 /// Loads an SWF and runs it through the Ruffle core for a number of frames.
 /// Tests that the trace output matches the given expected output.
+#[allow(clippy::too_many_arguments)]
 fn test_swf_with_hooks(
     swf_path: &str,
     num_frames: u32,
@@ -1083,6 +1093,7 @@ fn test_swf_with_hooks(
     before_start: impl FnOnce(Arc<Mutex<Player>>) -> Result<(), Error>,
     before_end: impl FnOnce(Arc<Mutex<Player>>) -> Result<(), Error>,
     check_img: bool,
+    frame_time_sleep: bool,
 ) -> Result<(), Error> {
     let injector =
         InputInjector::from_file(simulated_input_path).unwrap_or_else(|_| InputInjector::empty());
@@ -1100,6 +1111,7 @@ fn test_swf_with_hooks(
         injector,
         before_end,
         check_img,
+        frame_time_sleep,
     )?;
     assert_eq!(
         trace_log, expected_output,
@@ -1128,6 +1140,7 @@ fn test_swf_approx(
         injector,
         |_| Ok(()),
         false,
+        false,
     )?;
     let mut expected_data = std::fs::read_to_string(expected_output_path)?;
 
@@ -1178,6 +1191,7 @@ fn run_swf(
     mut injector: InputInjector,
     before_end: impl FnOnce(Arc<Mutex<Player>>) -> Result<(), Error>,
     mut check_img: bool,
+    frame_time_sleep: bool,
 ) -> Result<String, Error> {
     check_img &= RUN_IMG_TESTS;
 
@@ -1185,6 +1199,7 @@ fn run_swf(
     let mut executor = NullExecutor::new();
     let movie = SwfMovie::from_path(swf_path, None)?;
     let frame_time = 1000.0 / movie.frame_rate().to_f64();
+    let frame_time_duration = Duration::from_millis(frame_time as u64);
     let trace_output = Rc::new(RefCell::new(Vec::new()));
     let mut builder = PlayerBuilder::new();
 
@@ -1224,7 +1239,23 @@ fn run_swf(
 
     before_start(player.clone())?;
 
-    for i in 0..num_frames {
+    for _ in 0..num_frames {
+        // If requested, ensure that the 'expected' amount of
+        // time actually elapses between frames. This is useful for
+        // tests that call 'flash.utils.getTimer()' and use
+        // 'setInterval'/'flash.utils.Timer'
+        //
+        // Note that when Ruffle actually runs frames, we can
+        // execute frames faster than this in order to 'catch up'
+        // if we've fallen behind. However, in order to make regression
+        // tests deterministic, we always call 'update_timers' with
+        // an elapsed time of 'frame_time'. By sleeping for 'frame_time_duration',
+        // we ensure that the result of 'flash.utils.getTimer()' is consistent
+        // with timer execution (timers will see an elapsed time of *at least*
+        // the requested timer interval).
+        if frame_time_sleep {
+            std::thread::sleep(frame_time_duration);
+        }
         player.lock().unwrap().run_frame();
         player.lock().unwrap().update_timers(frame_time);
         executor.run();
@@ -1253,19 +1284,6 @@ fn run_swf(
                 AutomatedEvent::Wait => unreachable!(),
             });
         });
-        if i != num_frames - 1 {
-            // Ensure that the correct amount of time *actually* elapses.
-            // The actual time firing is deterministic (we always advance
-            // the timer clock by `frame_time` milliseconds), but tests
-            // can observe the actual time via `flash.utils.getTimer()`.
-            //
-            // This allows us to write tests that verify that the time
-            // elapsed between timer events is at least the specified
-            // timer delay.
-            if let Some(sleep) = player.lock().unwrap().time_til_next_timer() {
-                std::thread::sleep(Duration::from_millis(sleep as u64));
-            }
-        }
     }
 
     // Render the image to disk

tests/tests/swfs/avm2/timer/Test.as

@@ -1,16 +1,79 @@
-package {
-	public class Test {}
+// We run all of the 'testN' functions in this file twice - once
+// invoked directly from a Timer event handler, and once invoked from an 'ENTER_FRAME'
+// handler. This validates that we can schedule a new timer while inside the event handler
+// for another timer.
+
+package {
+	import flash.display.Stage;
+	import flash.events.Event;
+
+	public class Test {
+		public function run(theStage: Stage) {
+			stage = theStage
+			runNext()
+		}
+	}
 }
 import flash.utils.Timer;
+import flash.events.Event;
 import flash.events.TimerEvent;
 import flash.utils.getTimer;
+import flash.display.Stage;
 
-// Note that this SWF has its framerate set to 60fps, to improve
-// the timer resolution. This seems to help avoid cases where Flash
-// runs timers too *quickly* (maybe due to some explicit 'catch-up' behavior
-// when the VM is running behind?)
+// Note that this SWF has its framerate set to 50fps,
+// which corresponds to one frame every 1000/50 = 20 ms
+// We set all of our timer delays to multiples of 20ms, to
+// try to avoid triggering Flash's "catchup" behavior (where
+// the time between timer events is *less thasn* the requested delay)
+
+// This allows us to have each test assert that the time between events
+// is at *least* the expected delay.
+
+// Add new tests to this array - tests are executed from back to front.
+var allTests = [test9, test8, test7, test6, test5, test4, test3, test2, test1]
+
+// Internal helpers used to run all of our tests twice
+
+// When 'true', directly cal lthe function for our next test.
+// This will result in scheduling a new timer (via Timer.start)
+// from within an existing timer's event handler. After all of the tests
+// have run in this mode, we set 'runDirect = false'
+//
+// When 'false', we will execute the next test from an 'ENTER_FRAME' handler,
+// after the current timer has finished executing.
+var runDirect: Boolean = true;
+
+// A copy of 'allTests' - we pop tests from this, and then restore it after
+// the first full run (with 'runDirect = true'')
+var currentTests = allTests.concat()
+
+var stage: Stage = null
+
+function runNext() {
+	if (currentTests.length == 0) {
+		// Once we've run all of our tests in 'runDirect' mode,
+		// run them again in non-'runDirect' modes
+		if (runDirect) {
+			trace("")
+			trace("Restarting tests with runDirect=false")
+			runDirect = false
+			currentTests = allTests.concat()
+		} else {
+			return
+		}
+	}
+	var nextTest = currentTests.pop()
+	if (runDirect) {
+		nextTest()
+	} else {
+		function runNextFrame(e:*) {
+			stage.removeEventListener(Event.ENTER_FRAME, runNextFrame);
+			nextTest()
+		}
+		stage.addEventListener(Event.ENTER_FRAME, runNextFrame);
+	}
+}
 
-// Test running a timer to completion
 function test1() {
 	var timer = new Timer(60, 3);
 	var prev = getTimer();
@@ -22,7 +85,7 @@ function test1() {
 	});
 	timer.addEventListener(TimerEvent.TIMER_COMPLETE, function(e) {
 		trace("test1 tick: timer complete at " + timer.currentCount);
-		test2();
+		runNext();
 	});
 	timer.start();
 }
@@ -36,7 +99,7 @@ function test2() {
 		trace("test2 tick: timer.currentCount = " + timer.currentCount);
 		if (timer.currentCount == 3) {
 			timer.stop();
-			test3();
+			runNext();
 		}
 	});
 	timer.addEventListener(TimerEvent.TIMER_COMPLETE, function(e) {
@@ -62,7 +125,7 @@ function test3() {
 	});
 	timer.addEventListener(TimerEvent.TIMER_COMPLETE, function(e) {
 		trace("test3 timer done");
-		test4();
+		runNext();
 	});
 	timer.start();
 }
@@ -83,7 +146,7 @@ function test4() {
 		// Adjusting repeatCount when the timer has finished does *not*
 		// make it continue
 		timer.repeatCount = 10;
-		test5();
+		runNext();
 	});
 	timer.start();
 }
@@ -97,7 +160,7 @@ function test5() {
 		if (timer.currentCount == 5) {
 			trace("test5: Reached count 5");
 			timer.stop();
-			test6();
+			runNext();
 		} else if (timer.currentCount > 5) {
 			trace("ERROR: test5 continued after stop");
 		}
@@ -118,7 +181,7 @@ function test6() {
 	});
 	timer.addEventListener(TimerEvent.TIMER_COMPLETE, function(e) {
 		trace("test6 timer done");
-		test7();
+		runNext();
 	});
 	timer.start();
 }
@@ -137,8 +200,51 @@ function test7() {
 	});
 	timer.addEventListener(TimerEvent.TIMER_COMPLETE, function(e) {
 		trace("test7 timer done");
+		runNext();
 	});
 	timer.start();	
 }
 
-test1();
+// Test decreasing repeatCount
+function test8() {
+	var timer = new Timer(40, 5);
+	var delay = new DelayCheck();
+	timer.addEventListener(TimerEvent.TIMER, function(e) {
+		delay.check(timer.delay);
+		trace("test8 tick: timer.currentCount = " + timer.currentCount);
+		if (timer.currentCount == 2) {
+			trace("test8: setting timer.repeatCount = 1");
+			timer.repeatCount = 1;
+		}
+	});
+	timer.addEventListener(TimerEvent.TIMER_COMPLETE, function(e) {
+		trace("test8 timer done");
+		runNext()
+	})
+	timer.start();
+}
+
+function test9() {
+	var timer:Timer = new Timer(200, 2);
+	var delay = new DelayCheck()
+	var ticked = false
+	timer.addEventListener(TimerEvent.TIMER, function(e) {
+		delay.check(timer.delay)
+		trace("test9 tick: timer.currentCount = " + timer.currentCount);
+		ticked = true
+	})
+	timer.addEventListener(TimerEvent.TIMER_COMPLETE, function(e) {
+		trace("ERROR: test9: Unexpected TimerEvent.TIMER_COMPLETE")
+	})
+
+	function enterFrameHandler(e:*) {
+		if (ticked) {
+			trace("test9: setting timer.repeatCount to 1 outside of tick event handler")
+			timer.repeatCount = 1;
+			stage.removeEventListener(Event.ENTER_FRAME, enterFrameHandler)
+			runNext()
+		}		
+	}
+	stage.addEventListener(Event.ENTER_FRAME, enterFrameHandler)
+	timer.start()
+}

tests/tests/swfs/avm2/timer/output.txt

@@ -36,3 +36,55 @@ test7: Increasing delay
 test7 tick: timer.currentCount = 4
 test7 tick: timer.currentCount = 5
 test7 timer done
+test8 tick: timer.currentCount = 1
+test8 tick: timer.currentCount = 2
+test8: setting timer.repeatCount = 1
+test8 timer done
+test9 tick: timer.currentCount = 1
+test9: setting timer.repeatCount to 1 outside of tick event handler
+
+Restarting tests with runDirect=false
+test1: timer.currentCount = 0
+test1 tick: timer.currentCount = 1
+test1 tick: timer.currentCount = 2
+test1 tick: timer.currentCount = 3
+test1 tick: timer complete at 3
+test2 tick: timer.currentCount = 1
+test2 tick: timer.currentCount = 2
+test2 tick: timer.currentCount = 3
+test3 tick: timer.currentCount = 1
+test3 tick: timer.currentCount = 2
+test3 RESET
+test3 tick: timer.currentCount = 1
+test3 tick: timer.currentCount = 2
+test3 tick: timer.currentCount = 3
+test3 tick: timer.currentCount = 4
+test3 tick: timer.currentCount = 5
+test3 tick: timer.currentCount = 6
+test3 timer done
+test4 tick: timer.currentCount = 1
+test4 tick: timer.currentCount = 2
+test4 tick: timer.currentCount = 3
+test4 tick: timer.currentCount = 4
+test4 tick: timer.currentCount = 5
+test4 timer done
+test5: Reached count 5
+test6 tick: timer.currentCount = 1
+test6 tick: timer.currentCount = 2
+test6 tick: timer.currentCount = 3
+test6 tick: timer.currentCount = 4
+test6 tick: timer.currentCount = 5
+test6 timer done
+test7 tick: timer.currentCount = 1
+test7 tick: timer.currentCount = 2
+test7 tick: timer.currentCount = 3
+test7: Increasing delay
+test7 tick: timer.currentCount = 4
+test7 tick: timer.currentCount = 5
+test7 timer done
+test8 tick: timer.currentCount = 1
+test8 tick: timer.currentCount = 2
+test8: setting timer.repeatCount = 1
+test8 timer done
+test9 tick: timer.currentCount = 1
+test9: setting timer.repeatCount to 1 outside of tick event handler