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

When does the openWindow promise resolve? #728

Closed
aykevl opened this issue Jul 30, 2015 · 15 comments
Closed

When does the openWindow promise resolve? #728

aykevl opened this issue Jul 30, 2015 · 15 comments
Labels
Milestone

Comments

@aykevl
Copy link

aykevl commented Jul 30, 2015

I am trying to send a message to a page from a serviceWorker. The goal is to let the page respond to the click (opening what the notification is about).

With this code inside a notificationclick event handler:

clients.openWindow(url).then(function(client) {
    client.postMessage(message);
});

There is a race between delivering the message event and attaching the message event listener (while loading the page). Experimentally, I have found that the only way to receive the event is to add the message event listener before <html>, like so:

<!DOCTYPE html>

<script>
navigator.serviceWorker.addEventListener('message', function (event) {
    console.log('got message:', event.data);
});
</script>

<html xmlns="http://www.w3.org/1999/xhtml">
    <head>
    (...)

This appears very unreliable to me (tested in Chrome for Android version 44 and 45).

A possible solution is letting the promise resolve around the DOMContentLoaded or load events. Another option could be to let the service worker detect when the page is loaded (via an event).

I have used a workaround: sending the needed information in the URL fragment identifier and navigating to the proper URL using history.replaceState in the client (webpage). This works reliable so far.

@jungkees
Copy link
Collaborator

Isn't the sync between listener setup and sender's posting a message between different threads rather a general case? I'm not sure whether a snippet below could help? /cc @jakearchibald

// Page
navigator.serviceWorker.addEventListener('message', function (e) {
    console.log('got message:', e.data);
});

document.addEventListener('DOMContentLoaded', e => {
  var controller = navigator.serviceWorker.controller;
  if (controller)
    controller.postMessage('clientloaded');
})

// SW

// Yeah, we need a persistent state to store |client_id|, so assuming that ..

self.addEventListener('notificationclick', e => {
  e.waitUntil(clients.openWindow(url).then(c => { client_id = c.id; }));
});

self.onmessage = e => {
  if (e.data == 'clientloaded') {
    clients.matchAll().then(clients => {
      clients.forEach(c => {
        if (c.id == client_id) {
          c.postMessage('fromnotificationclick');
        }
      });
    });
  }
};

@smaug----
Copy link

I think we need some kind of explicit ready() on window side, which would resolve the promise
and probably also implicit ready() at page 'load' event time if the explicit ready() hasn't been used.
The current setup is just way too racy and guaranteed to cause issues on slow networks etc.

@jakearchibald
Copy link
Contributor

F2F:

We should buffer messages until startMessages is invoked

  • Page onload invokes startMessages
  • Assignment to navigator.serviceWorker.onmessage invokes startMessages
  • Invocation of navigator.serviceWorker.startMessages() (TODO: bikeshed) invokes startMessages

No change on when openWindow resolves.

@smaug----
Copy link

(I'm slightly against making yet another broken onfoo event handler, but I guess since MessagePort already has similarly broken onmessage, doesn't matter too much.)

@jakearchibald
Copy link
Contributor

FWIW I didn't know about the onmessage assignment thing and I consider it to be batshit. I'd much rather we just admitted that in some cases listener adding can have side-effects.

@jakearchibald jakearchibald added this to the Version 1 milestone Oct 28, 2015
@jakearchibald
Copy link
Contributor

F2F resolution: startMessages it is.

@jakearchibald
Copy link
Contributor

I'd like us to consider allowing addEventListener to also trigger startMessages, then we don't need to expose startMessages.

@mkruisselbrink
Copy link
Collaborator

If addEventListener triggers startMessages you can't have multiple event listeners anymore. A library setting an event listener will prevent a later (but before document load) added event listener from other code to receive messages that were send before the listener was added.

@aykevl
Copy link
Author

aykevl commented Apr 11, 2016

EDIT: nevermind, this has mostly been suggested before.

Just an idea: triggering startMessages on load or DOMContentLoaded.
This may not be a good solution either: libraries may want to do initialization in DOMContentLoaded and onload may be too late.
Other ideas:

  • Do startMessages immediately after DOMContentLoaded without exposing startMessages.
  • Expose startMessages, but do it anyway after onload in case the web app hasn't requested for it specifically.

@wanderview
Copy link
Member

@wanderview
Copy link
Member

We decided to keep the previous f2f decision. Messages are automatically started on DOMContentLoaded, but if you want messages before that you need to call startMessages(). The onmessage assignment also implicitly calls startMessages().

@jungkees
Copy link
Collaborator

@annevk I'm thinking of defining a task source called client message queue on Window object and WorkerGlobalScope object. Then, client.postMessage() add a task to the client message queue which is consumed by the even loop of those globals when the task source is enabled (like what MessagePort does). Enabling the task source can be done by invoking the start client message algorithm from the three call sites: on DOMContentLoaded (or onload?), onmessage assignment, and navigator.serviceWorker.startMessages().

I was thinking of defining the task source in the service worker client which is a type of environment settings object, but then it can't cover the DOMContentLoaded case.

If the above outline makes sense, I'll make a PR to HTML.

/cc @domenic

@annevk
Copy link
Member

annevk commented Jun 20, 2016

Sounds reasonable.

jungkees added a commit to jungkees/html that referenced this issue Jul 7, 2016
A ServiceWorkerContainer object has a client message queue task
source, initially disabled, which holds tasks until the client is
ready to get the messages from its service worker. This patch adds a
condition that enables the task source:
 - when a document is dispatched a DOMContentLoaded event
 - when a worker client has executed run a worker algorithm

Related Issue: w3c/ServiceWorker#728
Related commit (WIP): w3c/ServiceWorker@c2db88b
@jungkees
Copy link
Collaborator

jungkees commented Jul 7, 2016

I made patches as follows:

I defined the task source in ServiceWorkerContainer object instead of the global objects. I think it's better since a ServiceWorkerContainer object is the target object at which the message event is dispatched, and we don't pollute the globals. Please review them.

jungkees added a commit to jungkees/html that referenced this issue Jul 8, 2016
A ServiceWorkerContainer object has a client message queue task
source, initially disabled, which holds tasks until the client is
ready to get the messages from its service worker. This patch adds a
condition that enables the task source:
 - when a document is dispatched a DOMContentLoaded event
 - when a worker client has executed run a worker algorithm

Related Issue: w3c/ServiceWorker#728
Related commit (WIP): w3c/ServiceWorker@c2db88b
jungkees added a commit to jungkees/html that referenced this issue Jul 8, 2016
A ServiceWorkerContainer object has a client message queue task
source, initially disabled, which holds tasks until the client is
ready to get the messages from its service worker. This patch adds a
condition that enables the task source:
 - when a document is dispatched a DOMContentLoaded event
 - when a worker client has executed run a worker algorithm

Related Issue: w3c/ServiceWorker#728
Related commit: w3c/ServiceWorker@5c0ecae
jungkees added a commit to jungkees/html that referenced this issue Jul 22, 2016
A ServiceWorkerContainer object has a client message queue task
source, initially disabled, which holds tasks until the client is
ready to get the messages from its service worker. This patch adds a
condition that enables the task source:
 - when a document is dispatched a DOMContentLoaded event
 - when a worker client has executed run a worker algorithm

Related Issue: w3c/ServiceWorker#728
Related commit: w3c/ServiceWorker@5c0ecae
annevk pushed a commit to whatwg/html that referenced this issue Jul 22, 2016
A ServiceWorkerContainer object has a client message queue task source, initially disabled, which holds tasks until the client is ready to get the messages from its service worker. This patch adds a condition that enables the task source:

* when DOMContentLoaded is dispatched
* when a worker client has executed run a worker algorithm

Related issue: w3c/ServiceWorker#728
Related commit: w3c/ServiceWorker@5c0ecae
@jungkees
Copy link
Collaborator

Addressed by 5c0ecae and whatwg/html@d615947

Closing.

annevk pushed a commit to whatwg/html that referenced this issue Jul 25, 2016
A ServiceWorkerContainer object has a client message queue task source, initially disabled, which holds tasks until the client is ready to get the messages from its service worker. This patch adds a condition that enables the task source:

* when DOMContentLoaded is dispatched
* when a worker client has executed run a worker algorithm

Related issue: w3c/ServiceWorker#728
Related commit: w3c/ServiceWorker@5c0ecae
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
A ServiceWorkerContainer object has a client message queue task source, initially disabled, which holds tasks until the client is ready to get the messages from its service worker. This patch adds a condition that enables the task source:

* when DOMContentLoaded is dispatched
* when a worker client has executed run a worker algorithm

Related issue: w3c/ServiceWorker#728
Related commit: w3c/ServiceWorker@5c0ecae
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants