[IWA] Dynamic feature flags for Isolated Web Apps [chromium/src : main]
Simon Hangl (Gerrit)
Simon Hangl added 4 comments![]( "Open in Gerrit")
// Maximum size of the manifest file. 1MB.is that speced somewhere?
manifest_loader_ = network::SimpleURLLoader::Create(will you also clean this up once the loading is complete?
return;if we end up here and the user data at the bottom of this function was set before, we'd still stick with the old data, wouldn't we?
// Yes, this parses untrusted data in the browser process. But it's Rust JSONis this something we can make sure to be true even when the parser code evolves?
Related details
- Zgroza (Luke) Klimek
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. |
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek added 4 comments![]( "Open in Gerrit")
// Maximum size of the manifest file. 1MB.is that speced somewhere?
Not really, though there's already a place where this is the expectation: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/apps/app_service/app_install/web_app_installer.cc;drc=e8b169d1e8ed51cc6e49a169f10c4876e5a9e30f;l=37
Where do You think a nice place for specing this could be? Explainer of the IWAs themselves?
manifest_loader_ = network::SimpleURLLoader::Create(will you also clean this up once the loading is complete?
Once the navigation is resumed this object is destroyed along with its members. So this should be automatic.
return;if we end up here and the user data at the bottom of this function was set before, we'd still stick with the old data, wouldn't we?
I think this is impossible. The only place that can set this data for a navigation handle is here and handle is not reused for multiple navigations.
Not to mention that I have no idea what scenario could lead to this, considering that IWA without manifest shouldn't be installable. Idk, someone would have to quickly remove some of the internal Chromium files? I'm even wondering whether not to put a `CHECK` in here.
// Yes, this parses untrusted data in the browser process. But it's Rust JSONis this something we can make sure to be true even when the parser code evolves?
The assumption of this parser is that it's memory safe. I recall it being used in other places that have such constraints as well. I'll investigate whether there is any `CHECK` or something I could place here.
Related details
- Simon Hangl
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. |
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek added 1 comment![]( "Open in Gerrit")
// Maximum size of the manifest file. 1MB.Zgroza (Luke) Klimekis that speced somewhere?
Not really, though there's already a place where this is the expectation: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/apps/app_service/app_install/web_app_installer.cc;drc=e8b169d1e8ed51cc6e49a169f10c4876e5a9e30f;l=37
Where do You think a nice place for specing this could be? Explainer of the IWAs themselves?
Maybe just setting a boundary that no one will reasonably ever surpass like 10MB or something would be a good idea?
Matthew Riley (Gerrit)
Matthew Riley added 1 comment![]( "Open in Gerrit")
// Yes, this parses untrusted data in the browser process. But it's Rust JSONis this something we can make sure to be true even when the parser code evolves?
The assumption of this parser is that it's memory safe. I recall it being used in other places that have such constraints as well. I'll investigate whether there is any `CHECK` or something I could place here.
Hi, yes, the JSON decoder in Chrome is now memory-safety by spec.
Related details
- Simon Hangl
- Zgroza (Luke) Klimek
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. |
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek added 1 comment![]( "Open in Gerrit")
Zgroza (Luke) Klimekif we end up here and the user data at the bottom of this function was set before, we'd still stick with the old data, wouldn't we?
I think this is impossible. The only place that can set this data for a navigation handle is here and handle is not reused for multiple navigations.
Not to mention that I have no idea what scenario could lead to this, considering that IWA without manifest shouldn't be installable. Idk, someone would have to quickly remove some of the internal Chromium files? I'm even wondering whether not to put a `CHECK` in here.
This is not relevant anymore, as I switched to the caching approach.
Related details
- Simon Hangl
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. |
Simon Hangl (Gerrit)
Simon Hangl added 6 comments![]( "Open in Gerrit")
- Cache permissions policies somewhere with the preferred lifetime ofit would be useful to describe how the cache behaves. from a high-level, is it correct to assume that the cache (and hence the parsed permissions policies for a specific IWA) will remain constant until the session is restarted?
raw_ptr<IwaPermissionsPolicyCache> cache_ = nullptr;nit: can we make this const?
if (!is_isolated_web_app_navigation() || !cache_) {can the cache be null?
const url::Origin iwa_origin =we technically only need this inside the if, don't we?
class IwaPermissionsPolicyCache : public KeyedService,is the cache volatile or will it preserve state across browser restarts? one may argue that the lazy reviewer should check that in the code, but it also doesn't hurt to provide that key information in a comment above 😊.
Related details
- Simon Hangl
- Zgroza (Luke) Klimek
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. |
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek added 6 comments![]( "Open in Gerrit")
- Cache permissions policies somewhere with the preferred lifetime ofit would be useful to describe how the cache behaves. from a high-level, is it correct to assume that the cache (and hence the parsed permissions policies for a specific IWA) will remain constant until the session is restarted?
Or the IWA gets uninstalled/updated, yes. I added a comment on the class itself, do You think adding this to the commit message also would make sense?
nit: can we make this const?
Done
can the cache be null?
In fact it cannot, changed.
we technically only need this inside the if, don't we?
Yes, sorry, this is still a bit messy.
is the cache volatile or will it preserve state across browser restarts? one may argue that the lazy reviewer should check that in the code, but it also doesn't hurt to provide that key information in a comment above 😊.
Yes, I'll add a comment, sorry.
TL;DR it's volatile as the manifest itself already stores it in a persistent way.
Related details
- Simon Hangl
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")
if (status.has_value()) {-1 for this. Use the existing `OnWebAppManifestUpdated()` similar to install/uninstall routines
DependsOn(IwaPermissionsPolicyCacheFactory::GetInstance());It's the reverse dependency, no?
!!iwa_policy) {nit: if (auto*) already checks everything
Related details
- Simon Hangl
- Zgroza (Luke) Klimek
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. |
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek added 6 comments![]( "Open in Gerrit")
Zgroza (Luke) Klimekwill you also clean this up once the loading is complete?
Once the navigation is resumed this object is destroyed along with its members. So this should be automatic.
Done
// Yes, this parses untrusted data in the browser process. But it's Rust JSONis this something we can make sure to be true even when the parser code evolves?
Matthew RileyThe assumption of this parser is that it's memory safe. I recall it being used in other places that have such constraints as well. I'll investigate whether there is any `CHECK` or something I could place here.
Hi, yes, the JSON decoder in Chrome is now memory-safety by spec.
Done
-1 for this. Use the existing `OnWebAppManifestUpdated()` similar to install/uninstall routines
Huh, idk why I did it like this. Refactored a bit.
DependsOn(IwaPermissionsPolicyCacheFactory::GetInstance());It's the reverse dependency, no?
Yes. Idk how this even worked, really. Now it should be alright.
nit: if (auto*) already checks everything
- Andrew Rayskiy
- Matthew Riley
- Simon Hangl
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. |
Robbie McElrath (Gerrit)
Robbie McElrath added 3 comments![]( "Open in Gerrit")
GetIwaPermissionPolicy(BrowserContext* browser_context,Does this need to be different than the previous function? If so, can you explain why in the description? When would callers use one vs the other?
Also, can you make the names have a similar naming convention?
isolated_app_policy, permissions_policy_header_,Does this mean that if you create a bundle with different Permissions-Policy headers for different pages, that we'll cache the resolved policy for the first page that gets loaded and not respect the potentially different header for other pages?
static network::ParsedPermissionsPolicy ParseIWAPermissionsPolicy(Can you explain here why IWAs have different parsing behavior?
Related details
- Andrew Rayskiy
- Simon Hangl
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. |