Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Simplify the communication between the browser host and iframe #2065

Closed
6 of 9 tasks
natebosch opened this issue Jul 19, 2023 · 2 comments · Fixed by #2099
Closed
6 of 9 tasks

Simplify the communication between the browser host and iframe #2065

natebosch opened this issue Jul 19, 2023 · 2 comments · Fixed by #2099

Comments

@natebosch
Copy link
Member

natebosch commented Jul 19, 2023 •
edited
Loading

Currently there are 3 hops during startup.

This communication all happens an an opaque way, especially on remote CI infrastructure, and may be involved with some flakes that we see in a few places where browser tests never start. We should simplify the communication, and add some logging to see if we can get any insight into the cause of the flakes.

A better pattern would be

  • iframe posts the MessageChannelPort to the parent and wires it up to the channel.
  • host receives the port and wires it up to the channel. Start forwarding messages when the channel is ready instead immediately forwarding to a callback that waits for the channel to be ready.

Separately, we have some unnecessary legacy behavior with extra messages that turned out to be a no-op round trip sending a signal back to the calling code to continue. We can clean that up in the same pass - I've already removed the usage in source but have not yet released that version internally.

#2061 does the full refactoring, and would be safe to land if this was the only test runner. As mentioned in #2061 (comment) we will need to do this in multiple steps with backwards compatibility along the way because there is another copy of both sides of the implementation, and we cannot roll them all atomically.

Somewhat interesting side note - Even though the flutter use case doesn't pin package:test as a dependency, they reach into the pub solve within the flutter SDK to find the version of test pinned through a dev_dependency and use that version of the compiled host js.

My plan is to:

  • update this test runner host.dart side to handle either behavior - it receives the first message and can know what version of the iframe it is communicating with.
  • Update the internal copy of the host to handle either behavior.
  • land in google3 and publish.
  • roll the test runner into flutter.
  • Update flutter's copy of postMessageChannel to the new behavior.
  • Update test runner copy of postMessageChannel to the new behavior.
  • Remove support for the old behavior in this test runner host.dart.
  • land and publish again.
  • Remove support for the old behavior in the internal runner host.

@jakemac53 - do you have any concerns?
@natebosch
Copy link
Member Author

I do not plan to have local tests validating backwards compatibility at any step here since I don't expect any step to be long lived. I plan on manually verifying cross compatibility as we go. Existing tests within each of the three relevant repositories will ensure that the used set of cross compatibilities are working as we use them, so the risk is only that some steps might need multiple PRs if a problem surfaces trying to land it in the next repo.

@natebosch natebosch mentioned this issue Jul 19, 2023
Merged
natebosch added a commit that referenced this issue Jul 19, 2023
@natebosch
Expand dom interop to cover more uses
a58241c 
Add a `parent` getter on `window`. Use it to post a parent message
instead of a private copy of the JS interop for the same. This had been
using `@JS()` locally as a workaround for a bug in `dart:html`, and now
that we aren't using `dart:html` anywhere in this code we can drop the
extra copy.

Expose the `source` field on `MessageEvent`. Use `js_util` to read the
properties which may be missing to get to the `href` for the message.
Trying to read the field through `dart:html` could throw, but after the
migration to `@JS()` style interop the difference interfaces for the
event source can be handled safely. Even though the host is no longer
reading the href key from the messages they are still sent from the
frame side for backwards compatibility with other host implementations.
See #2065
@natebosch
Copy link
Member Author

reach into the pub solve within the flutter SDK

We saw in a recent issue changing some code in test_api which impacted usage in flutter_test that some flutter users are running flutter update-packages and corrupting their SDK pub solve - I think with this plan even those users would be safe from this particular cross compatibility issue because the plan is already to make the new test backwards compatible with the current flutter_test for at least one release, and update-packages shouldn't ever downgrade test to a version that wouldn't be compatible.

