DevTools: use local root's devtools frame token for NavigationURLLoader. [chromium/src : main]
Danil Somsikov (Gerrit)
Danil Somsikov voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Alex Rudenko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Somsikov (Gerrit)
Danil Somsikov added 1 comment![]( "Open in Gerrit")
Related details
- Alex Rudenko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Charlie Reis (Gerrit)
Charlie Reis added 2 comments![]( "Open in Gerrit")
Thanks-- some questions below to see if we can make this more obviously a correct fix, with fewer maintenance risks going forward.
local_root_rfh->devtools_frame_token(),At first glance, this looks like a regression to me, but maybe that's from a lack of documentation of what this is for.
In general, it's better to use frame-specific info, since each frame under a local root may have a separate SiteInstance, origin, etc, even if they share the same SiteInstanceGroup and process. In this case, the dev_tools_frame_token is explicitly frame-specific in the name, making this line look wrong. That may not actually be the case for how it's used, but it's not clear.
I'm guessing that DevTools only creates throttling profiles for local roots, and that frames beneath them don't have a throttling profile? If that's the case, we should be more explicit in places like [here](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/navigation_request_info.h;drc=fc44a91d176d55c9c43a89bd7eefce89d96d0472;l=46) and [here](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/navigation_request_info.h;drc=fc44a91d176d55c9c43a89bd7eefce89d96d0472;l=120) that a local root's token is required.
I almost wonder if it would be better to use the actual frame's token and have an ability to look up the throttling profile via that frame's local root when the time comes? Or be more explicit here about passing in something about the throttling profile and not a "frame" token for a different frame, so that other code doesn't mistakenly use the token thinking it's for the current frame rather than the local frame root?
Related details
- Alex Rudenko
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. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
local_root_rfh->devtools_frame_token(),At first glance, this looks like a regression to me, but maybe that's from a lack of documentation of what this is for.
In general, it's better to use frame-specific info, since each frame under a local root may have a separate SiteInstance, origin, etc, even if they share the same SiteInstanceGroup and process. In this case, the dev_tools_frame_token is explicitly frame-specific in the name, making this line look wrong. That may not actually be the case for how it's used, but it's not clear.
I'm guessing that DevTools only creates throttling profiles for local roots, and that frames beneath them don't have a throttling profile? If that's the case, we should be more explicit in places like [here](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/navigation_request_info.h;drc=fc44a91d176d55c9c43a89bd7eefce89d96d0472;l=46) and [here](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/navigation_request_info.h;drc=fc44a91d176d55c9c43a89bd7eefce89d96d0472;l=120) that a local root's token is required.
I almost wonder if it would be better to use the actual frame's token and have an ability to look up the throttling profile via that frame's local root when the time comes? Or be more explicit here about passing in something about the throttling profile and not a "frame" token for a different frame, so that other code doesn't mistakenly use the token thinking it's for the current frame rather than the local frame root?
DevTools is currently fairly inconsistent in how it creates these tokens. For instance, for subresource requests it is using correctly the local root token [1][2] but for workers the wrong token is set similar to navigation requests for iframes fixed in this CL. The throttling configuration in Chrome DevTools Protocol applies per target which includes the main frame and local frames. Out-of-process frames have dedicated targets. That's why this change aims to figure out where the CDP target boundary is by using is_local_root to match the behavior for subresource requests in the renderer which is the expected behavior from the client perspective.
I almost wonder if it would be better to use the actual frame's token and have an ability to look up the throttling profile via that frame's local root when the time comes?
I think the limitation here is that the throttling profiles are used in the network service that does not have the information about the free tree making it impossible to determine if two frames belong to the same CDP target.
Or be more explicit here about passing in something about the throttling profile and not a "frame" token for a different frame, so that other code doesn't mistakenly use the token thinking it's for the current frame rather than the local frame root?
I think this is a good idea and I would up to renaming devtools_token to something explicit like throttling_profile_id as it seems to be used only for throttling. WDYT?
[1]: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/inspector/inspector_network_agent.cc;l=2875;drc=87f4290357c0c1cb422a10d31670acfb9dff1b72
[2]: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/inspector/inspector_network_agent.cc;l=1515;drc=87f4290357c0c1cb422a10d31670acfb9dff1b72
Related details
- Charlie Reis
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. |