1953743 - Return "clientWindow" property in "browsingContext.contextCreated" event for window.open on Android and in "browsingContext.contextDestroyed" event

Status: RESOLVED FIXED

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

Description

[Alexandra Borovova [:Sasha]]

During the work on bug 1920952, we found out that the way we usually retrieve the window for browsing context by getting first a tab for the browsing context doesn't work in case of "browsingContext.contextCreated" event is sent for a new window (because probably a tab is not attached yet) and in case of "browsingContext.contextDestroyed" event (because probably the tab is gone already).
So we either have to find a different way to retrieve the window for a browsing context (which would work for both cases) or delay sending "browsingContext.contextCreated" event to the moment when a tab is created. In case of delaying the tab creation, we also might be able to save window ID to retrieve it for "browsingContext.contextDestroyed" event.
In the scope of this bug, we also have to update all the remaining wdspec tests to check for clientWindow field, which were not updated in the scope of bug 1920952 to not increase amount of failures.

Comment 1

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

Sasha, based on the code as landed via bug 1920952 is browsingContext.created still affected? Based on your comment and latest suggestion on the Phab revision it doesn't sound like that.

Comment 2

[Alexandra Borovova [:Sasha]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #1)

Sasha, based on the code as landed via bug 1920952 is browsingContext.created still affected? Based on your comment and latest suggestion on the Phab revision it doesn't sound like that.

"browsingContext.contextCreated" event for window.open on Android is still affected.
I updated the bug title.

Comment 3

[Liam DeBeasi]

I can try to fix this if you'd like! If I'm able to come up with a solution we all like I can also fix the incorrect test in my previous patch too.

If so, any suggestions for what I should try first or things to look out for?

Comment 4

[Alexandra Borovova [:Sasha]]

Hi Liam!
Sorry for the delay! Sure, you can take bug :)
I think I would approach it in this way:

• First, update all the remaining test to check for clientWindow, run the tests on desktop and android to verify again where the current logic fails;
• Explore the context object to look for other ways to retrieve the window. Search around: https://searchfox.org/mozilla-central/source/ to see how it's done in other parts of mozilla-central;
• In worst case scenario if there is nothing better for browsingContext.contextDestroyed, we can do something similar to what we did for browsing context ids, where we have a map to retrieve them differently specifically for this event, see what we do with navigableIds map: https://searchfox.org/mozilla-central/source/remote/shared/TabManager.sys.mjs#28-32.

Let me know if you have any questions! (For quicker feedback, you can also join our matrix channel).

Comment 5

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

An information which hasn't made it into this bug yet is the potential usage of the topChromeWindow property of a CanonicalBrowsingContext. Maybe it would help if present in those cases?

Comment 6

[Liam DeBeasi]

Hey there,

I have a fix in place that seems to be working well (Thanks for the tip on using topChromeWindow, that came in handy!). How would you recommend I update this test in original_opener.py?

It looks like the context we want in the get_tree result is not always in the same place depending on the @pytest.mark.parametrize value used. I was thinking of filtering the get_tree result to find the context we want and then asserting on that, but I wanted to check in case you had a better idea.

Comment 7

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

