core: Track 'actions since timeout check' globally

[Dinnerbone]

We're missing cases (like #554) where the infinite loop is across multiple AVM activations. They'll eat all the memory and crash, instead of being caught by our timeout check.

This seems to match the Flash Player's behaviour more closely too.

(also hi <3)

[relrelb]

Welcome back! How about moving actions_since_timeout_check to struct Avm1 instead, since it's only relevant for AVM1?

[adrian17]

since it's only relevant for AVM1

AVM2 has a similar timeout too. Doesn't mean it'll be implemented the same way, but right now, I don't see why it couldn't be.

[adrian17]

Hmm, two questions, both related to my old comment in #3628:

should actions_since_timeout_check be stored in Context instead of Activation? In practice, I feel like this isn't an issue, as even if an inifinite loop span across a deep call stack, the outermost frame with the loop will amass 2000 actions at some point, right?

I'm assuming this means my assumption was wrong here? Does this swf do something particularly weird?

Second question: could we increase the action count limit a bit? This PR will increase the average number of checks per frame (as previously functions that executed <2000 actions were never interrupted by a time check), so would be nice to offset that to not get an accidental perf regression.

[Dinnerbone]

I'm assuming this means my assumption was wrong here? Does this swf do something particularly weird?

So in essence, this issue is basically a fancier version of this:
Frame 1: goto frame 2
Frame 2: goto frame 1

Frame 1 executes, gets a new context, gets a new activation, executes a couple of actions, done with activation, done with context.
Frame 2 executes immediately as part of frame 1, as gotos are immediate. New context, new activation, executes a few actions, done with activation, done with context.
Repeat.

Activations don't count the actions held in sub-activations - so that method only catches infinite loops at a single level of an activation, not counting its children or parents.
Contexts aren't permanent enough, and have the same nesting issue.

Second question: could we increase the action count limit a bit? This PR will increase the average number of checks per frame (as previously functions that executed <2000 actions were never interrupted by a time check), so would be nice to offset that to not get an accidental perf regression.

I have no strong feelings either way!

[adrian17]

I just tested on Meat Boy on FF and the profiles don't appear to show any meaningful difference, so I guess it's not a big deal either way. Personally I'd still bump it for a peace of mind, but that's just me :)

[relrelb]

since it's only relevant for AVM1

AVM2 has a similar timeout too. Doesn't mean it'll be implemented the same way, but right now, I don't see why it couldn't be.

My main concern is keeping AVM1 and AVM2 as much self-hosted as possible, so in the future it'll be easier to make them optional (#6750).

[Dinnerbone]

Having a single variable and test for inactivity doesn't need to hinder a separation of VMs, and seems preferable to two pieces of code tracking (and polling) for the same thing. I'd argue that this should be in player. It's a general script timeout, not a specific avm1 timeout.