Open Bug 1683193 Opened 4 years ago Updated 1 year ago

Move handling of actions input states to WebDriverSession

Categories

(Remote Protocol :: Marionette, task, P2)

Default
task

Tracking

(Not tracked)

People

(Reporter: whimboo, Unassigned)

References

(Depends on 1 open bug)

Details

(Whiteboard: [marionette-fission-reserve])

Right now the action input state is stored in each and every process (including different content processes) separately. This is because it's state is hold in action.js., which gets loaded by the MarionetteCommandsActor child.

It means that different processes have their own state, and it cannot be shared.
This is especially important for Fission, when sub-frames could be out of process, and won't have the state from eg. the parent.

I can see especially the pointerevents web-platform tests failing a lot with invalid input states, which cause harness errors and as such most of the tests are marked as expected Error.

James, am I reading the WebDriver spec correctly? Would be great to get your feedback before we actually get this changed. Thanks.

Flags: needinfo?(james)

I think this should be per session rather than per window and afaict that's what ChromeDriver does.

Flags: needinfo?(james)

Tracking marionette-fission-reserve bugs for Fission MVP

Fission Milestone: --- → MVP
Blocks: 1669172

Julien, I might not be able to get to this bug this week. Would it be possible for you to investigate / fix that in January? It's the last blocking bug for the framescript code removal. We need some similar logic as for the element reference store.

I'm happy to chat about details this week, so it's clear what needs to be done here.

Flags: needinfo?(jdescottes)
Priority: P3 → P2

Sure, I can take a look this week.

Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Flags: needinfo?(jdescottes)

I have a basic implementation working at https://treeherder.mozilla.org/jobs?repo=try&revision=330cc3ad37b0844ab4b7cc35d4e3a653dda60fa9 but it needs a lot of cleanup.

The general idea was to copy action.js to an action-parent.js module. action-parent.js is almost identical to action.js, except that it relies on JSWindowActors to create/synthesize events.

We can share a lot of code between action & action-parent, my current patch was just a WIP to get an idea of what kind of issues could occur.

Some interesting bits:

  • the DoubleClick state stays in the content process for now. We might move it to the parent process later, but it is updated outside of the scope of actions, so I would prefer not to update it here
  • the PointerMove was a bit challenging here. Initially I imagined that everything except event creation would be done in parent. But I'm afraid that adding async roundtrips between parent and child for PointerMove could create issues. So for now, the child process still handles the timer & creation of pointer move events (https://searchfox.org/mozilla-central/rev/014fe72eaba26dcf6082fb9bbaf208f97a38594e/testing/marionette/action.js#1406-1436)
  • for now I am sending keyEvent and mouseEvent objects from parent to child. This created an issue with mouseEvents because the serialization turns undefined properties into null. This means that a mouseEvent with code: undefined on the parent became code: null in the child, which confused the event creation. As a workaround, I perform a special cleanup on those parameters. As a final solution, we can either try to pass "safer" arguments (it was just easier to pass the keyEvent/mouseEvent object, but in some cases, we could get away with smaller payloads). Or we could also change the logic that turns undefined to null (https://searchfox.org/mozilla-central/rev/014fe72eaba26dcf6082fb9bbaf208f97a38594e/testing/marionette/evaluate.js#301-303)

Does this marionette-fission-reserve bug need to block shipping Fission MVP?

Fission Milestone: MVP → ?

For now, I am assuming marionette-fission-reserve bugs don't need to block shipping Fission MVP.

Fission Milestone: ? → Future
No longer blocks: 1669172
Depends on: 1669172

As per spec the WebDriver session has to hold these values. As such we should wait for bug 1683193.

A session has an associated list of active input sources.
A session has an associated input state table.
A session has an associated input cancel list.

Depends on: 1691402
Summary: Move handling of actions input state to parent process → Move handling of actions input states to WebDriverSession

This bug is not a priority right now but I want to mention that it is unblocked now. So whenever there is a free slot feel free to continue.

Lets wait at least until the refactoring of the WebDriver session has been done via bug 1720707.

Assignee: jdescottes → nobody
Status: ASSIGNED → NEW
Fission Milestone: Future → ---
Depends on: 1720707
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.