devtools: Properly handle opening and closing client connections #42583
devtools: Properly handle opening and closing client connections #42583simonwuelker merged 2 commits intoservo:mainfrom
Conversation
|
@eerii do you mind reviewing this one? |
dfebb10 to
e8713f9
Compare
eerii
left a comment
There was a problem hiding this comment.
This is great, thank you :) Keeping track of state in two places is a recipe for disaster, changes like this really help.
| // us noticing. | ||
| let mut connections = Vec::<TcpStream>::new(); | ||
| for stream in self.connections.values() { | ||
| for stream in connections_map.values() { |
There was a problem hiding this comment.
Does this work? That way we shouldn't need the explicit drop.
| for stream in connections_map.values() { | |
| for stream in self.connections.lock().unwrap().values() { |
Alternatively we could use a scope surrounding the for loop.
There was a problem hiding this comment.
That does work! I was sure it wouldn't, since values_mut is referencing the short-lived mutex guard. Interesting.
e8713f9 to
04a7991
Compare
04a7991 to
9b20876
Compare
TimvdLippe
left a comment
There was a problem hiding this comment.
Approving on behalf of Eerrii
|
🔨 Triggering try run (#21988582683) for Linux (WPT) |
|
Test results for linux-wpt from try job (#21988582683): Flaky unexpected result (72)
Stable unexpected results that are known to be intermittent (24)
Stable unexpected results (5)
|
|
|
|
Hmm. Many seemingly intermittent crashes(timeouts?) that seem completely unrelated, that don't reproduce locally. |
|
Oh i bet its because I don't have #42562 on my branch yet. |
Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>
Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>
9b20876 to
1a5911f
Compare
Both the
DevtoolsInstanceand theBrowsingContextActormaintain a list of live connections. When a new global is created, itsBrowsingContextActorcopies the list of active connections from theDevtoolsInstance. When a client handler thread exits, theBrowsingContextActors remove the connection from their list of connections.This has two implications:
BrowsingContextActors don't know about connections that are created after they were constructed, causing them to possibly incorrectly tell script that it doesn't need to send devtools updates anymoreDevtoolsInstancenever notices when connections are closed and panics when writing to them.To fix this, I've removed the list of connections from the
BrowsingContextActorand adjusted the logic to notify script when updates aren't needed anymore accordingly.For some reason, our tests always open two connections and close the first one immediately, which is how I found this bug in the first place.
Part of #42454 - previously
servo/components/script/dom/globalscope.rs
Lines 240 to 242 in d5d400c