Searching with duckduckgo.com/html freezes Servo #26807
Comments
|
Using the https://duckduckgo.com/ URL still appears to work. It's just the https://duckduckgo.com/html version that has the issue. |
atouchet
changed the title
|
I tried to follow the steps and can reproduce it with a panic. |
|
Because I saw readablestream in the backtrace, @gterzian you might be interested in this? |
CYBAI
added
A-content/script
|
Interesting! I think I figured out how to fix the panic but with that fix, we will get a 400 page instead of a ddg search result page. |
|
While checking out to the commit before landing |
|
I suspect something about the request body that ddg receives from servo is broken, given those pieces of data. |
|
This can fix the panic and yes, looks like the request body is incorrect! diff --git a/components/script/dom/readablestream.rs b/components/script/dom/readablestream.rs
index 1c6ac242e9..90e6c72da7 100644
--- a/components/script/dom/readablestream.rs
+++ b/components/script/dom/readablestream.rs
@@ -6,7 +6,7 @@ use crate::dom::bindings::conversions::{ConversionBehavior, ConversionResult};
use crate::dom::bindings::error::Error;
use crate::dom::bindings::reflector::{reflect_dom_object, DomObject, Reflector};
use crate::dom::bindings::root::DomRoot;
-use crate::dom::bindings::settings_stack::AutoEntryScript;
+use crate::dom::bindings::settings_stack::{AutoEntryScript, AutoIncumbentScript};
use crate::dom::bindings::utils::get_dictionary_property;
use crate::dom::globalscope::GlobalScope;
use crate::dom::promise::Promise;
@@ -118,6 +118,7 @@ impl ReadableStream {
source: ExternalUnderlyingSource,
) -> DomRoot<ReadableStream> {
let _ar = enter_realm(global);
+ let _ais = AutoIncumbentScript::new(global);
let cx = global.get_cx();
let source = Rc::new(ExternalUnderlyingSourceController::new(source));btw, @jdm @gterzian while trying to spot the bug, I suspect this should be servo/components/script/dom/promise.rs Lines 245 to 247 in 2f6efcb |
|
Nice catch. I'm not entirely sure about whether In any case it sounds like a good idea to add the call that you added in your fix. With regards to the request body, how does it look now? From the backtrace it looks like the body originates from The various places where would check would be: servo/components/script/body.rs Line 429 in 2f6efcb servo/components/script/body.rs Line 166 in 2f6efcb servo/components/net/http_loader.rs Line 452 in 2f6efcb just seeing we're not actually adding a content-type there, whereas in FF I can see the Post has a Maybe try to add this to: servo/components/script/body.rs Line 435 in 2f6efcb |
|
oh, interesting! I didn't notice we miss the content-type here (I did see it's logged from the request though 🤔) |
|
Also another thing: should we not enter into a realm or a script before calling |
|
Ah ok, so But since the I think it's worth a try to add a servo/components/script/body.rs Line 435 in 2f6efcb |
I found we've handled the content-type in servo/components/script/dom/htmlformelement.rs Lines 772 to 776 in 2f6efcb
I'm not very sure but if I understand correctly about |
|
Ok so I'd say your fix at #26807 (comment) then the question is still what is wrong with the request body when it is sent... |
one thing I also noticed is, if I set a breakpoint at this line 👇 servo/components/script/body.rs Line 166 in 2f6efcb then I will endlessly get into it and can inspect the same Does that mean we get into an infinite loop and result in a 400 request 🤔? |
|
Yes, please add |
|
ooooooooooooh!!!!! gooooood catch!!! let me try it 🤣🤣🤣 |
|
Interesting! With sending the I suspect the IPC router in |
|
ok, I just tried a workaround and it fixed the issue! In this IPC router, we will just send a servo/components/net/http_loader.rs Lines 461 to 469 in 0645d8c I say workaround because I wonder doing the checking in above IPC router is more straightforward? but, it seems we don't hold a RequestBody there so we might can only handle it in the following IPC router? diff --git a/components/script/body.rs b/components/script/body.rs
index 8017519cdd..4ac1b780bd 100644
--- a/components/script/body.rs
+++ b/components/script/body.rs
@@ -165,6 +165,7 @@ impl TransmitBodyConnectHandler {
// In case of the data being in-memory, send everything in one chunk, by-passing SpiderMonkey.
if let Some(bytes) = self.in_memory.clone() {
let _ = bytes_sender.send(bytes);
+ let _ = control_sender.send(BodyChunkRequest::Done);
return;
}
@@ -346,7 +347,11 @@ impl ExtractedBody {
BodyChunkRequest::Extract(receiver) => {
body_handler.re_extract(receiver);
},
- BodyChunkRequest::Chunk => body_handler.transmit_body_chunk(),
+ BodyChunkRequest::Chunk => {
+ if body_handler.bytes_sender.is_some() {
+ body_handler.transmit_body_chunk();
+ }
+ },
// Note: this is actually sent from this process
// by the TransmitBodyPromiseHandler when reading stops.
BodyChunkRequest::Done => body_handler.stop_reading(), |
|
Maybe let me send a PR and discuss there! |
|
Also, it might be worth adding a new test for this 🤔? |
|
Ok good one. I think the way to do it so that it matches the flow in the case where we go through the native promise handler, is to add a Then at the top of So "normally", when we use the native promise handler, then when we send the "last" chunk, the other route in So I think we should replicate this with a flag in the Yes I think you can add a test in the mozilla folder, I think you can get the failure by doing a POST using a Blob too(might be easier to write than a form). |
|
I see! Thanks for the clear explanation!
Now I can understand why it's better to be handled in
Thanks! Will try to do that! |
As discussed in servo#26807 (comment), we'd like to add a new flag, `in_memory_done`, to `TransmitBodyConnectHandler` so that we can correctly finish and drop the sender correctly. When we send the bytes, we will mark the body as done and we can recognize it's already done in next tick so that we can send a Done request to finish the sender.
Fix infinite stream and its missing incumbent script environment when newing a new stream --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #26807 - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___
As discussed in servo#26807 (comment), we'd like to add a new flag, `in_memory_done`, to `TransmitBodyConnectHandler` so that we can correctly finish and drop the sender correctly. When we send the bytes, we will mark the body as done and we can recognize it's already done in next tick so that we can send a Done request to finish the sender.
As discussed in servo#26807 (comment), we'd like to add a new flag, `in_memory_done`, to `TransmitBodyConnectHandler` so that we can correctly finish and drop the sender correctly. When we send the bytes, we will mark the body as done and we can recognize it's already done in next tick so that we can send a Done request to finish the sender. Also, when there comes a redirect request, it will go to `re-extract` route, we can set the `done` flag to `false` which means we won't stop the IPC routers yet. Then, if the re-extract sent its bytes, we will be marked as done again so that we can finish with stopping the IPC routes when Chunk request comes.
Fix infinite stream and its missing incumbent script environment when newing a new stream --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #26807 - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___
As discussed in servo#26807 (comment), we'd like to add a new flag, `in_memory_done`, to `TransmitBodyConnectHandler` so that we can correctly finish and drop the sender correctly. When we send the bytes, we will mark the body as done and we can recognize it's already done in next tick so that we can send a Done request to finish the sender. Also, when there comes a redirect request, it will go to `re-extract` route, we can set the `done` flag to `false` which means we won't stop the IPC routers yet. Then, if the re-extract sent its bytes, we will be marked as done again so that we can finish with stopping the IPC routes when Chunk request comes.
As discussed in servo#26807 (comment), we'd like to add a new flag, `in_memory_done`, to `TransmitBodyConnectHandler` so that we can correctly finish and drop the sender correctly. When we send the bytes, we will mark the body as done and we can recognize it's already done in next tick so that we can send a Done request to finish the sender. Also, when there comes a redirect request, it will go to `re-extract` route, we can set the `done` flag to `false` which means we won't stop the IPC routers yet. Then, if the re-extract sent its bytes, we will be marked as done again so that we can finish with stopping the IPC routes when Chunk request comes.
As discussed in servo#26807 (comment), we'd like to add a new flag, `in_memory_done`, to `TransmitBodyConnectHandler` so that we can correctly finish and drop the sender correctly. When we send the bytes, we will mark the body as done and we can recognize it's already done in next tick so that we can send a Done request to finish the sender. Also, when there comes a redirect request, it will go to `re-extract` route, we can set the `done` flag to `false` which means we won't stop the IPC routers yet. Then, if the re-extract sent its bytes, we will be marked as done again so that we can finish with stopping the IPC routes when Chunk request comes.
As discussed in servo#26807 (comment), we'd like to add a new flag, `in_memory_done`, to `TransmitBodyConnectHandler` so that we can correctly finish and drop the sender correctly. When we send the bytes, we will mark the body as done and we can recognize it's already done in next tick so that we can send a Done request to finish the sender. Also, when there comes a redirect request, it will go to `re-extract` route, we can set the `done` flag to `false` which means we won't stop the IPC routers yet. Then, if the re-extract sent its bytes, we will be marked as done again so that we can finish with stopping the IPC routes when Chunk request comes.
Fix infinite stream and its missing incumbent script environment when newing a new stream --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #26807 - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___
|
Phew! Thanks @gterzian a lot! This is my first time to play with IPC things! and it makes me wonder if I can leverage the power of IPC to make module script work better and more readable 🤔 |
|
You're welcome, let me know what you find out! |
URL: https://duckduckgo.com/html
Steps to reproduce:
This issue first occurred in the 2020-06-04 nightly build. Tested in Windows 10.
Regression range: ea491b4...8536cee
The text was updated successfully, but these errors were encountered: