1654513 - Move usage of element store from content to parent process

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, task, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

While working on implementing WebDriver:FindElement over on bug 1654454, I noticed that there is a problem when we should raise Stale Element errors but we don't. Given that for each page being loaded we get a new JSWindowActor pair, storing the already found elements there, will always cause an empty list. As such no State Element is raised for already retrieved elements.

The same would most likely be the case for frames, but that's something I cannot test yet because WebDriver:SwitchToFrame hasn't been implemented yet.

When transfering the elements from the client to the parent process we should also include the browsing context (id) the element actually lives within. Similar to what ContentDOMReference.jsm is doing. That way we can make sure to only validate elements from the currently set browsing context.

Maja, do you want to have a look at this over the next 2 weeks?

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

It shouldn't actually block bug 1654454 bug depend on it. Otherwise you won't be able to actually test it.

Comment 2

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Yes

Comment 3

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

I'll summarize some recent conversations and investigations about this.

Marionette's element store basically maps a {uuid, element-type} (see WebElement) to a DOM element (weakref) and it's used to detect stale elements, for example. The scope of this bug is the element store used by the MarionetteFrameParent/Child actors. The child actor needs to operate on actual Elements.

Note: If we instantiate the element store in the global scope of MarionetteFrameChild.jsm then it lives in a content process. This actually works fine, even with cross-origin navigation, but that's not enough. If there's any remoteness change (such as navigating to about:robots), we lose any state we stored. If there are cross-origin subframes, they will each see a different element store in their own process, which is not what we want.

So as mentioned before, we want the element store in the parent process, but then we have a lot of IPC: for every element update or look-up in the child actor, we have to send a request to the parent actor.

The two related ideas I have so far are to (1) map from ContentDOMReference identifier to WebElement in global scope of MarionetteFrameParent.jsm alongside an element.Store, and then use only ContentDOMReference in the child actor. And/or (2) refactor Marionette's element.Store as a fork of ContentDOMReference -- some initial investigation was done in Bug 1650800

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Maja Frydrychowicz :maja_zf (UTC-4) ([e-mail]) from comment #3)

So as mentioned before, we want the element store in the parent process, but then we have a lot of IPC: for every element update or look-up in the child actor, we have to send a request to the parent actor.

Element lookups should only be done by the executing method that lives in the child process (actor). Anything in the parent process wouldn't have to know about that element except its WebElement id. Note that resolving the id would even only be possible within the same process the website actually runs in.

The two related ideas I have so far are to (1) map from ContentDOMReference identifier to WebElement in global scope of MarionetteFrameParent.jsm alongside an element.Store, and then use only ContentDOMReference in the child actor. And/or (2) refactor Marionette's element.Store as a fork of ContentDOMReference -- some initial investigation was done in Bug 1650800

Note that for (1) the parent process exactly would have to know about the type of node (element, window, frame). That information would have to be transferred beside the element's weakref id. And in regards of (2) it would be good if that's something we should do at all or not. If there is a clear statement that it should not be done, we just immediately drop the idea.

Comment 5

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

In the discussion above, we're considering forking ContentDOMReference.jsm to meet Marionette's requirements for an element store as we move parts of the Marionette protocol to a JSWindowActor implementation. Is ContentDOMReference.jsm expected to change much? Is it expected to be used in the longer term?

Comment 6

[Kris Maglione [:kmag]]

Honestly, I'm probably going to replace it with something completely native eventually. The implementation isn't super efficient. But I don't think you should have any issues if you fork it.

That said, I'm not entirely clear on why you need to fork at all. What doesn't ContentDOMReference do that you need, and why can't it just be updated to do it?

Comment 7

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Attachment: Bug 1654513 - [marionette] Track seen web content elements in parent process

In the parent process, ReferenceStore maps from WebElement uuids to
the ElementIdentifier expected by ContentDOMReference and the
WebElement json representation. Thus, the elements retrieved in
MarionetteFrameChild via ContentDOMReference are tracked
across all types of navigation so we can accurately throw errors
when stale elements are used.

The other goal here is take advantage of ContentDOMReference
while keeping other Marionette internals around elements largely
the same.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7b70f6088f0c
[marionette] Track seen web content elements in parent process r=marionette-reviewers,whimboo

Comment 9

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/7b70f6088f0c