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

Add an option to include frozen documents. #1442

Open
wants to merge 4 commits into
base: master
from

Conversation

@dtapuska
Copy link
Contributor

commented Jun 24, 2019

It is off by default to exclude them when frozen. Since postMessage
will be delayed until the page is unfrozen this is the safest option
to exclude frozen pages by default.


Preview | Diff

@dtapuska

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

@wanderview WDYT?

@wanderview

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

Why do we restrict this to window clients? At least in firefox you can have dedicated workers that are frozen by virtue of their owning document being frozen.

Also, I think if we are adding an ignoreFrozen we should add a client.frozen attribute for completeness.

Also, I'm not sure this adequately captures the issue where frozen clients should not be considered controlled when determining if an active service worker can be discarded. We also probably need to ignore frozen clients in clients.claim(). Not sure if your intent is for those to be handled here or in a separate PR.

Finally, exposing the frozen indicator probably needs some discussion amongst others in the group. We should probably renew discussion in #626.

@dtapuska

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

I've added the frozen attribute to Client and removed the requirement for it to be a WindowClient.

@jakearchibald How can we proceed with this?

@@ -66,6 +66,10 @@ spec: page-visibility; urlPrefix: https://www.w3.org/TR/page-visibility/
type: enum; text: VisibilityState; url: VisibilityState
type: attribute; text: visibilityState; for: Document; url: dom-document-visibilitystate

spec: page-lifecycle; urlPrefix: https://wicg.github.io/page-lifecycle/spec.html
type: dfn; text: frozen; url: frozen
type: dfn; text: owning document; url: dedicatedworkerglobalscope-owning-document

This comment has been minimized.

Copy link
@jakearchibald

jakearchibald Jun 27, 2019

Contributor

@tabatkins can we get https://wicg.github.io/page-lifecycle/spec.html added to bikeshed's database?

This comment has been minimized.

Copy link
@tabatkins

tabatkins Jun 27, 2019

Member

Should be done, give it an hour or so.

@@ -1050,6 +1055,8 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe

A {{Client}} object has an associated <dfn id="dfn-service-worker-client-frame-type" for="Client">frame type</dfn>, which is one of "`auxiliary`", "`top-level`", "`nested`", and "`none`". Unless stated otherwise it is "`none`".

A {{Client}} object has an associated <dfn id="dfn-service-worker-client-frozen" for="Client">frozen state</dfn>, which if a {{WindowClient}} should reflect [=Client/service worker client=]'s [=responsible document=] [=frozen=] state, otherwise it should reflect [=Client/service worker client=]'s [=environment settings object/global object=]'s [=owning document=] [=frozen=] state.

This comment has been minimized.

Copy link
@jakearchibald

jakearchibald Jun 27, 2019

Contributor

