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.

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

Don't register the message handler with null #160

Merged
merged 3 commits into from
Jun 12, 2024

Conversation

skeet70
Copy link
Member

@skeet70 skeet70 commented Jun 11, 2024

This fixes an edge case where the worker is still loading but messages sent to it are accepted and sent to a null callback handler.

On the worker side we make sure that there is never a point where the worker registers a null function as its handler for message. We also add a single postMessage("ready") to the parent once we reach the end of the worker file.

On the frame's worker mediator side, we add Promise that is only completed when it receives "ready" from its worker. Once completed it stays completed for the life of the WorkerMediator instance. Only once that internal Promise is completed does the worker mediator start sending messages to the worker.

The combination of these two things makes sure we aren't sending messages before the worker is ready. As far as I can tell from reading the spec and all documentation, none of this should be necessary as the out MessagePort on the frame side should exist as soon as the worker is called for, and simply buffer messages until a message handler is registered on the worker side. In implementation though that didn't hold true.

@skeet70 skeet70 requested a review from coltfred June 11, 2024 01:23
skeet70 added 2 commits June 11, 2024 19:57
But jest throws a type error. TSC is fine and browsers run it fine, but
jsdom+jest blows up
@skeet70 skeet70 force-pushed the dropped-worker-to-frame-postmessage branch from 5818db4 to 3d168e8 Compare June 12, 2024 02:49
Copy link
Member

@coltfred coltfred left a comment

Choose a reason for hiding this comment

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

This seems reasonable. I think we can merge it and do further testing in staging.

Should we bump the minor?

@skeet70 skeet70 merged commit 8cff90d into main Jun 12, 2024
1 check passed
@skeet70 skeet70 deleted the dropped-worker-to-frame-postmessage branch June 12, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants