-
-
Notifications
You must be signed in to change notification settings - Fork 31.7k
inspector: initial support for Network.loadNetworkResource #58077
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
base: main
Are you sure you want to change the base?
Conversation
Review requested:
|
86ba453
to
435c33f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #58077 +/- ##
==========================================
- Coverage 90.19% 90.18% -0.01%
==========================================
Files 635 639 +4
Lines 187344 187614 +270
Branches 36793 36813 +20
==========================================
+ Hits 168974 169208 +234
- Misses 11137 11139 +2
- Partials 7233 7267 +34
🚀 New features to boost your workflow:
|
src/inspector/io_agent.cc
Outdated
namespace node { | ||
namespace inspector { | ||
namespace protocol { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit... since we're finally up on c++20, we can start using the more condensed namespace node::inspector::protocol {
syntax where appropriate.
src/inspector/network_agent.cc
Outdated
}) | ||
)"; | ||
|
||
auto [r, response, err] = spawnFetchProcess(code, env_, in_url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a fan of spawning a Node.js process for each Network.loadNetworkResource
request from devtools frontend. A devtool frontend may call this method multiple times simultaneously if there are several source maps and spawning a Node.js process for each of them seems superfluous and resource draining.
As an alternative, we could expose this as a hook to JS inspector API so that it will be properly covered by environment variables like HTTP_PROXY
and the permission model. Additionally, this allows the JS API hook to filter which domain the process is allowed to send request to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we hook into the JavaScript side, wouldn’t it end up running on the same event loop as the main script?
If the code breaks before the fetch process for loadNetworkResource completes, it could cause a problem where the resource can’t be fully retrieved.
That said, as you pointed out, spawning a new process every time would consume a lot of resources, which is also problematic.
So I’m considering other approaches that don’t interfere with the event loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe spawning a worker thread would be a good enough compromise for the moment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like a good.
I had excluded worker_thread from consideration because it also gets targeted by the debugger,
but I’ll add a flag to prevent it from being targeted by the debugger.
src/inspector/network_agent.cc
Outdated
std::string command = | ||
env->exec_path() + " --eval \"" + code.data() + "\" -- " + url.data(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This command
is not used.
d5c6690
to
5e20033
Compare
5e20033
to
1f0c547
Compare
@legendecas cc: @joyeecheung |
Fixes: #57873
Adds initial support for Network.loadNetworkResource in the inspector.
https://chromedevtools.github.io/devtools-protocol/tot/Network/#method-loadNetworkResource
how to load network resource
Resource fetching is done via a spawned child process to keep the inspector's event loop clean and isolated.The JavaScript code for the fetch process is currently hardcoded, which may not be ideal, but I couldn't come up with a better approach at this point.
When --experimental-inspector-network-resource is set, a dedicated worker for loadNetworkResource is launched.
This worker fetches the resource and returns the result to the frontend.
https://github.com/nodejs/node/pull/58077/files#diff-5d3d58cbcfda008f49966a11d73863e67b78a237aa386f1fbdc5c9124136c1eb
check the behavior in chromium
To verify this change in Chromium, modify the section in the link as shown below and launch Chromium with the built DevTools frontend.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/front_end/core/sdk/Target.ts;l=87-88
# open Chromium with built devtools-frontend Chromium --custom-devtools-frontend=(file path)
other infomation
If this change is considered acceptable, I will send a follow-up change request to expand the Node.js DevTools target with IO support.
If it is necessary to wait until Network.loadNetworkResource becomes stable in CRDP, please feel free to keep this PR pending.