Client objects are snapshots of state – their values never update.

  • Change this to a simple boolean.
  • Add a getter.
  • Update Create Client and Create Window Client to set the frozen value.
  • Update Create Client and Create Window Client call sites to provide the correct frozen value (similar to what you're currently doing above).

https://w3c.github.io/ServiceWorker/#dfn-service-worker-client-focusstate might be a good reference for how this should work.

@@ -1248,6 +1256,7 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
1. If |client|'s [=environment/execution ready flag=] is unset or |client|'s [=discarded flag=] is set, [=continue=].
1. If |client| is not a [=secure context=], [=continue=].
1. If |options|["{{ClientQueryOptions/includeUncontrolled}}"] is false, and if |client|'s [=active service worker=] is not the associated [=ServiceWorkerGlobalScope/service worker=], [=continue=].
1. If |options|["{{ClientQueryOptions/includeFrozen}}"] is false, |client| is {{Client/frozen}}, [=continue=].

This comment has been minimized.

Copy link
@jakearchibald

jakearchibald Jun 27, 2019

Contributor

|client| is a [=service worker client=], not a {{Client}}, so {{Client/frozen}} doesn't make sense here.

At this point you need to determine if the client is actually frozen or not. Also, be wary of race conditions here. Ensure that, if includeFrozen is false, no frozen clients are returned.

@jakearchibald

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2019

@aliams @asutherland @youennf @cdumez are you happy for client objects to reflect state defined in the page lifecycle spec?

It seems to make sense to be, but I don't know how other browsers feel about page lifecycle.

@annevk

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

@EricRahm since you've been looking at Page Lifecycle you might wanna check this too.

(Mozilla's position on Page LIfecycle is tracked at mozilla/standards-positions#87 and should be updated soonish.)

@jakearchibald

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

Some more thoughts:

Are we ever going to expose discarded top level window clients? If the user clicks a notification, it's desirable to focus a discarded tab rather than create a new one. If we might expose this, we should think about the API here, since a client cannot be both frozen and discarded.

@wanderview

Also, I'm not sure this adequately captures the issue where frozen clients should not be considered controlled when determining if an active service worker can be discarded.

Is this true? I know that discarded clients aren't considered controlled, but I'm less sure about frozen ones. What happens when they become unfrozen? It feels like we need to consider them controlled, or discard them so it doesn't matter.

@dtapuska what happens to tasks queued against a frozen page? As in, if I postMessage a frozen tab (using service worker clients or BroadcastChannel), are they queued up or discarded?

@dtapuska

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

I think it totally make sense to support showing discarded clients in these views. Since from the user perspective the tab is still there it just has to be reloaded.

Whether the need to call out frozen from discarded windows is necessary I don't know. postMessage to a frozen window will get queued until it is unfrozen. postMessage obviously doesn't exist for discarded windows at this time but I'd probably expect them to discard. If the client is a top level window then focus will unfreeze the window.

@jakearchibald

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

Ok, so a client object should probably have a single property (lifecycle state?) that is active, frozen or discarded.

When we get client objects, I can't decide if we should be providing frozen clients by default. Right now, matchAll returns all clients controlled by this service worker. If a frozen client will prevent a waiting worker from activating, it feels like matchAll should return it by default.

Discarded clients shouldn't be returned by default.

@annevk

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

It sounds like discarded is different from a removed iframe or a closed tab, for which we also use discarded as terminology. How are those exposed today? (Should we have different terminology?)

@wanderview

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

Is this true? I know that discarded clients aren't considered controlled, but I'm less sure about frozen ones. What happens when they become unfrozen? It feels like we need to consider them controlled, or discard them so it doesn't matter.

We discussed this previously in the context of bfcache. At the time we decided that clients frozen in bfcache should not be considered controlled. When they thaw and come out of bfcache they become controlled again. If a new service worker version has activated while they were frozen then the frozen client should be evicted from bfcache and reloaded.

I think all that still makes sense. Can we apply the last part about eviction and reloading to clients frozen in background tabs instead of bfcache?

@jakearchibald

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

@annevk

Just saw https://wicg.github.io/page-lifecycle/spec.html#html-discarding.

Yeah, I don't think the clients API cares about what the HTML spec calls "discarded". Whereas with page lifecycle's "discarded", there's still UI representing the tab, which will reload if the user makes it visible.

@wanderview

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

Note, at the time we were discussing frozen bfcache clients page lifecycle spec stuff did not exist. So we couldn't express this in the spec since bfcache was just a "browser optimization".

@jakearchibald

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

Can we apply the last part about eviction and reloading to clients frozen in background tabs instead of bfcache?

The only difference here is the client is getting queued stuff (eg via postMessage). @dtapuska do you think it's ok if we 'discard' frozen clients if they're blocking a service worker update?

@wanderview

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

Alternatively, now that we have events that fired when unfrozen we could also fire a controllerchange event at the same place.

I guess the one we don't have a good event for right now is when the SW is unregistered while frozen. In that case we would have to fire a controllerchange to null.

@jakearchibald

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

Alternatively, now that we have events that fired when unfrozen we could also fire a controllerchange event at the same place.

Right now we only change controllers if the developer opts into it via clients.claim(). At some point clear-site-data will do the same, but this is still a developer action. Would it be weird to do it for frozen clients like this?

I guess we also change controllers in case of corruption, but this is a disaster case.

@wanderview

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

Or maybe bfcache freezing should be a different page lifecycle from other types of freezing. If a tab has a frozen document outside of bfcache we could in theory have it just remain controlled. Its hard to do that as long as we treat bfcache and the iframe type freezing identically.

@wanderview

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

I spoke with Dave and I think I've been conflating things a bit.

Its true that we mark documents going into bfcache as frozen, but that is not adequate to describe bfcache behavior as a whole. There is more work needed to fully describe bfcache.

In this case there is a semantic difference between a frozen client in a background tab the user hasn't visited in a week and a frozen client in bfcache. The user can see the background tab exists and focus it. Conceptually the client exists from the user's perspective. In bfcache, though, they have navigated away from the frozen client, can't see it exists any more, and can't focus it without a history operation.

I think this means our previous discussions about bfcache still hold true, but that they should not apply to all frozen clients. We need to spec the additional constraints on bfcache above and beyond the frozen state.

For the purposes of this PR I recommend adding a non-normative not indicating that, while clients in bfcache are frozen, they should not be exposed to clients.matchAll(), etc. This can then be improved when bfcache is more fully spec'd.

WDYT?

@dtapuska

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

I think bfcache and frozen clients are kind of different beasts here. Items that are in the bfcache shouldn't appear controlled or uncontrolled to a service worker. So the service worker should be free to update while something is in the bfcache. If the bfcache needs to evict and reload, or send a controllerchanged when it is restored from the bfcache those seem like reasonable approaches.

For discarded top level windows, they shouldn't be controlled but appear as an uncontrolled client so it could be focused.

For frozen top level windows, and frozen iframes I believe they should still appear to a service worker and probably still block updating the service worker because we expect these things to be temporary (an open tab in these states would block it as well, and it is no different). It presumably is useful to know what the state of the document is in. Perhaps we just add a lifecycle attribute to Client and let matchAll return them all. Then that array based on state if it so chooses.

It is off by default to exclude them when frozen. Since postMessage
will be delayed until the page is unfrozen this is the safest option
to exclude frozen pages by default.
@dtapuska dtapuska force-pushed the dtapuska:master branch from 8ed2165 to a5bc071 Jul 8, 2019
@dtapuska

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

@jakearchibald WDYT. I've adjusted the approach to return a ClientState which is an enum of "active", "frozen" with desire in the future to extend it to include "discarded".

@dtapuska

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

@wanderview I believe bfcache is already covered in service worker spec via this text: "If client is a window client and client’s responsible document is not browsingContext’s active document, then set isClientEnumerable to false and abort these steps." Since a document in bfcache won't be the active document for the browsingContext.

Copy link
Contributor

left a comment

This is looking much better. A few comments/nits:

:: |state|, a string

1. Let |state| be {{ClientState/"active"}}.
1. If |client|'s [=environment settings object/global object=]'s [=owning document=] is [=frozen=], set |state| to be {{ClientState/"frozen"}}.

This comment has been minimized.

Copy link
@jakearchibald

jakearchibald Jul 12, 2019

Contributor

Since the client is an environment, can you just do "client's responsible document"?

This comment has been minimized.

Copy link
@dtapuska

dtapuska Jul 12, 2019

Author Contributor

Client is an enviornment not necessarily an environment settings object (which defines responsible document). But if you think that is clear I'm fine with that.

This comment has been minimized.

Copy link
@jakearchibald

jakearchibald Jul 15, 2019

Contributor

Ah good point. I think it should be fine.

docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
"frozen",
"all"
};
</pre>

This comment has been minimized.

Copy link
@jakearchibald

jakearchibald Jul 12, 2019

Contributor

I think we should break this out into multiple enums, since a client can never have a state of "all".

I think enums can inherit, so ClientStateQuery (which adds "all") can inherit from ClientState.

This comment has been minimized.

Copy link
@dtapuska

dtapuska Jul 12, 2019

Author Contributor

I can't get enums to inherit.. I don't see any that do that in WPT . Either way added a separate definition

dtapuska added 2 commits Jul 12, 2019
- Separate enums
- Remove owner document reference
- Fix links
@asutherland

This comment has been minimized.

Copy link

commented Jul 15, 2019

Restating my understanding of this issue:

  • Per spec, currently frozen clients will be returned by matchAll.
    • As explicitly stated in the initiating comment, this is not desirable from a postMessage perspective because the tasks for the client are disabled and the message payloads will stack up, effectively leaking memory. Depending on browser behavior, this may cause the client to be discarded more aggressively. This is a similar issue to BroadcastChannel or use of MessageChannel, but those APIs don't have visibility into the state of the client.
    • Being able to see frozen clients is desirable for ServiceWorkers that handle push notifications and preferentially invoke Client.focus() instead of using Clients.openWindow() to avoid tab proliferation.
  • This issue doesn't do anything to expose page-lifecyle "discarded" clients which could correspond to unloaded tabs (a mechanism that Firefox exposes to WebExtensions) or not-yet-loaded tabs (as Firefox's lazy session restore does).
    • The previously mentioned Client.focus()/Clients.openWindow() would like this.
  • At the same time, this is explicitly exposing the concept of frozen pages to the spec.

It seems reasonable to me to filter out the frozen clients by default. Given that this is also exposing a new concept, I think it would be desirable to either directly provide some non-normative guidance on the implications of frozen clients or link to some in the page-lifecycle not-yet-spec.

In particular, I think ServiceWorker authors would be interested in the not-immediately obvious fact that frozen clients won't process the message until unfrozen. And it would be good to make clear what the expected outcome of spamming postMessage() at a frozen client is. Is it a nefarious trick they can use to get their client unfrozen? Or is it expected that such postMessage() calls will hasten the discarding of the client?

@dtapuska

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

I don't really know the answers..

Is it a nefarious trick they can use to get their client unfrozen?
It may be subject to UA policies of freezing and unfreezing. I know there was some discussions in Chrome if the browser should periodically unfreeze the client.

Or is it expected that such postMessage() calls will hasten the discarding of the client?
In Chrome, postMessages may cause your own death sooner not necessarily discarding the client. But again there was some thought that a client should transition to discarded if it gets too many postMessages queued.

Do you expect these to be covered in the Service Worker spec, I'd think these should be described maybe in the page lifecycle spec because you could postMessage or broadcastChannel to a frame with window handles or message channels so it isn't directly tied to ServiceWorkers.

@asutherland

This comment has been minimized.

Copy link