(In reply to Liam DeBeasi from comment #6)

It looks like the context we want in the get_tree result is not always in the same place depending on the @pytest.mark.parametrize value used. I was thinking of filtering the get_tree result to find the context we want and then asserting on that, but I wanted to check in case you had a better idea.

Great to hear! This test has actually 3 different parameterizations. Which of those is causing the trouble for you? We seem to only test top-level browsing contexts in that test so I wonder what the actual difference is that you are seeing. Maybe you can add an excerpt from the trace log? Thanks!

Comment 9

[Liam DeBeasi]

Attachment: trace log

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #7)
> (In reply to Liam DeBeasi from comment #6)
> > It looks like the context we want in the `get_tree` result is not always in the same place depending on the `@pytest.mark.parametrize` value used. I was thinking of filtering the `get_tree` result to find the context we want and then asserting on that, but I wanted to check in case you had a better idea.
> 
> Great to hear! This test has actually 3 different parameterizations. Which of those is causing the trouble for you? We seem to only test top-level browsing contexts in that test so I wonder what the actual difference is that you are seeing. Maybe you can add an excerpt from the trace log? Thanks!

The following tests fail on the clientWindow assertion:

`test_original_opener_window_open[popup-True-same_origin-tab]`
`test_original_opener_window_open[popup-True-same_origin-window]`
`test_original_opener_window_open[popup-True-cross_origin-tab]`
`test_original_opener_window_open[popup-True-cross_origin-window]`

For context, here is what my revised test body looks like:

```diff
top_level_context = await bidi_session.browsing_context.create(type_hint=type_hint)

await subscribe_events([CONTEXT_CREATED_EVENT])
on_created = wait_for_event(CONTEXT_CREATED_EVENT)

url = inline("", domain=domain)

result = await bidi_session.script.evaluate(
    expression=f"""window.open("{url}", "_blank", "{features}");""",
    target=ContextTarget(top_level_context["context"]),
    await_promise=False)

context_info = await wait_for_future_safe(on_created)

# We use None here as evaluate not always returns value.
context = None
if returns_window:
    context = result["value"]["context"]
    
+ contexts = await bidi_session.browsing_context.get_tree(root=top_level_context["context"])

assert_browsing_context(
    context_info,
    context=context,
    original_opener=top_level_context["context"],
    url="about:blank",
+   client_window=contexts[0]["clientWindow"]
)
```

Here's an excerpt of the trace log:

```
FAIL test_original_opener_window_open[popup-True-cross_origin-window] - AssertionError

...

bidi_session = <webdriver.bidi.client.BidiSession object at 0x103537cb0>
context    = 'e80e4c28-e5cc-4db7-b00c-dca776ff87d3'
context_info = {'children': None,
 'clientWindow': '12d5e389-90f4-43be-b369-1fa2091c89f0',
 'context': 'e80e4c28-e5cc-4db7-b00c-dca776ff87d3',
 'originalOpener': 'a428f2d7-3116-48ef-91b8-1ada89819da7',
 'parent': None,
 'url': 'about:blank',
 'userContext': 'default'}
contexts   = [{'children': [],
  'clientWindow': '27389ab7-d908-4f2b-9205-b2b1582564fc',
  'context': 'a428f2d7-3116-48ef-91b8-1ada89819da7',
  'originalOpener': None,
  'parent': None,
  'url': 'about:blank',
  'userContext': 'default'}]
```

Comment 10

[Liam DeBeasi]

Whoops looks like I somehow submitted my comment right after attaching a file. You can ignore the trace log summary at the bottom of my comment as I included the full log as an attachment.

Changing the root in the get_tree call to get_tree(root=context_info["context"]) that "fixes" the issue, but I don't know if that's the correct way to do it since we're using context_info as a reference for both sides of the assertion.

Comment 11

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

Liam, the attached trace log only contains the actual test assertion details. In the future it will be good to actually get the output as well between sending the command and receiving the response. But lets see...

So when you call get_tree() you are actually passing in top_level_context["context"] which is the context of the initially opened window via bidi_session.browsing_context.create but not by using window.open(). As you said it is because root=top_level_context["context"] which will return the first opened window via browsingContext.create(), but not window.open. What you want is to assert the clientWindow field of context_info against a known value that we need to get via the result value of bidi_session.script.evaluate().

Comment 12

[Liam DeBeasi]

Liam, the attached trace log only contains the actual test assertion details. In the future it will be good to actually get the output as well between sending the command and receiving the response. But lets see...

Oh good to know! I'll make sure to do that next time.

So when you call get_tree() you are actually passing in top_level_context["context"] which is the context of the initially opened window via bidi_session.browsing_context.create but not by using window.open(). As you said it is because root=top_level_context["context"] which will return the first opened window via browsingContext.create(), but not window.open. What you want is to assert the clientWindow field of context_info against a known value that we need to get via the result value of bidi_session.script.evaluate().

Thanks a lot for the help. I was able to get the test fixed. I'll get this code up for review shortly.

Comment 13

[Liam DeBeasi]

Attachment: Bug 1953743 - [webdriver-bidi] clientWindow is returned in contextCreated and contextDestroyed events r=Sasha

Comment 14

[Liam DeBeasi]

Attachment: Bug 1953743 - [wdspec] Update browsing context assertions to check for clientWindow r=Sasha

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/191baf1be3d0
[webdriver-bidi] clientWindow is returned in contextCreated and contextDestroyed events r=Sasha,webdriver-reviewers
https://hg.mozilla.org/integration/autoland/rev/40f9fe959b19
[wdspec] Update browsing context assertions to check for clientWindow r=Sasha,webdriver-reviewers

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Failed to create upstream wpt PR due to merge conflicts. This requires fixup from a wpt sync admin.

Comment 17

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/191baf1be3d0
https://hg.mozilla.org/mozilla-central/rev/40f9fe959b19

Comment 18

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/52469 for changes under testing/web-platform/tests

Comment 19

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot