1708070 - Still having some crashes after having dom.input_events.strict_input_vsync_alignment enabled again

Status: RESOLVED FIXED

(Core :: Performance, defect)

Description

[Sean Feng [:sefeng]]

We still having these DidRunTask because an edge case, such that vsync -> rAF -> xhr triggers event loop -> runInput -> crash.

Comment 1

[Sean Feng [:sefeng]]

Attachment: Bug 1708070 - Use VsyncTaskManager::WillRunTask to change InputVsyncState r=smaug

Currently, we use VsyncTaskManager::DidRunTask to change state from
RunVsync to NoPendingVsync, however, the issue is that if the vsync
starts an event loop (for instance, by using requestAnimationFrame), and
the event loop starts another input task, Firefox crashes because input
tasks don't expect the state to be RunVsync.

So instead of using DidRunTask, we start to use WillRunTask to fix
it.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/57e445916fc9
Use VsyncTaskManager::WillRunTask to change InputVsyncState r=smaug

Comment 3

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/57e445916fc9

Comment 4

[Sean Feng [:sefeng]]

Reopen this due to still having the crashes

Comment 5

[Sean Feng [:sefeng]]

Attachment: Bug 1708070 - Add a few more assertions to InputTaskManager to investigate some crashes r=smaug

Rarely, we sill see a input task was run when the expectation was a
vsync task. I am unable to find the root cause by looking at the code.
One possible case is the vsync was gone when the input task was run.
This patch added some assertions to see if that was the case.

Comment 6

[Sean Feng [:sefeng]]

Attachment: Bug 1708070 - Change InputVsyncState in InputPriorityController::WillRunTask r=smaug

Before this patch, we use InputPriorityController::DidRunTask to change
InputVsyncState which is problematic.

Consider this scenario

1. Two events are in the queue, vsync(V1) and input(I1)
2. I1 runs and starts an inner event loop (We only expect one input
   event to be run because there's only one input event)
3. Another input event(I2) arrives
4. Inner event loop picks I2 to run
5. When I2 is finished, it sets the InputVsyncState to RunVsync
6. I1's DidRunTask is called and crashed because the state shouldn't
   be RunVsync.

This patch moves the code which checks InputVsyncState from DidRunTask
to WillRunTask so that the state is correctly checked and updated
before the input task is about to run.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f4a7cf1a8f7
Change InputVsyncState in InputPriorityController::WillRunTask r=smaug

Comment 8

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/2f4a7cf1a8f7

Comment 9

[Julien Cristau [:jcristau]]

There's no crashes on beta AFAICT.

Comment 10

[Kim Moir [:kmoir] ET]

Sefeng can you set a priority and severity for this bug? (I'm going through triage)

Comment 11

[Sean Feng [:sefeng]]

This can now be closed after landing https://phabricator.services.mozilla.com/D117336 because if we still get new crashes, the crash signature will be changed.

Comment 12

[Mathew Hodson]

dom.input_events.strict_input_vsync_alignment is disabled on Beta, so that's why we wouldn't see crashes there.