commented Jul 15, 2019

I think describing in the page lifecycle spec with appropriate (probably non-normative) links from the ServiceWorker spec makes sense.

@youennf

This comment has been minimized.

Copy link

commented Jul 15, 2019

  • As explicitly stated in the initiating comment, this is not desirable from a postMessage perspective because the tasks for the client are disabled and the message payloads will stack up, effectively leaking memory.

Given a multi-process architecture, I am not sure how we can make sure that any client, believed to be frozen or unfrozen at matchAll resolution time, or postMessage call time, will not get frozen/discarded anyway before the postMessage task reaches the client.
If this is the real problem, maybe we should fix it at postMessage(), navigate(), openWindow()... level.

Also, I am not sure the concept of a discarded service worker client makes total sense since it is more related with the concept of a page. Is there any use in knowing whether iframe clients or worker clients are discarded for instance?

I get it that one might want to optimize the number of opened tabs.
There are a few caveats though. At least in Safari, a private browsing tab is in a different session than another private browsing tab. These tabs will have different service workers, even though the user might see these as the same web site.

When launching a browser, tabs might be visible but pages are not loaded.
Exposing information of these tabs to a service worker is exposing state information that might persist, even if for instance user decides to clear all website data (cookies, IDB...) before launching the browser.

It seems to make sense to be, but I don't know how other browsers feel about page lifecycle.

I'll try to get some feedback from Safari people.
My initial reaction is that this might not be a highly desirable feature.
If a tab gets frozen to preserve battery, why fire an event that will end up executing JS thus decrease the battery life and potentially allow the page to try not being frozen?
This might also be difficult to implement consistently.

dtapuska added a commit to dtapuska/page-lifecycle that referenced this pull request Jul 16, 2019
As discussed on w3c/ServiceWorker#1442

Unfortunately adoption of page-lifecycle is yet to be formally
supported by other vendors so we need to monkey patch this in the
page lifecycle spec.
domenic added a commit to dtapuska/page-lifecycle that referenced this pull request Jul 31, 2019
As discussed on w3c/ServiceWorker#1442

Unfortunately adoption of page-lifecycle is yet to be formally
supported by other vendors so we need to monkey patch this in the
page lifecycle spec.
domenic added a commit to dtapuska/page-lifecycle that referenced this pull request Jul 31, 2019
As discussed on w3c/ServiceWorker#1442

Unfortunately adoption of page-lifecycle is yet to be formally
supported by other vendors so we need to monkey patch this in the
page lifecycle spec.
domenic added a commit to WICG/page-lifecycle that referenced this pull request Aug 7, 2019
As discussed on w3c/ServiceWorker#1442

Unfortunately adoption of page-lifecycle is yet to be formally
supported by other vendors so we need to monkey patch this in the
page lifecycle spec.
@mattto mattto referenced this pull request Aug 19, 2019
21 of 51 tasks complete
chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Aug 21, 2019
Add code to support handling of frozen clients. Frozen clients do not
run their event loop, so postMessage to them just causes problems. To
allow service workers to continue working with frozen windows we expose
includeFrozen in the matchAll and the frozen attribute on the Client. If
a service worker calls focus on a client it will unfreeze the window when
it is moved to have focus.

This is specified in https://wicg.github.io/page-lifecycle/ and
w3c/ServiceWorker#1442

BUG=957597

Change-Id: I6abe1882e88c65dac99250db5bb7fa8d3a4b2b1d
chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Aug 21, 2019
Add code to support handling of frozen clients. Frozen clients do not
run their event loop, so postMessage to them just causes problems. To
allow service workers to continue working with frozen windows we expose
includeFrozen in the matchAll and the frozen attribute on the Client. If
a service worker calls focus on a client it will unfreeze the window when
it is moved to have focus. This feature is currently marked as
experimental and an intent to ship will be sent.

This is specified in https://wicg.github.io/page-lifecycle/ and
w3c/ServiceWorker#1442

BUG=957597

Change-Id: I6abe1882e88c65dac99250db5bb7fa8d3a4b2b1d
chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Aug 21, 2019
Add code to support handling of frozen clients. Frozen clients do not
run their event loop, so postMessage to them just causes problems. To
allow service workers to continue working with frozen windows we expose
includeFrozen in the matchAll and the frozen attribute on the Client. If
a service worker calls focus on a client it will unfreeze the window when
it is moved to have focus. This feature is currently marked as
experimental and an intent to ship will be sent.

This is specified in https://wicg.github.io/page-lifecycle/ and
w3c/ServiceWorker#1442

BUG=957597

Change-Id: I6abe1882e88c65dac99250db5bb7fa8d3a4b2b1d
chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Aug 22, 2019
Add code to support handling of frozen clients. Frozen clients do not
run their event loop, so postMessage to them just causes problems. To
allow service workers to continue working with frozen windows we expose
includeFrozen in the matchAll and the frozen attribute on the Client. If
a service worker calls focus on a client it will unfreeze the window when
it is moved to have focus. This feature is currently marked as
experimental and an intent to ship will be sent.

This is specified in https://wicg.github.io/page-lifecycle/ and
w3c/ServiceWorker#1442

BUG=957597

Change-Id: I6abe1882e88c65dac99250db5bb7fa8d3a4b2b1d
chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Aug 22, 2019
Add code to support handling of frozen clients. Frozen clients do not
run their event loop, so postMessage to them just causes problems. To
allow service workers to continue working with frozen windows we expose
includeFrozen in the matchAll and the frozen attribute on the Client. If
a service worker calls focus on a client it will unfreeze the window when
it is moved to have focus. This feature is currently marked as
experimental and an intent to ship will be sent.

This is specified in https://wicg.github.io/page-lifecycle/ and
w3c/ServiceWorker#1442

BUG=957597

Change-Id: I6abe1882e88c65dac99250db5bb7fa8d3a4b2b1d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1677065
Commit-Queue: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#689558}
aarongable pushed a commit to chromium/chromium that referenced this pull request Aug 22, 2019
Add code to support handling of frozen clients. Frozen clients do not
run their event loop, so postMessage to them just causes problems. To
allow service workers to continue working with frozen windows we expose
includeFrozen in the matchAll and the frozen attribute on the Client. If
a service worker calls focus on a client it will unfreeze the window when
it is moved to have focus. This feature is currently marked as
experimental and an intent to ship will be sent.

This is specified in https://wicg.github.io/page-lifecycle/ and
w3c/ServiceWorker#1442

BUG=957597

Change-Id: I6abe1882e88c65dac99250db5bb7fa8d3a4b2b1d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1677065
Commit-Queue: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#689558}
chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Aug 22, 2019
Add code to support handling of frozen clients. Frozen clients do not
run their event loop, so postMessage to them just causes problems. To
allow service workers to continue working with frozen windows we expose
includeFrozen in the matchAll and the frozen attribute on the Client. If
a service worker calls focus on a client it will unfreeze the window when
it is moved to have focus. This feature is currently marked as
experimental and an intent to ship will be sent.

This is specified in https://wicg.github.io/page-lifecycle/ and
w3c/ServiceWorker#1442

BUG=957597

