Remove embedder-defined Isolated Context [chromium/src : main]
Andrew Rayskiy (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 1 comment![]( "Open in Gerrit")
Hey Camille, PTAL at `content/public/` files as well as `navigation_request.cc`.
Related details
- Camille Lamy
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot voted Code-Review+1![]( "Open in Gerrit")
- Andrew Rayskiy
- Camille Lamy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Camille Lamy (Gerrit)
Camille Lamy added 6 comments![]( "Open in Gerrit")
Thanks! Adding Chris for a question about the change to direct sockets availability affecting PNA.
Chris, for the context, the IWA team is splitting the way access to Direct Sockets is granted to contexts. Previously, Chrome Apps would force enable the IsolatedContext state. Now, they instead just force enable the access to Direct Sockets only. As part of this change, there is a modification in this CL that seems to bypass the PNA permission request when the Direct Sockets have been enabled for Chrome Apps (see comment in content/browser/direct_sockets/direct_sockets_service_impl.cc). Could you have a look and validate whether it makes sense to do this change? Thanks!
Beyond the PNA question, I also have a more general question about the access to Direct Sockets from worker contexts. My read of the CL is that the embedder never gives access to ServiceWorkers. Is that correct? What happens to SharedWorkers and DirectWorkers? And worklet contexts?
bool AreDirectSocketsAllowedByEmbedder(RenderFrameHost* rfh) {nit: For readability, I wonder if it would be possible to have a DirectSocketsPolicy::DirectSocketsAllowedByEmbedderForOrigin static method somewhere that does this and can be used here and in NavigationRequest. This is a nit, so feel free to ignore it.
if (AreDirectSocketsAllowedByEmbedder(rfh)) {Is it possible to find yourself in a situation where DirectSockets are enabled both by the embedder and because the context is isolated?
if (AreDirectSocketsAllowedByEmbedder(rfh)) {I think this one should be looked at. Adding chthomp to the review to look at the implications for PNA.
} else if (!AreDirectSocketsAllowedByEmbedder(render_frame_host)) {nit: maybe add a comment clarifying what is happening as the else (!) condition is not immediately obvious.
// Embedder-enabled Direct Sockets run in custom contexts and do not rely onIt would be good to validate here that we are in an embedder enabled custom context with an appropriate CHECK.
Related details
- Andrew Rayskiy
- Chris Thompson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 4 comments![]( "Open in Gerrit")
Thanks! Adding Chris for a question about the change to direct sockets availability affecting PNA.
Chris, for the context, the IWA team is splitting the way access to Direct Sockets is granted to contexts. Previously, Chrome Apps would force enable the IsolatedContext state. Now, they instead just force enable the access to Direct Sockets only. As part of this change, there is a modification in this CL that seems to bypass the PNA permission request when the Direct Sockets have been enabled for Chrome Apps (see comment in content/browser/direct_sockets/direct_sockets_service_impl.cc). Could you have a look and validate whether it makes sense to do this change? Thanks!
Beyond the PNA question, I also have a more general question about the access to Direct Sockets from worker contexts. My read of the CL is that the embedder never gives access to ServiceWorkers. Is that correct? What happens to SharedWorkers and DirectWorkers? And worklet contexts?
Attaching my response to the PNA topic here (from another thread):
ChrApps have historically been exempt from any PNA restrictions -- see the now removed `ShouldAllowPrivateNetworkAccessUnconditionally()` call; there's no behavior change here modulo the permissions-policy check which has no effect for ChrApps whatsoever.
Direct Sockets are exposed in Windows and Dedicated, Shared, and Service Workers (see the [.idl](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/direct_sockets/tcp_socket.idl?q=-f:%5Eout%2F%20-f:%5Egen%2F%20-f:%5Esrc%2F%20case:auto%20tcp_socket.idl)), which effectively excludes worklets. However, Shared and Service Workers actually illustrate what happens when a project stalls; we never gained enough momentum to launch the API there due to unresolved permission policy issues. While we had some initial ideas (due to the baseline permissions policy being clearly defined), they never moved forward, leaving us with an unresolved gap :) With that in mind, there's nothing wrong with not moving forward with embedder access in those contexts (as stuff doesn't work there anyway)
if (AreDirectSocketsAllowedByEmbedder(rfh)) {Is it possible to find yourself in a situation where DirectSockets are enabled both by the embedder and because the context is isolated?
No, these two serve different purposes and cover different sets of origins.
if (AreDirectSocketsAllowedByEmbedder(rfh)) {I think this one should be looked at. Adding chthomp to the review to look at the implications for PNA.
ChrApps have historically been exempt from any PNA restrictions -- see the now removed `ShouldAllowPrivateNetworkAccessUnconditionally()` call; there's no behavior change here modulo the permissions-policy check which has no effect for ChrApps whatsoever.
// Embedder-enabled Direct Sockets run in custom contexts and do not rely onIt would be good to validate here that we are in an embedder enabled custom context with an appropriate CHECK.
That's not possible though -- the only signal we get from the browser is whether the runtime flag is enabled or not.
Related details
- Camille Lamy
- Chris Thompson
Camille Lamy (Gerrit)
Camille Lamy added 2 comments![]( "Open in Gerrit")
if (AreDirectSocketsAllowedByEmbedder(rfh)) {Andrew RayskiyIs it possible to find yourself in a situation where DirectSockets are enabled both by the embedder and because the context is isolated?
No, these two serve different purposes and cover different sets of origins.
Acknowledged
// Embedder-enabled Direct Sockets run in custom contexts and do not rely onAndrew RayskiyIt would be good to validate here that we are in an embedder enabled custom context with an appropriate CHECK.
That's not possible though -- the only signal we get from the browser is whether the runtime flag is enabled or not.
- Andrew Rayskiy
- Chris Thompson
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 1 comment![]( "Open in Gerrit")
// Embedder-enabled Direct Sockets run in custom contexts and do not rely onAndrew RayskiyIt would be good to validate here that we are in an embedder enabled custom context with an appropriate CHECK.
Camille LamyThat's not possible though -- the only signal we get from the browser is whether the runtime flag is enabled or not.
And it also gets enabled in the IWA context?
Indeed, that's exactly what happens in `content/browser/renderer_host/navigation_request.cc` -- see the `MutableRuntimeFeatureStateContext()`
Related details
- Camille Lamy
- Chris Thompson
Camille Lamy (Gerrit)
Camille Lamy added 2 comments![]( "Open in Gerrit")
Overall this is good for me, except the interaction with LNA part, on which I am waiting for Chris's opinion. It's quite possible that this it is fine to not check the LNA permission in this case, but I'd like us to explicitly make and document this decision.
// Embedder-enabled Direct Sockets run in custom contexts and do not rely onAndrew RayskiyIt would be good to validate here that we are in an embedder enabled custom context with an appropriate CHECK.
Camille LamyThat's not possible though -- the only signal we get from the browser is whether the runtime flag is enabled or not.
Andrew RayskiyAnd it also gets enabled in the IWA context?
Indeed, that's exactly what happens in `content/browser/renderer_host/navigation_request.cc` -- see the `MutableRuntimeFeatureStateContext()`
- Andrew Rayskiy
- Chris Thompson
Chris Thompson (Gerrit)
Chris Thompson voted and added 2 comments![]( "Open in Gerrit")
Votes added by Chris Thompson
| Code-Review | +1 |
2 comments
Answering the LNA question in-line. I also did a cursory overall review and this seems reasonable (but I did not do a deep code review).
if (AreDirectSocketsAllowedByEmbedder(rfh)) {I think this one should be looked at. Adding chthomp to the review to look at the implications for PNA.
ChrApps have historically been exempt from any PNA restrictions -- see the now removed `ShouldAllowPrivateNetworkAccessUnconditionally()` call; there's no behavior change here modulo the permissions-policy check which has no effect for ChrApps whatsoever.
I'm comfortable with not worrying about LNA for Chrome Apps. The remaining Chrome Apps are limited and deprecated.
Related details
- Andrew Rayskiy
- Camille Lamy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Camille Lamy (Gerrit)
Camille Lamy voted and added 1 comment![]( "Open in Gerrit")
Votes added by Camille Lamy
| Code-Review | +1 |
1 comment
Thanks Chris! Lgtm
Related details
- Andrew Rayskiy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy voted and added 3 comments![]( "Open in Gerrit")
Votes added by Andrew Rayskiy
| Commit-Queue | +2 |
3 comments
Thanks! Adding Chris for a question about the change to direct sockets availability affecting PNA.
Chris, for the context, the IWA team is splitting the way access to Direct Sockets is granted to contexts. Previously, Chrome Apps would force enable the IsolatedContext state. Now, they instead just force enable the access to Direct Sockets only. As part of this change, there is a modification in this CL that seems to bypass the PNA permission request when the Direct Sockets have been enabled for Chrome Apps (see comment in content/browser/direct_sockets/direct_sockets_service_impl.cc). Could you have a look and validate whether it makes sense to do this change? Thanks!
Beyond the PNA question, I also have a more general question about the access to Direct Sockets from worker contexts. My read of the CL is that the embedder never gives access to ServiceWorkers. Is that correct? What happens to SharedWorkers and DirectWorkers? And worklet contexts?
Attaching my response to the PNA topic here (from another thread):
Andrew RayskiyChrApps have historically been exempt from any PNA restrictions -- see the now removed `ShouldAllowPrivateNetworkAccessUnconditionally()` call; there's no behavior change here modulo the permissions-policy check which has no effect for ChrApps whatsoever.
Direct Sockets are exposed in Windows and Dedicated, Shared, and Service Workers (see the [.idl](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/direct_sockets/tcp_socket.idl?q=-f:%5Eout%2F%20-f:%5Egen%2F%20-f:%5Esrc%2F%20case:auto%20tcp_socket.idl)), which effectively excludes worklets. However, Shared and Service Workers actually illustrate what happens when a project stalls; we never gained enough momentum to launch the API there due to unresolved permission policy issues. While we had some initial ideas (due to the baseline permissions policy being clearly defined), they never moved forward, leaving us with an unresolved gap :) With that in mind, there's nothing wrong with not moving forward with embedder access in those contexts (as stuff doesn't work there anyway)
Acknowledged
bool AreDirectSocketsAllowedByEmbedder(RenderFrameHost* rfh) {nit: For readability, I wonder if it would be possible to have a DirectSocketsPolicy::DirectSocketsAllowedByEmbedderForOrigin static method somewhere that does this and can be used here and in NavigationRequest. This is a nit, so feel free to ignore it.
Imo it won't change much -- the only difference would be in skipping the delegate presence check.
} else if (!AreDirectSocketsAllowedByEmbedder(render_frame_host)) {nit: maybe add a comment clarifying what is happening as the else (!) condition is not immediately obvious.
Will do in a follow-up!
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Remove embedder-defined Isolated Context
This CL replaces embedder-defined Isolated Context with on-demand
enablement logic tailored specifically for Direct Sockets. As such,
there are now two independent areas where Direct Sockets are exposed:
* IsolatedContext (<=> Isolated Web Apps): API access is guarded by
direct-sockets, direct-sockets-private and direct-sockets-multicast
permissions policies; this is the core context for the API.
* Embedder-specific contexts (mostly Chrome Apps + selected extensions):
permissions policies are ignored here, so the system relies on
ChrApp/extension manifest permissions (the "sockets" field) instead.
Multicast is not accessible there (as there's no demand for it).
See crrev.com/c/7639669 for the feature flag propagation flow.
- M chrome/browser/chrome_content_browser_client.cc
- M chrome/browser/chrome_content_browser_client.h
- M chrome/browser/direct_sockets/chrome_direct_sockets_delegate.cc
- M chrome/browser/direct_sockets/chrome_direct_sockets_delegate.h
- M chrome/browser/direct_sockets/direct_sockets_apitest.cc
- M content/browser/direct_sockets/direct_sockets_service_impl.cc
- M content/browser/renderer_host/navigation_request.cc
- M content/public/browser/content_browser_client.cc
- M content/public/browser/content_browser_client.h
- M content/public/browser/direct_sockets_delegate.h
- M content/public/browser/isolated_context_util.cc
- M third_party/blink/renderer/modules/direct_sockets/socket.cc
- M third_party/blink/renderer/modules/direct_sockets/udp_socket.cc
Code-Review: +1 by Camille Lamy, +1 by Vlad Krot, +1 by Chris Thompson
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |