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

Client.postMessage() throwing for unloaded client is problematic #1291

Closed
wanderview opened this issue Mar 23, 2018 · 8 comments · Fixed by #1293
Closed

Client.postMessage() throwing for unloaded client is problematic #1291

wanderview opened this issue Mar 23, 2018 · 8 comments · Fixed by #1293

Comments

@wanderview
Copy link
Member

Currently Client.postMessage() says:

2. Let destination be the ServiceWorkerContainer object whose
service worker client is the context object’s service worker client,
or null if no match is found.

3. If destination is null, throw an "InvalidStateError" DOMException.

This is problematic for a few reasons:

  1. Most postMessage() implementations don't throw. They just silently consume the message if the other end is no longer listening. For example Worker.postMessage() and MessageChannel, etc.
  2. The lookup of the destination is presented as synchronous in the spec, but it must be async in any cross-process browser implementation.

If we wanted we could change postMessage() to return a promise, but its probably better to just silently eat the message.

I'm filing this issue in regards to web-platform-tests/wpt#10153

@wanderview
Copy link
Member Author

cc @jungkees

@jungkees
Copy link
Collaborator

I see. port.postMessage just returns when the target is unavailable. (Step 6 of https://html.spec.whatwg.org/#dom-messageport-postmessage.) I'm not sure though we had had any specific intent to make Client.postMessage throw in the current text.

@jakearchibald, do you recall anything about it?

From web-platform-tests/wpt#10153 (comment),

@jungkees Also, how is the implementation capable of synchronously throwing here? The client and its ServiceWorkerContainer may be in another process.

When @mattto approved #1274, I thought we could maintain a data structure in the browser process that tracks the clients.

/cc @aliams @cdumez

@mfalken
Copy link
Member

mfalken commented Mar 26, 2018

I didn't realize postMessage usually doesn't throw. Sounds like we should just drop the message silently then.

@jungkees
Copy link
Collaborator

jungkees commented Mar 26, 2018

drop the message silently

Would this be trying to get the destination and returning if the target doesn't exist? Or just queuing a task for the event and the task failing to find a target in the content process?

I think port.postMessage step 6 is doing the former based on the state it keeping in the sender's process (not crossing the process boundary at the time of the call). Then, it won't cover the case where the target port has been nullified in the target process.

Do you expect we have a similar behavior for client.postMessage?

@mfalken
Copy link
Member

mfalken commented Mar 26, 2018

It sounds like postMessage should just be best-effort and if the message couldn't be delivered, regardless of the reason, it just fails silently.

@domenic
Copy link
Contributor

domenic commented Mar 26, 2018

I don't think this is relevant to the current bug thread, but I wanted to point out that that is not quite the general principle these days. In particular messages that cannot be delivered because of deserialization errors (such as crossing process boundaries while passing a SharedArrayBuffer) are currently specced to generate a messageerror event at the destination. (We talked a bit about signaling back to the source, but that's tricky, and the whole discussion was light on use cases, so it was deferred.)

Just something to remember when talking about the model of how postMessage deals with errors.

@wanderview
Copy link
Member Author

When @mattto approved #1274, I thought we could maintain a data structure in the browser process that tracks the clients.

Sure, the list is maintained in the browser process, but accessing that list from the renderer process is going to be async. Unless we want to block or spin the event loop while we consult cross-process state we can't synchronously throw here.

IMO anyway.

@jungkees
Copy link
Collaborator

@domenic, thanks for the information. Good to be aware of for future discussion.

Sure, the list is maintained in the browser process, but accessing that list from the renderer process is going to be async.

Ah.. right. This call starts from the renderer. I was confused myself we were on the browser process at the beginning.

I'll make another PR to address this issue and close the PR in WPT.

I'm thinking of getting the associated service worker client and returning if it doesn't exist, which would be analogous to https://cs.chromium.org/chromium/src/content/browser/service_worker/service_worker_version.cc?type=cs&q=ServiceWorkerVersion::PostMessageToClient&l=1155.

jungkees added a commit that referenced this issue Mar 27, 2018
cf834f0
clarified the target object can be null, and it must throw in that case.

But #1291 pointed out that we
cannot implement that behavior without blocking the service worker
process in multi-process browser architectures.

This change makes the control return right away if the target client has
been unloaded.

Fixes #1291.
jungkees added a commit that referenced this issue May 28, 2019
cf834f0
clarified the target object can be null, and it must throw in that case.

But #1291 pointed out that we
cannot implement that behavior without blocking the service worker
process in multi-process browser architectures.

This change makes the control return right away if the target client has
been unloaded.

Fixes #1291.
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 a pull request may close this issue.

4 participants