Change-Id: I6abe1882e88c65dac99250db5bb7fa8d3a4b2b1d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1677065
Commit-Queue: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#689558}
natechapin added a commit to natechapin/wpt that referenced this pull request Aug 23, 2019
Add code to support handling of frozen clients. Frozen clients do not
run their event loop, so postMessage to them just causes problems. To
allow service workers to continue working with frozen windows we expose
includeFrozen in the matchAll and the frozen attribute on the Client. If
a service worker calls focus on a client it will unfreeze the window when
it is moved to have focus. This feature is currently marked as
experimental and an intent to ship will be sent.

This is specified in https://wicg.github.io/page-lifecycle/ and
w3c/ServiceWorker#1442

BUG=957597

Change-Id: I6abe1882e88c65dac99250db5bb7fa8d3a4b2b1d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1677065
Commit-Queue: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#689558}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Aug 27, 2019
… out frozen windows., a=testonly

Automatic update from web-platform-tests
Add ability for service worker to filter out frozen windows.

Add code to support handling of frozen clients. Frozen clients do not
run their event loop, so postMessage to them just causes problems. To
allow service workers to continue working with frozen windows we expose
includeFrozen in the matchAll and the frozen attribute on the Client. If
a service worker calls focus on a client it will unfreeze the window when
it is moved to have focus. This feature is currently marked as
experimental and an intent to ship will be sent.

This is specified in https://wicg.github.io/page-lifecycle/ and
w3c/ServiceWorker#1442

BUG=957597

Change-Id: I6abe1882e88c65dac99250db5bb7fa8d3a4b2b1d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1677065
Commit-Queue: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#689558}

--

wpt-commits: 3d052c54485970c1673500c1ba7d52182adf6f4f
wpt-pr: 17554
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Aug 27, 2019
… out frozen windows., a=testonly

Automatic update from web-platform-tests
Add ability for service worker to filter out frozen windows.

Add code to support handling of frozen clients. Frozen clients do not
run their event loop, so postMessage to them just causes problems. To
allow service workers to continue working with frozen windows we expose
includeFrozen in the matchAll and the frozen attribute on the Client. If
a service worker calls focus on a client it will unfreeze the window when
it is moved to have focus. This feature is currently marked as
experimental and an intent to ship will be sent.

This is specified in https://wicg.github.io/page-lifecycle/ and
w3c/ServiceWorker#1442

BUG=957597

Change-Id: I6abe1882e88c65dac99250db5bb7fa8d3a4b2b1d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1677065
Commit-Queue: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#689558}

--

wpt-commits: 3d052c54485970c1673500c1ba7d52182adf6f4f
wpt-pr: 17554
@dtapuska

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

I think describing in the page lifecycle spec with appropriate (probably non-normative) links from the ServiceWorker spec makes sense.

We've made the changes in the page lifecycle spec. We've chosen just to expose the lifecycleState on the client and not introduce any form of filtering to matchAll at this time.

@jakearchibald

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2019

For TPAC:

@jakearchibald

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

Resolution:

  • Find out WebKit's position on lifecycle
  • Create way to attach clonable data to clients
    • AI Jake: File issue
    • What to do about size restrictions?
    • Group still a little keen on clients API in clients.
  • Find a way to get "delivered" info from postMessage.
  • Avoid adding frozen data to clients for now.
  • Frozen clients will appear in clients.matchAll.
  • Frozen clients will prevent SW activation.
@wanderview

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

For the record I just want to state I find the pushback on Client.frozen attribute a bit frustrating.

I acknowledge that using it has a small race condition, but the service worker group specifically accepted that design when it chose to make Client object a snapshot years ago. I believe this was a tradeoff to avoid the expensive operations required for live updated attributes, etc. I hope we don't have to re-litigate the Client object design every time we want to add a boolean flag to Client.

Also, freeze state is not changed frequently. The probability of the race in checking Client.frozen actually manifesting is quite small. Meanwhile, in the majority of cases the attribute could provide real benefit.

Finally, adding Client.frozen does not preclude other possible changes to postMessage or other new APIs. If the race condition issue actually manifests as a problem in practice we still have options.

Given two browser engines are in favor I would hope we could proceed with Client.frozen.

@jakearchibald

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

@wanderview are you supporting bringing it into the service worker spec, or keeping it in the page lifecycle spec?

@jakearchibald

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

