Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Always pass recent decryption retry successes to widgets #12890

Closed
wants to merge 1 commit into from

Conversation

hughns
Copy link
Member

@hughns hughns commented Aug 14, 2024

Potential fix for element-hq/element-call#2561.

It looks like the current behaviour was set by #6695 for element-hq/element-web#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).

Copy link
Member

@robintown robintown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hughns
Copy link
Member Author

hughns commented Aug 21, 2024

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.

@robintown
Copy link
Member

Okay, I finally found the MSC that defines how widgets receive events: matrix-org/matrix-spec-proposals#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
Copy link
Member

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 😛

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants