-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
[BUG] Request Interception Breaks Worker importScripts with Remote Resource #5952
Comments
Some more debugging info 🎉 If you circumvent Playwright's // ctx.route("**", async (route) => {
// await route.continue();
// });
let foundImportJS = false
const cdp = await ctx.newCDPSession(page);
cdp.on("Fetch.requestPaused", (evt) => {
if (evt.request.url === "http://localhost:4747/import-me.js") {
foundImportJS = true;
}
cdp.send("Fetch.continueRequest", { requestId: evt.requestId });
});
await cdp.send('Network.enable');
await Promise.all([
cdp.send('Network.setCacheDisabled', { cacheDisabled: true }),
cdp.send('Fetch.enable', {
handleAuthRequests: true,
patterns: [{urlPattern: '*'}],
}),
]); the tests run clean. |
More debugging 🐛 : To debug the first test case in the issue ( _onRequest(workerFrame: frames.Frame | undefined, requestWillBeSentEvent: Protocol.Network.requestWillBeSentPayload, requestPausedEvent: Protocol.Fetch.requestPausedPayload | null) {
console.log("_onRequest", requestWillBeSentEvent.request.url);
…
}
_onRequestWillBeSent(workerFrame: frames.Frame | undefined, event: Protocol.Network.requestWillBeSentPayload) {
console.log("_onRequestWillBeSent", event.request.url);
…
}
_onRequestPaused(workerFrame: frames.Frame | undefined, event: Protocol.Fetch.requestPausedPayload) {
console.log("_onRequestPaused", event.request.url);
…
} After running the test (which times out), I see the following logged:
Notably, there is no |
Modifying
to
allows the test to pass but then _onRequestWill be sent comes after
The code on master appears to deadlock: For this particular request ( |
@pavelfeldman I had a chance to setup my machine with a Chromium debug build this evening, and think I've found some useful additional bits. (Wow, does compilation of Chromium for the first time on a new machine take a good deal of time!) I'm operating under the assumption that Chrome SHOULD send us If you run the failing test case (#5953) with Playwright pointed at a local Chromium build with the following logging in diff --git a/third_party/blink/renderer/core/inspector/inspector_network_agent.cc b/third_party/blink/renderer/core/inspector/inspector_network_agent.cc
index 6370da0d58..8cc24776cc 100644
--- a/third_party/blink/renderer/core/inspector/inspector_network_agent.cc
+++ b/third_party/blink/renderer/core/inspector/inspector_network_agent.cc
@@ -36,6 +36,7 @@
#include "base/containers/span.h"
#include "base/macros.h"
#include "base/memory/scoped_refptr.h"
+#include "base/logging.h"
#include "build/build_config.h"
#include "net/base/ip_address.h"
#include "net/base/ip_endpoint.h"
@@ -1122,6 +1123,7 @@ void InspectorNetworkAgent::WillSendRequestInternal(
Maybe<String> maybe_frame_id;
if (!frame_id.IsEmpty())
maybe_frame_id = frame_id;
+ VLOG(1) << "WillSendRequestInternal:: URL:" << request_info->getUrl();
GetFrontend()->requestWillBeSent(
request_id, loader_id, documentURL, std::move(request_info),
base::TimeTicks::Now().since_origin().InSecondsF(), you will see that, in fact, the
However, if we experiment and remove the guard to the deliberate flush (and therefore immediately flush): diff --git a/third_party/blink/renderer/core/inspector/inspector_network_agent.cc b/third_party/blink/renderer/core/inspector/inspector_network_agent.cc
index 6370da0d58..651a6eec83 100644
--- a/third_party/blink/renderer/core/inspector/inspector_network_agent.cc
+++ b/third_party/blink/renderer/core/inspector/inspector_network_agent.cc
@@ -36,6 +36,7 @@
#include "base/containers/span.h"
#include "base/macros.h"
#include "base/memory/scoped_refptr.h"
+#include "base/logging.h"
#include "build/build_config.h"
#include "net/base/ip_address.h"
#include "net/base/ip_endpoint.h"
@@ -1122,14 +1123,15 @@ void InspectorNetworkAgent::WillSendRequestInternal(
Maybe<String> maybe_frame_id;
if (!frame_id.IsEmpty())
maybe_frame_id = frame_id;
+ VLOG(1) << "WillSendRequestInternal:: URL:" << request_info->getUrl();
GetFrontend()->requestWillBeSent(
request_id, loader_id, documentURL, std::move(request_info),
base::TimeTicks::Now().since_origin().InSecondsF(),
base::Time::Now().ToDoubleT(), std::move(initiator_object),
BuildObjectForResourceResponse(redirect_response), resource_type,
std::move(maybe_frame_id), request.HasUserGesture());
- if (is_handling_sync_xhr_)
- GetFrontend()->flush();
+ // if (is_handling_sync_xhr_)
+ GetFrontend()->flush();
if (pending_xhr_replay_data_) {
resources_data_->SetXHRReplayData(request_id, the test PASSES since we now see the the If you think I'm on the right track (and Chrome is misbehaving here), please let me know and I will contribute a test to Chromium and then work with you on figuring out the appropriate patch. (I'm assuming always flushing won't fly 😄 .) I've also noticed that Fetch and Network are separate domains. Do they queue independently and are therefore going to cause us to see unexpected orderings of combined events depending on when each of them flushes? (If they queue in the same queue, the order would be preserved regardless of when we eventually flush.) /cc @aslushnikov since it looks like he fixed a similar timing/flushing bug for sync xhrs. |
I opened a repro test case in Chromium: https://chromium-review.googlesource.com/c/chromium/src/+/2789674. This is operating on the assumption that the Also, I noticed for network activity from workers the |
Wow, great job! The order of the events is not that important as long as we get both requestPaused and requestWillBeSent. But it is essential that we receive requestWillBeSent and your flush pointer is a great discovery. Workers should flush messages upon exiting the task, which does not happen. should happen for workers too. @dgozman: could you take a look? |
https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/core/inspector/worker_inspector_controller.cc;l=175?q=worker_inspector_controller&ss=chromium - here is that code. You can probably check why it does not flush the event... |
@pavelfeldman Thanks for the pointers! (No pun intended 😄 ) I guess order doesn't matter, but it does matter that we receive both events independently and that they are not blocked by one another. (Currently, I suppose my upstream test should be modified to not assert an ordering, but to assert that we see both events for |
I updated the test and will try to upload upstream later. |
Phew 🌬️ I fixed my chromium setup and was able to upload a new patch here with the failing test case: https://chromium-review.googlesource.com/c/chromium/src/+/2793525 1 I haven't yet had a chance to stick debuggers in the worker_inspector_controller to get my bearing in the code base and try to see what's wrong. My (naïve) instinct is to try adding a flush in the In the next day or so, I'll stick some debug statements in the code to understand the flow of things (and what a "task" even means in this context). One thing that's caught my attention, is that if you replace 1 Ignore the other—now abandoned—change. I suppose I should have uploaded a new patch to the old review 🤦 . Learning! |
Some notes/questions from debugging this morning:
@pavelfeldman or @dgozman: Since it looks like we won't get the flush from Thanks for y'alls help (and patience)! I definitely have a LOT of learning/reading to do to understand even the basic architecture of the rendered. |
It looks like one option could be to update the signature of |
I went with a simpler approach than changing WillSendRequest. Upstream patch available for review. I'll carry on implementation detail discussions there now that we have a candidate. Thanks! |
Network.requestWillBeSent is not getting flushed until its corresponding Fetch.requestPaused is continued when using importScripts with a remote resource in a worker. Both events should be emitted and flushed without needing to continue the request for the imported script. NB: This change assumes there's not a more direct/appropriate time to invoke flush in WorkerInspectorController itself. See discussion here: microsoft/playwright#5952 (comment) Change-Id: I759d8c0e38d515bb07e8cebb73623b75baada5c8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2793525 Reviewed-by: Andrey Kosyakov <caseq@chromium.org> Reviewed-by: Dmitry Gozman <dgozman@chromium.org> Commit-Queue: Dmitry Gozman <dgozman@chromium.org> Cr-Commit-Position: refs/heads/master@{#868876} GitOrigin-RevId: 031a7948c827a0e50000e1c5da40a11489cf4124
This bug is not fixed, version: 1.30, I can't intercept the request which is loaded from a worker by importScript |
@Jabbar2010 Please file a new issue with a repro, filling in the "Bug Report" template. |
Context:
Code Snippet
/worker/worker-http-import.html
:/worker/worker-http-import.js
:/worker/import-me.js
:Describe the bug
Request interception causes
importScripts("./import-me.js")
to hang. If youdisable request interception (i.e. comment out the
context.route(…)
bits),the importScripts succeeds,
hello from import-me.js
is logged, and thewaitForSelector
succeeds and matches the
finished
message that's bubbled up.Additional Debugging Notes
DEBUG=pw:protocol
we see aFetch.requestPaused
forhttp://localhost:4747/import-me.js
,but no protocol messages about that domain after when we would eventually expect
a
requestWillBeSent
and a resume.http://localhost:4747/import-me.js
,requestWillBeSent
isundefined
here incrNetworkManager.js
:playwright/src/server/chromium/crNetworkManager.ts
Line 182 in 495085c
the below demo (
fetch-worker.js
) thefetch
request succeeds.importScripts
hang. For example, in the below demos (inline-import-worker.js
and
blob-import-worker.js
) a Data URL and Object URL are successfully imported.Here's a real world example that's broken:
If you comment out
ctx.route(…)
and re-run, it will succeed.And below are various demos/experiments I used to narrow down why a website wasn't
working. We started with a 3rd-party PDF reader (https://mozilla.github.io/pdf.js/) appearing
subtly broken and hanging only when connected with Playwright, and were not sure
where the problem was, but have hopefully started to narrow it down! Unfortunately, we encounter a handful of libraries that use Web Workers with remote importScripts that are broken during testing because of this bug.
If you run the above, you'll notice all entries in the table flip to
ran
except
http-import-worker
. If you comment out thectx.route(…)
bits, itsucceeds.
Thank you for all your work and let me know if you have some additional tips
for debugging or need more information! I would love help getting to the bottom
of this! (I will contribute an MR with the test case shortly.)
The text was updated successfully, but these errors were encountered: