This change is to make visibility uniformly crate wide across all of the
structs and their props within devtools.
Testing: Tested using `.\mach build -d` `.\mach fmt` `.\mach test-tidy`
all passed
Part of: #41893
---------
Signed-off-by: Seiran <bo646ru@gmail.com>
Replace `RefCell` with `AtomicRefCell` for structs implementing Actor,
making them `Sync`.
Consolidate `register` and `register_later` into a single function,
removing the need to wait for a loop before accessing newly created
actors.
Now `ActorRegsitry` has improved locking. Instead of locking the entire
struct, each member can be locked separately. Additionally, since `find`
now returns `Arc`, we can `find` and `register` multiple actors
depending on each other, since the lock is only needed for the operation
and we can keep the reference after that.
Depends on: #41741, #41744
Testing: Manual testing
---------
Signed-off-by: eri <eri@igalia.com>
Co-authored-by: Martin Robinson <mrobinson@igalia.com>
Remove `find_mut` from `ActorRegistry`. Force that all actors must be
immutable after being inserted in the registry, only allowing changes
through internal mutability.
First step in refactoring `ActorRegistry` to make it more reliable,
easier to use and less error prone.
Depends on #41741. `NetworkEventActor` was more complicated to refactor
and it needed special care so it is split into its own change.
Testing: This patch doesn't change behaviour.
Signed-off-by: eri <eri@igalia.com>
Replace the previously hardcoded "name prefix" strings for actors with
[`type_name`](https://doc.rust-lang.org/std/any/fn.type_name.html).
Note that we aren't using the prefix directly as an unique identifier,
and `type_name` wouldn't be suitable for that. The suffix is an
incremental counter shared across all actors, so that alone is enough to
differentiate them.
The purpose of the prefix is to visually inspect the logs and see what
actors are involved. With this change the prefix will be slightly
different: "InspectorActor11" vs "inspector11", but that shouldn't
affect behaviour.
The eventual goal is to remove `new_name` and force the use of
`register/register_later` to create an actor. This is however a bit more
complicated, see #41768.
While I'd love to add `base_name` directly to the `Actor` trait, that
would unfortunately mean that it wouldn't be [dyn
compatible](https://doc.rust-lang.org/reference/items/traits.html#dyn-compatibility).
There are workarounds for this like separating the trait in two and
implementing it automatically, but I feel that would be too convoluted.
Testing: Manual testing, this patch shouldn't change behaviour.
Part of: #41768
Signed-off-by: eri <eri@igalia.com>
The `ActorRegistry::shareable` field is a copy of the
`Arc<Mutex<ActorRegistry>>` in `DevtoolsInstance`. It seemed to only be
used for the timeline actor. That adds complexity to `ActorRegistry` for
only one actor (which we aren't even using at the moment, the entire
`timeline.rs` is marked as dead code), and it is possible to avoid it by
storing the `Arc<Mutex<..>>` copy directly on the timeline actor when it
is created.
The same goes for `start_stamp`, it is only used in the timeline and it
is ok to have it there instead. Since (if used) the timeline would have
to be created alongside the registry there wouldn't be a noticeable time
difference.
Rename the field in `DevtoolsInstance` from `actors` to `registry` for
consistency with (most) of the other code in the crate.
Testing: Manual testing, but this patch doesn't modify behaviour.
Signed-off-by: eri <eri@igalia.com>
Update the target spec version from 133 to 145. Solves a few issues with
incorrect or outdated messages, such as an error with `reflow`.
The only meaningful change I could find is that the performance actor is
now called "perf" instead of "performance". It is part of a set of
global actors that the root handles. I made it more explicit and moved
the global actor registration inside of the root actor, it makes more
sense to have it there.
Finally, I ordered the message handling alphabetically. The only changed
entry is `getRoot` because of the global actors.
Testing: I manually tested the functionalities and checked the logs.
Signed-off-by: eri <eri@igalia.com>
Unify various `encodable` and `encode` methods from actors in an
`ActorEncode` trait, making it more consistent.
Add an `ActorRegistry::encode` method that finds an actor by name and
then returns its serialized version.
Add an `ActorMsg` struct to avoid creating repeated structs for simple
serializations.
There are two exceptions: `EncodableConsoleMessage` and `NodeInfo`.
These also have an `encode` method, but they are not actors, so
including them in this trait doesn't make much sense. Besides, they have
special requirements so it is better to keep them separate.
Testing: Manual testing in `about:debugging`.
Signed-off-by: eri <eri@igalia.com>
Some cleanup to avoid having to specify an empty `handle_message` method
if it is not implemented or not necessary. Removed the TODOs from
Environment and Pause since they don't respond to any message.
Testing: This patch doesn't change behaviour.
Signed-off-by: eri <eri@igalia.com>
Small refactor to reduce verbosity when creating actors. Since they
always need to be boxed, do that in `register{_later}` instead. I also
added the `Send` trait to `Actor` directly since it is always needed.
Testing: This patch doesn't change behaviour.
Signed-off-by: eri <eri@igalia.com>
#37686 fixed an entire class of devtools protocol bugs that led to
protocol desyncs with warnings in the Servo log, but it also removed the
warnings, making it harder to spot where our devtools impl is
incomplete.
this patch reintroduces a warning whenever some Actor::handle_message()
returns Err(ActorError).
Testing: debug logging only, so not really worth testing
---------
Signed-off-by: Delan Azabani <dazabani@igalia.com>
Co-authored-by: atbrakhi <atbrakhi@igalia.com>
Co-authored-by: Martin Robinson <mrobinson@igalia.com>
test_sources_list() relied on <https://servo.org/js/load-table.js> to
test scripts loaded from multiple origins, but that file was deleted a
couple of weeks ago. this patch adds a second internal web server, then
replaces that load with scripts loaded from two distinct origins:
<http://127.0.0.1:10000> and <http://127.0.0.1:10001>.
fixing that test revealed an impl bug where inline module scripts
containing `import` statements may never get their source contents
populated. this is because the logic for populating source contents for
inline scripts only applied to source actors created *before* we
finished parsing the page. we assumed that inline scripts always block
the parser, but this is not the case. this patch ensures that inline
source contents can be populated for source actors created after parse.
Testing: adds a new test,
test_source_content_with_inline_module_import_external()
Fixes: part of #36325
Signed-off-by: Delan Azabani <dazabani@igalia.com>
Co-authored-by: atbrakhi <atbrakhi@igalia.com>
Client messages, which are always requests, are dispatched to Actor
instances one at a time via Actor::handle_message. Each request must be
paired with exactly one reply from the same actor the request was sent
to, where a reply is a message with no type (if a message from the
server has a type, it’s a notification, not a reply).
Failing to reply to a request will almost always permanently break that
actor, because either the client gets stuck waiting for a reply, or the
client receives the reply for a subsequent request as if it was the
reply for the current request. If an actor fails to reply to a request,
we want the dispatcher (ActorRegistry::handle_message) to send an error
of type `unrecognizedPacketType`, to keep the conversation for that
actor in sync. Since replies come in all shapes and sizes, we want to
allow Actor types to send replies without having to return them to the
dispatcher.
This patch adds a wrapper type around a client stream that guarantees
request/reply invariants. It allows the dispatcher to check if a valid
reply was sent, and guarantees that if the actor tries to send a reply,
it’s actually a valid reply (see ClientRequest::is_valid_reply). It does
not currently guarantee anything about messages sent via the TcpStream
released via ClientRequest::try_clone_stream or the return value of
ClientRequest::reply. We also send `unrecognizedPacketType`,
`missingParameter`, `badParameterType`, and `noSuchActor` messages per
the
[protocol](https://firefox-source-docs.mozilla.org/devtools/backend/protocol.html#error-packets)
[docs](https://firefox-source-docs.mozilla.org/devtools/backend/protocol.html#packets).
Testing: automated tests all pass, and manual testing looks ok
Fixes: #37683 and at least six bugs, plus one with a different root
cause, plus three with zero impact
---------
Signed-off-by: atbrakhi <atbrakhi@igalia.com>
Signed-off-by: Delan Azabani <dazabani@igalia.com>
Co-authored-by: delan azabani <dazabani@igalia.com>
Co-authored-by: Simon Wülker <simon.wuelker@arcor.de>
Co-authored-by: the6p4c <me@doggirl.gay>
To show the contents of inline scripts in the Sources panel, we need to
send the whole HTML file from script to devtools, not just the script
code. This is trickier than the external script case, but we can look to
[how Firefox does
it](https://servo.zulipchat.com/#narrow/channel/263398-general/topic/Getting.20the.20original.20page.20HTML.20from.20script/near/524392861)
for some inspiration. The process is as follows:
- when we execute a script
- notify devtools to create the source actor
- if it’s an external script, send the script code to the devtools
server
- if it’s an inline script, don’t send any source contents yet
- devtools stores the contents in the source actor
- while loading a new document
- buffer the markup, so we can send it to devtools
- when we finish loading a new document
- send the buffered markup to the devtools server
- devtools stores the contents in any source actors with no contents yet
- when a source actor gets a `source` request
- if we have the contents, send those contents to the client
- if we don’t have the contents (inline script that loaded while
devtools was closed)
- FUTURE: try to fetch the markup out of cache
- otherwise send `<!-- not available; please reload! -->`
Testing: Several tests added to test the changes, also updates an
existing test with correct assertion
Fixes: https://github.com/servo/servo/issues/36874
---------
Signed-off-by: atbrakhi <atbrakhi@igalia.com>
Signed-off-by: Delan Azabani <dazabani@igalia.com>
Co-authored-by: Delan Azabani <dazabani@igalia.com>
* Respond to the "connect" message
Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>
* Bump log levels in devtools implementation a bit
If everything is "debug" then nothing stands out.
Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>
---------
Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>
This removes the last few uses of `time@0.1` in Servo. There are still
dependencies from `style` and `webrender`, but they will be removed soon
as well. The uses of this version of `time` are replaced with
`std::time` types and `time@0.3` when negative `Duration` is necessary.
Signed-off-by: Martin Robinson <mrobinson@igalia.com>
