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

Define dedicated worker owner document #6379

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

domenic
Copy link
Member

@domenic domenic commented Feb 11, 2021

This is upstreamed from https://wicg.github.io/page-lifecycle/#dedicatedworkerglobalscope-owning-document, with some refinements and assertions.

It would also be useful for WICG/serial#116 and WICG/nav-speculation#36.


/workers.html ( diff )

This is upstreamed from https://wicg.github.io/page-lifecycle/#dedicatedworkerglobalscope-owning-document, with some refinements and assertions (notably we've since made sure you can't nest dedicated workers inside shared workers).

<li><p>If <var>owner</var> is a <code>Document</code>, then return <var>owner</var>.</p></li>

<li><p>Assert: <var>owner</var> is a <code>DedicatedWorkerGlobalScope</code>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

Why is this true? Note: [Exposed=(Window,DedicatedWorker,SharedWorker)].

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I was looking at the wrong [Exposed]. OK then. (Although, note that the assert does hold in Chromium and WebKit.)

It seems like then we have to consider what callers would want in these cases. I think we have two options. First, let's say the algorithm returns null. Then:

  • Page Lifecycle does not freeze such workers, like it is specced to do today.

  • Web Serial (and presumably other APIs which are exposed in workers but want to do "allowed to use" checks on their owner document) would just disallow usage of the feature.

  • The prerendering restrictions would likely not be applicable since the features would be disallowed anyway in such contexts and not need extra restrictions while prerendering.

Another option is to have it return an owner document set containing all such owners. Then:

  • Page Lifecycle could either do its current behavior, or it could do something more complicated by freezing such workers if and only if all their owning documents are frozen. (Probably in such a case the shared workers should also get frozen though?)

  • Web Serial/etc. could either disallow usage of the feature, or it could check that all of its owners are allowed to use the feature. (But, that would still raise questions about what sites are allowed to get the ports... Maybe this is Define permission storage mechanism WICG/serial#107.)

  • Prerendering restrictions would follow the features in question.

Practically speaking, I think returning null to signify "multiple owner documents" and having features bail out in such cases would be best, especially given that it's a Gecko-only concern. WDYT?

/cc @mfalken @reillyeon

Copy link
Member

@annevk annevk Feb 12, 2021

Choose a reason for hiding this comment

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

For Page Lifecycle the question is indeed what happens for shared workers.

We had responsible document for this before btw, but I don't remember what it did for shared workers.

Copy link
Member Author

Choose a reason for hiding this comment

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

responsible document still exists, but it's just "not applicable" for anything except window ESOs.

Copy link
Member

Choose a reason for hiding this comment

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

No longer, but it used to be.

Choose a reason for hiding this comment

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

Deciding what to do with multiple owning Documents is why WebUSB and Web Serial aren't exposed in shared workers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately they kind of are in spec-land (and in Firefox), since dedicated workers can be nested inside shared workers.

@domenic domenic requested a review from annevk February 18, 2021 19:16
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

I think we need to be a bit clearer about what this will be used for, since it cannot be used for CSP or something like that for instance.


<p class="note">In cases where a dedicated worker is nested inside a shared worker, there is no
single unambiguous owner document, so the <span>owner document</span> concept returns null.
Generally, APIs which need to consult the owner document, e.g. to check if it's <span>allowed to
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Generally, APIs which need to consult the owner document, e.g. to check if it's <span>allowed to
Generally, APIs which need to consult the owner document, e.g., to check if it's <span>allowed to

single unambiguous owner document, so the <span>owner document</span> concept returns null.
Generally, APIs which need to consult the owner document, e.g. to check if it's <span>allowed to
use</span> a policy-controlled feature, will just deny access in such scenarios, since the
ownership relationship is too complex to be useful.</p>
Copy link
Member

Choose a reason for hiding this comment

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

What kind of policies are we talking about here? Are any of them mutable?

Copy link
Member

Choose a reason for hiding this comment

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

Also, looking at https://wicg.github.io/page-lifecycle/#get-client-lifecycle-state-algorithm and https://w3c.github.io/ServiceWorker/#dfn-service-worker-client I don't see how this can be restricted to dedicated workers. Clients can be any type of global.

@domenic
Copy link
Member Author

domenic commented Feb 19, 2021

I agree that we need to be clear about how to use this. Ideas are welcome. In general, I was hesitant to introduce this since I could see it being misused. But the problem is that in the cases where it should be used, it's so difficult to get right, so having a wrapper algorithm is pretty necessary.

What kind of policies are we talking about here? Are any of them mutable?

In the sentence you commented on, I'm specifically referring to permissions policy checks ("policy-controlled features"). Web serial is where I encountered this problem most recently, although there's probably more.

Another interesting case might be permissions API checks? Those would be mutable, I suppose.

The page lifecycle integration is another case, where it's relying on the fact that only windows interact with the user and can be cleanly classified as frozen/unfrozen. There, it wants to propagate that frozenness to dedicated workers which the window unambiguously owns.

Also, looking at https://wicg.github.io/page-lifecycle/#get-client-lifecycle-state-algorithm and https://w3c.github.io/ServiceWorker/#dfn-service-worker-client I don't see how this can be restricted to dedicated workers. Clients can be any type of global.

The intention is that non-window/dedicated worker clients count as "active" at all times. I think the bug is that #get-client-lifecycle-state-algorithm should null check owner/owning document.

@annevk
Copy link
Member

annevk commented Feb 22, 2021

Isn't the problem that it assumes owning document is defined for all globals on the caller side and the callee only defines it for dedicated workers?

I guess permissions still work, but only as long as you don't allow the page to revoke them (which is a thing Google has suggested to add and other browsers objected to, if memory serves). Once you allow manipulation of policies by the web developer, this mechanism seems inadequate to define the observable behavior. Maybe that should be called out in a note.

@domenic
Copy link
Member Author

domenic commented Feb 23, 2021

Isn't the problem that it assumes owning document is defined for all globals on the caller side and the callee only defines it for dedicated workers?

If you're talking about page lifecycle, then yes, this is the problem. We'd fix this by having the caller do a null check on the result.

I guess permissions still work, but only as long as you don't allow the page to revoke them (which is a thing Google has suggested to add and other browsers objected to, if memory serves). Once you allow manipulation of policies by the web developer, this mechanism seems inadequate to define the observable behavior. Maybe that should be called out in a note.

I don't quite understand the scenario you're envisioning. Let me try to write it out:

  • Inside the worker part of the spec, e.g. doThingWithDeviceFromWorker() method steps, it says: "access the device if my owner document is non-null and my owner document has permission to access the device."

  • The document revokes its own permission to access the device.

  • Now future invocations of doThingWithDeviceFromWorker() will fail.

This seems fine, and very similar to the corresponding main-page version:

  • Inside the document part of the spec, e.g. doThingWithDevice() method steps, it says: "access the device if the current global object's associated Document has permission to access the device."

  • The document revokes its own permission to access the device.

  • Now future invocations of doThingWithDevice() will fail.

@annevk
Copy link
Member

annevk commented Feb 24, 2021

If you're assuming the worker has synchronous access, sure, it would play out like that, but if not, there's a race. And it's unclear how that race plays out when you add other communication channels, such as postMessage() or shared memory.

(And for page lifecycle you can't just fix it by null checking the result. You also have to check what client you have as not all of them will have "owning document" defined.)

@domenic
Copy link
Member Author

domenic commented Feb 24, 2021

If you're assuming the worker has synchronous access, sure, it would play out like that, but if not, there's a race. And it's unclear how that race plays out when you add other communication channels, such as postMessage() or shared memory.

In general, anything that crosses process boundaries (e.g., the entire service worker spec and "clients" concept; any permissions checks assuming browsers don't implement permissions in their renderer process; etc.) is susceptible to these sorts of races. That's just kind of the cost of doing business in a multi-process world. I'm not sure how it should impact this PR...

(And for page lifecycle you can't just fix it by null checking the result. You also have to check what client you have as not all of them will have "owning document" defined.)

I see, yes, agreed.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Reluctantly approving. While I agree there might be some unavoidable races, in general I think more defined ordering should be possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants