matrix-react-sdk icon indicating copy to clipboard operation
matrix-react-sdk copied to clipboard

Always pass recent decryption retry successes to widgets

Open hughns opened this issue 1 year ago • 1 comments

Potential fix for https://github.com/element-hq/element-call/issues/2561.

It looks like the current behaviour was set by https://github.com/matrix-org/matrix-react-sdk/pull/6695 for https://github.com/element-hq/element-web/issues/18060. But, I don't know if the scenario I am encountering was considered. So, this might be the wrong way to go about it.

I tried to add some test cases that cover the basic encrypted event semantics too.

Checklist

  • [ ] Tests written for new code (and old code if feasible).
  • [ ] New or updated public/exported symbols have accurate TSDoc documentation.
  • [ ] Linter and other CI checks pass.
  • [ ] Sign-off given on the changes (see CONTRIBUTING.md).

hughns avatar Aug 14 '24 16:08 hughns

The semantics seem a bit confused now: there's a comment saying that this code is meant to prevent out-of-order delivery of events, but this new branch appears to deliberately do the opposite, forwarding events in whatever order they're decrypted. I would expect to see this code block the delivery of future events on whether past events have been decrypted, I think.

I can also see why this behavior could be more desirable though (everything gets through without any head-of-line blocking). Maybe I need to read up on the semantics that the widget API prescribes again.

Yeah, it wasn't obvious to me what the original intention was.

If the intention is that a Widget should only receive events from the point at which it was added/started on the room then a simpler, time based filtering could be applied.

The current implementation appears to give some history and then I don't think it allows for late decryptions.

hughns avatar Aug 21 '24 14:08 hughns

Okay, I finally found the MSC that defines how widgets receive events: https://github.com/matrix-org/matrix-spec-proposals/pull/2762 Notably it says,

The client SHOULD only send events which were received by the client after the session has been established with the widget (after the widget's capabilities are negotiated).

So that suggests that we shouldn't be sending historical messages at all. The same MSC gives a way for widgets to backfill events on-demand, so there's still a proper way for widgets to get this information if they want it.

However, the MSC ultimately has nothing to say about the order in which events are pushed to the widget. So, this situation is still just as tricky: clearly, an effort was made to have this code push events strictly in the same order that they're received, but clearly, this also means that you must accept either head-of-line blocking or unreliable delivery as a trade-off.

The possible ways forward I see are:

  • Update the MSC to say that events can be delivered out of order, and add a numeric order key on events that are pushed, so that the widget can reconstruct the order on the other side, if this seems necessary & reasonable. Rip out the code that enforces in-order delivery.
  • Stick with strict in-order delivery, and determine which trade-off, out of head-of-line blocking and unreliable delivery, we want to live with.

robintown avatar Sep 03 '24 16:09 robintown

Notes from discussion:

  • Our preferred way to fix this would be to declare that events are not necessarily delivered by the widget API in any particular order. But, we should still have some kind of heuristic to avoid forwarding events that are too far back in the backfill.
  • Even if we don't make any ordering guarantees, there would still be the opportunity to add some back at a later date, possibly as an order property on the event objects that are sent through, or as an option that the widget sets to determine whether a strict ordering should be followed.
  • Because we're moving to to-device key distribution in Element Call, this isn't a priority for us currently, so we're going to shelve this.

This PR doesn't implement this preferred way, so we're going to close this PR. Too big of a can of worms for now :stuck_out_tongue:

robintown avatar Sep 06 '24 14:09 robintown