@natebosch natebosch mentioned this issue Jul 19, 2023
Open
natebosch added a commit that referenced this issue Jul 19, 2023
@natebosch
Expand dom interop to cover more uses (#2066)
e76bffe 
Add a `parent` getter on `window`. Use it to post a parent message
instead of a private copy of the JS interop for the same. This had been
using `@JS()` locally as a workaround for a bug in `dart:html`, and now
that we aren't using `dart:html` anywhere in this code we can drop the
extra copy.

Expose the `source` field on `MessageEvent`. Use `js_util` to read the
properties which may be missing to get to the `href` for the message.
Trying to read the field through `dart:html` could throw, but after the
migration to `@JS()` style interop the difference interfaces for the
event source can be handled safely. Even though the host is no longer
reading the href key from the messages they are still sent from the
frame side for backwards compatibility with other host implementations.
See #2065
@natebosch natebosch mentioned this issue Jul 19, 2023
Merged
natebosch added a commit that referenced this issue Jul 19, 2023
@natebosch
Handle initial message entirely within conditional
3c2702c 
Towards #2065

Refactor the current implementation to reduce complexity and keep the
code for the current behavior within the condition body. This will allow
the new behavior to be implemented in another branch of the conditional.

- Remove the `readyCompleter`. Don't start listening on the server
  channel until the frame communication is ready instead of eagerly
  listening and holding the messages callbacks awaiting the ready
  future.
- Keep only the long-term message channel in the stored subscriptions.
  We only expect a single message on the window channel from each frame,
  so we can cancel the subscription after we receive it.
natebosch added a commit that referenced this issue Jul 19, 2023
@natebosch
Handle initial message entirely within conditional (#2067)
37e54e3 
Towards #2065

Refactor the current implementation to reduce complexity and keep the
code for the current behavior within the condition body. This will allow
the new behavior to be implemented in another branch of the conditional.

- Remove the `readyCompleter`. Don't start listening on the server
  channel until the frame communication is ready instead of eagerly
  listening and holding the messages callbacks awaiting the ready
  future.
- Keep only the long-term message channel in the stored subscriptions.
  We only expect a single message on the window channel from each frame,
  so we can cancel the subscription after we receive it.
natebosch added a commit that referenced this issue Jul 28, 2023
@natebosch
Allow frame to send the message port
c645686 
Towards #2065

Handle the new communication pattern from the host side. Allows the
frame to send a single `'port'` message with a read `MessageChannel` in
stead of the `{'ready': true}` message signalling that the host should
send a message port. This will remove one hop in the communication
pattern.

Retains handling of the current pattern and does not update the frame
side yet. There are multiple implementations of both the host and the
frame behavior. All host implementations will be updated to allow either
pattern before the frame implementations are updated.

Change from a `if/else` chain to a `switch`. Use pattern matching to
destructure the `'data'` field in the case of exceptions.
@natebosch natebosch mentioned this issue Jul 28, 2023
Merged
natebosch added a commit that referenced this issue Jul 31, 2023
@natebosch
Allow frame to send the message port (#2068)
92eb0f7 
Towards #2065

Handle the new communication pattern from the host side. Allows the
frame to send a single `'port'` message with a read `MessageChannel` in
stead of the `{'ready': true}` message signalling that the host should
send a message port. This will remove one hop in the communication
pattern.

Retains handling of the current pattern and does not update the frame
side yet. There are multiple implementations of both the host and the
frame behavior. All host implementations will be updated to allow either
pattern before the frame implementations are updated.

Change from a `if/else` chain to a `switch`. Use pattern matching to
destructure the `'data'` field in the case of exceptions.

Don't append the frame to the dom until the window message listener
is listening.
natebosch added a commit that referenced this issue Aug 2, 2023
@natebosch
Send a MessagePort to host on frame startup
b44b015 
Towards #2065

All host implementations are updated to allow either the legacy format,
or this new format.

Simplify the communication and avoid a listener on the frame window
message events. Upon startup replace the `{'ready': true}` message with
a `'port'` message and the `MessagePort` for a channel created on the
frame side. Omit the manual 'href' field, no host implementations would
read it.
This was referenced Aug 2, 2023
Merged
Merged
natebosch added a commit to flutter/flutter that referenced this issue Aug 3, 2023
@natebosch
Send test message channel from the test frame
73b7917 
Towards dart-lang/test#2065

The flutter test runner uses the copy of `host.dart.js` from the copy of
`package:test` that surfaces in the pub solve for `flutter_tool`. This
copy has been updated to allow either the old pattern of communication,
or this new pattern. The new pattern removes an extra hop and use of the
frame `window.onMessage` messages.
natebosch added a commit that referenced this issue Aug 9, 2023
@natebosch
Send a MessagePort to host on frame startup (#2072)
9b1828f 
Towards #2065

All host implementations are updated to allow either the legacy format,
or this new format.

Simplify the communication and avoid a listener on the frame window
message events. Upon startup replace the `{'ready': true}` message with
a `'port'` message and the `MessagePort` for a channel created on the
frame side. Omit the manual 'href' field, no host implementations would
read it.
fluttermirroringbot pushed a commit to flutter/flutter that referenced this issue Aug 11, 2023
@natebosch
Send test message channel from the test frame (#131881)
d23fb23 
Towards dart-lang/test#2065

The flutter test runner uses the copy of `host.dart.js` from the copy of
`package:test` that surfaces in the pub solve for `flutter_tool`. This
copy has been updated to allow either the old pattern of communication,
or this new pattern. The new pattern removes an extra hop and use of the
frame `window.onMessage` messages.
natebosch added a commit that referenced this issue Sep 21, 2023
@natebosch
Drop support for legacy iframe communication
16e52d0 
Closes #2065

All known iframe side implementations have been updated to send the
`'port'` message with an immediately available channel over the map and
the extra window message.

Move some still relevant `assert` into the implementation for the
current approach.
@natebosch natebosch mentioned this issue Sep 21, 2023
Merged
natebosch added a commit that referenced this issue Sep 25, 2023
@natebosch
Drop support for legacy iframe communication (#2099)
7ec1bbf 
Closes #2065

All known iframe side implementations have been updated to send the
`'port'` message with an immediately available channel over the map and
the extra window message.

Move some still relevant `assert` into the implementation for the
current approach.
natebosch added a commit to natebosch/engine that referenced this issue Oct 13, 2023
@natebosch
Handle new iframe communication pattern
797f732 
See dart-lang/test#2065

Multiple copies of the host and iframe side implementations have been
updated, but this copy of `host.dart` was missed. Apply some of the
changes from the linked test issue to this copy.

- Add `MessageEventSource` and `MessageEventSourceLocation` interop
  classes. The frame side communication no longer includes an `href`
  field in the map, read it through `message.source.location`.
- Move the `DomMessageChannel` initiation into the handler for the
  `{'ready': true}` event. This removes the need for the
  `readyCompleter` since we do not start forwarding messages until the
  port is ready for use.
- Add handling for the new pattern, when the iframe sends a message with
  the string `'port'` instead of a Map.
@natebosch natebosch mentioned this issue Oct 20, 2023
Merged
auto-submit bot pushed a commit to flutter/engine that referenced this issue Oct 23, 2023
@natebosch
Update to the latest package:test (#46592)
fbc0a34 
- Update the pinned version of `test` to the latest published.
- Add support for reading the `source.location` of messages to the JS
  interop library.
- Update `host.dart` to support the new communication pattern with the
  test frame. See dart-lang/test#2065
- Use `Runtime.edge` for the edge browser. We may deprecate or remove
  the constant. Edge is a more appropriate value for this usage.
harryterkelsen pushed a commit to flutter/engine that referenced this issue Oct 23, 2023
@natebosch @harryterkelsen
Update to the latest package:test (#46592)
f0e2b6d 
- Update the pinned version of `test` to the latest published.
- Add support for reading the `source.location` of messages to the JS
  interop library.
- Update `host.dart` to support the new communication pattern with the
  test frame. See dart-lang/test#2065
- Use `Runtime.edge` for the edge browser. We may deprecate or remove
  the constant. Edge is a more appropriate value for this usage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Drop support for legacy iframe communication dart-lang/test
1 participant
@natebosch