Oh also, @annevk suggested it would be bad to have postMessage indicate delivery success/failure, due to GC exposure I think,

@wanderview

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

@wanderview are you supporting bringing it into the service worker spec, or keeping it in the page lifecycle spec?

It means Client.frozen? Yes, I think we should bless this as official service worker spec behavior. Maybe I misunderstand the implications of monkey-patching vs bringing it into the service worker spec.

@youennf

This comment has been minimized.

Copy link

commented Sep 17, 2019

From what I heard, I am not convinced Client.frozen is the best way to solve the issues we talked about.
@wanderview, is your concern the added burden of maintaining the service worker specific bits in the page life cycle API spec?

gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 4, 2019
… out frozen windows., a=testonly

Automatic update from web-platform-tests
Add ability for service worker to filter out frozen windows.

Add code to support handling of frozen clients. Frozen clients do not
run their event loop, so postMessage to them just causes problems. To
allow service workers to continue working with frozen windows we expose
includeFrozen in the matchAll and the frozen attribute on the Client. If
a service worker calls focus on a client it will unfreeze the window when
it is moved to have focus. This feature is currently marked as
experimental and an intent to ship will be sent.

This is specified in https://wicg.github.io/page-lifecycle/ and
w3c/ServiceWorker#1442

BUG=957597

Change-Id: I6abe1882e88c65dac99250db5bb7fa8d3a4b2b1d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1677065
Commit-Queue: Ken Buchanan <kenrbchromium.org>
Reviewed-by: Ken Buchanan <kenrbchromium.org>
Reviewed-by: Matt Falkenhagen <falkenchromium.org>
Cr-Commit-Position: refs/heads/master{#689558}

--

wpt-commits: 3d052c54485970c1673500c1ba7d52182adf6f4f
wpt-pr: 17554

UltraBlame original commit: d34dc9e00ee2c4ca00e2b78bdad1f7c51f68ba3e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 4, 2019
… out frozen windows., a=testonly

Automatic update from web-platform-tests
Add ability for service worker to filter out frozen windows.

Add code to support handling of frozen clients. Frozen clients do not
run their event loop, so postMessage to them just causes problems. To
allow service workers to continue working with frozen windows we expose
includeFrozen in the matchAll and the frozen attribute on the Client. If
a service worker calls focus on a client it will unfreeze the window when
it is moved to have focus. This feature is currently marked as
experimental and an intent to ship will be sent.

This is specified in https://wicg.github.io/page-lifecycle/ and
w3c/ServiceWorker#1442

BUG=957597

Change-Id: I6abe1882e88c65dac99250db5bb7fa8d3a4b2b1d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1677065
Commit-Queue: Ken Buchanan <kenrbchromium.org>
Reviewed-by: Ken Buchanan <kenrbchromium.org>
Reviewed-by: Matt Falkenhagen <falkenchromium.org>
Cr-Commit-Position: refs/heads/master{#689558}

--

wpt-commits: 3d052c54485970c1673500c1ba7d52182adf6f4f
wpt-pr: 17554

UltraBlame original commit: d34dc9e00ee2c4ca00e2b78bdad1f7c51f68ba3e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 4, 2019
… out frozen windows., a=testonly

Automatic update from web-platform-tests
Add ability for service worker to filter out frozen windows.

Add code to support handling of frozen clients. Frozen clients do not
run their event loop, so postMessage to them just causes problems. To
allow service workers to continue working with frozen windows we expose
includeFrozen in the matchAll and the frozen attribute on the Client. If
a service worker calls focus on a client it will unfreeze the window when
it is moved to have focus. This feature is currently marked as
experimental and an intent to ship will be sent.

This is specified in https://wicg.github.io/page-lifecycle/ and
w3c/ServiceWorker#1442

BUG=957597

Change-Id: I6abe1882e88c65dac99250db5bb7fa8d3a4b2b1d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1677065
Commit-Queue: Ken Buchanan <kenrbchromium.org>
Reviewed-by: Ken Buchanan <kenrbchromium.org>
Reviewed-by: Matt Falkenhagen <falkenchromium.org>
Cr-Commit-Position: refs/heads/master{#689558}

--

wpt-commits: 3d052c54485970c1673500c1ba7d52182adf6f4f
wpt-pr: 17554

UltraBlame original commit: d34dc9e00ee2c4ca00e2b78bdad1f7c51f68ba3e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.