1819902 - Support origin checks when deserializing a shared reference

Status: NEW

(Remote Protocol :: WebDriver BiDi, enhancement, P2)

Description

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

On bug 1770733 I'm going to add support for shared references. To be able to land the patch earlier we agreed on that support for origin checks can be done as a follow-up. Also because there is the open WebDriver BiDi issue which I would like to see clarified first - so that it is clear if we should throw an error or not.

Comment 1

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

The PR https://github.com/w3c/webdriver-bidi/issues/376 has been merged.

Comment 2

[Alexandra Borovova [:Sasha]]

As the way to verify this, we could also add a test case for browsingContext.locateNodes that uses a node from the one browsing context as startNode and then tries to locate in the another browsing context. See a discussion here: https://phabricator.services.mozilla.com/D196743#inline-1093416.

Comment 3

[Julian Descottes [:jdescottes]]

I think this makes a few cross-frame playwright tests fail, such as

it('should work for cross-frame evaluations', async ({ page, server }) => {
  await page.goto(server.EMPTY_PAGE);
  await attachFrame(page, 'frame1', server.EMPTY_PAGE);
  const frame = page.frames()[1];
  const elementHandle = await frame.evaluateHandle(() => window.top.document.querySelector('#frame1'));
  expect(await elementHandle.contentFrame()).toBe(frame);
});

First the test is creating an iframe dynamically via a script.evaluate/callFunction. The frame element is returned and stored in the seenNodesMap. It is associated with the top level browsingContext id (in this test usually 15).

Then we try to retrieve the iframe element from the iframe's context itself, by evaluating window.top.document.querySelector('#frame1').
I think this works, however at some point playwright is trying to "adoptElementHandle", and they end up trying to resolve the sharedId of the iframe element in the iframe context.

And then we fail the deserialization at https://searchfox.org/mozilla-central/rev/6d06ffc186ac356648f1feeadd52cc61b2ad6737/remote/webdriver-bidi/RemoteValue.sys.mjs#233-241

if (nodeDetails.isTopBrowsingContext && browsingContext.parent === null) {
  // As long as Navigables are not available any cross-group navigation will
  // cause a swap of the current top-level browsing context. The only unique
  // identifier in such a case is the browser id the top-level browsing
  // context actually lives in.
  return nodeDetails.browserId === browsingContext.browserId;
}

return nodeDetails.browsingContextId === browsingContext.id;

Here, nodeDetails.isTopBrowsingContext is true because the node was created in the top level context. But browsingContext.parent is not null, because it's the browsingContext for an iframe. And nodeDetails.browsingContextId is the top level browsingContext id while browsingContext.id is the iframe id.

I will try to create a wdspec test for this...

Comment 4

[Julian Descottes [:jdescottes]]

Here's a basic wdspec which triggers the same issue:

@pytest.mark.asyncio
async def test_deserialize_shared_id_cross_frame(bidi_session, top_context, inline):
    frame_url = inline("<div>foo</div>")
    url = inline(f"<iframe src='{frame_url}'></iframe>")
    await bidi_session.browsing_context.navigate(
        context=top_context["context"],
        url=url,
        wait="complete",
    )

    all_contexts = await bidi_session.browsing_context.get_tree()
    iframe_context = all_contexts[0]["children"][0]

    # 1. Retrieve the iframe element from the top context via JS: OK
    result = await bidi_session.script.evaluate(
        expression="document.querySelector('iframe')",
        target=ContextTarget(top_context["context"]),
        await_promise=False,
    )

    recursive_compare(
        {
            "type": "node",
            "sharedId": any_string,
            "value": {
                "localName": "iframe",
            },
        }, result)
    shared_id = result["sharedId"]

    # 2. Retrieve the iframe element from the iframe context via JS: OK
    result = await bidi_session.script.evaluate(
        expression="window.top.document.querySelector('iframe')",
        target=ContextTarget(iframe_context["context"]),
        await_promise=False,
    )
    assert shared_id == result["sharedId"]

    # 3. Retrieve the iframe element from the top context via sharedId: OK
    result = await bidi_session.script.call_function(
        function_declaration="x => x",
        arguments=[{
            "sharedId": shared_id
        }],
        await_promise=False,
        target=ContextTarget(top_context["context"]),
    )
    assert shared_id == result["sharedId"]

    # 4. Retrieve the iframe element from the iframe context via sharedId: KO
    result = await bidi_session.script.call_function(
        function_declaration="x => x",
        arguments=[{
            "sharedId": shared_id
        }],
        await_promise=False,
        target=ContextTarget(iframe_context["context"]),
    )
    assert shared_id == result["sharedId"]

But it also fails with Chrome, so I'm not sure if this is actually a bug or if this limitation is following the spec?
It seems odd that we can't use the sharedId of an element which is otherwise available in the context where we evaluate the script, but I can be misremembering this?

Comment 5

[Julian Descottes [:jdescottes]]

Henrik, can you read the 2 comments above, and let me know if our current behavior is correct?

Although note that Chrome throws, and Firefox just returns null, so there's still a small difference in terms of behavior, which we should address.

Comment 6

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

Yes, in such a case we compare the browsing context ids of both the node to deserialize and the context the command actually runs in. Because they are different the isSameBrowsingContext() helper returns false.

How many tests are currently failing because of that? I wonder about the prioritization.

Comment 7

[Julian Descottes [:jdescottes]]

I think there are at least 2 tests impacted:

• page/elementhandle-content-frame.spec.ts :: should work for cross-frame evaluations
• page/elementhandle-owner-frame.spec.ts :: should work for cross-frame evaluations

(I mostly investigated the first test, but the other one looks similar)