`process_response_eof` is the only method that needs cx at least right
now. This PR removes one temp_cx and introduces one, removing that one
will is hard (needs VirtualMethods and a lot of work)
Testing: Just refactor
Part of #40600
---------
Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
If the number of layout threads is set to 1, then there sometimes is no
style thread pool. Instead of panicking in this case, we should just run
the code in question on the main thread with no thread pool. This fixes
a crash when `-y 1` is passed to servoshell.
Testing: This fixes a crash, but we currently do not run any automated
tests
with a single layout thread.
Signed-off-by: Martin Robinson <mrobinson@igalia.com>
Co-authored-by: Oriol Brufau <obrufau@igalia.com>
Co-authored-by: Luke Warlow <lwarlow@igalia.com>
Most likely a regression introduced in #42208 where the pending loads
counter has been reset. For successful request this wasn't an issue, but
for failed requests it was. Therefore, guard against these generation
request problems by bailing out as soon as we post-process the request
(which we do for all failed requests).
Fixes#42477
Signed-off-by: Tim van der Lippe <tvanderlippe@gmail.com>
When a `<link>` element loads a stylesheet then it calls
`ElementStylesheetLoader::new`, followed immediately by
calling `load` on the new loader. This is the only place where these
functions are used, so we can coalesce them together and avoid an
unreachable assertion in the process.
This should have no impact on the end user, it's just a cleanup.
Testing: Regressions should be covered by existing tests
Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>
The `HTMLLinkElement::pending_loads` field should only be used to track
outstanding loads for the current generation; any loads from previous
generations can be ignored. This avoids confusion about when to dispatch
load/error events. See
https://github.com/servo/servo/issues/42187#issuecomment-3808888585 for
more details about how the timing of multiple loads for the same
`<link>` element resulted in intermittent timeouts in one WPT test.
Testing: Intermittent failure did not appear within 75 runs; previously
reproduced within 15.
Fixes: #42187
Signed-off-by: Josh Matthews <josh@joshmatthews.net>
While adding various spec comments to the parts of `#the-end` (which are
scattered throughout the script crate), I stumbled upon the render
blocking stylesheets implementation. There was a HTML PR to make clear
when it should run and the corresponding WPT tests have been fixed.
Fixes#22715
Signed-off-by: Tim van der Lippe <tvanderlippe@gmail.com>
Regardless of the `Ok` status, we should always create a
stylesheet. However, the determining of the load/error event
is based on whether it has parsed actual content or not.
It also realigns some of the spec text to now only do it for link
elements, since for styles we shouldn't be checking the result of
the content type.
Part of #22715
---------
Signed-off-by: Tim van der Lippe <tvanderlippe@gmail.com>
Signed-off-by: Tim van der Lippe <TimvdLippe@users.noreply.github.com>
Co-authored-by: Martin Robinson <mrobinson@igalia.com>
We only implemented the first part (parser_inserted), but
weren't checking any of the other cases.
Testing: WPT
Part of #22715
Signed-off-by: Tim van der Lippe <tvanderlippe@gmail.com>
Rather than having each callside specifying the relevant
information from the GlobalScope, do this via a trait instead.
This would have saved us quite a bit of test debugging
since we would often forget to set relevant information
from the global context for a request.
Now, in the future when we need additional information from
the globalscope for a request, we only need to update this
method to make that happen.
Previously it would also sometimes use `document`, but
calling the relevant information on either `document` or
`globalscope` doesn't matter, since the `globalscope`
defers to the value from the `document` anyways.
Signed-off-by: Tim van der Lippe <tvanderlippe@gmail.com>
In preparation for adding preload support for all requests, we need to
add relevant client information to all these entrypoints. Additionally,
for links we now also set the referrer correctly and initialize
documents. All of which are required to start loading preload
information when processing requests.
Part of #35035
Signed-off-by: Tim van der Lippe <tvanderlippe@gmail.com>
Co-authored-by: Josh Matthews <josh@joshmatthews.net>
After #40639, this variant holds a single value, and I don't think think
using named fields was providing much extra clarity.
Testing: not needed, no behavior change
Signed-off-by: Oriol Brufau <obrufau@igalia.com>
Rebase of #37021 with review comments applied. These changes bundle up
the required information from the relevant global when a stylesheet is
added so that any requests for web fonts match the specification.
Testing: Newly passing tests.
Fixes: #36590
---------
Signed-off-by: Uthman Yahaya Baba <uthmanyahayababa@gmail.com>
Signed-off-by: Josh Matthews <josh@joshmatthews.net>
Co-authored-by: Uthman Yahaya Baba <uthmanyahayababa@gmail.com>
There are multiple motivating factors for this change:
1. `tracing-rs` can and is commonly used for structured logging, to gain
understanding in what is happening in concurrent code. We would like to
attempt to make a distinction of the performance tracing related usage
and the logging related usage.
2. This reduces boilerplate code. We don't have to feature guard every
span anymore, since the macro will do it for us.
3. This makes it easier to add multiple options for the trace backend
(i.e. exchanging tracing-rs for hitrace on OpenHarmony or System Tracing
in Android), or something entirely custom.
4. This makes it easier to add further compile-time options to control
the amount of tracing. E.g. a future PR could add options to enable
tracing based on environment variables set at compile time. Tracing adds
runtime overhead, even if the runtime switch is disabled, so having more
fine-grained options to partially enabled tracing could be useful.
Testing: Tested manually by building with and without the `tracing`
feature. In CI we also build with the tracing feature in the HarmonyOS
build. We don't have any automated tests for the correctness / presence
of the traced entries.
---------
Signed-off-by: Jonathan Schwender <schwenderjonathan@gmail.com>
This was due to the fact that two PRs doing this landed around the same
time. This PR eliminates the duplicate check.
Testing: This should not change behavior so is covered by existing
tests.
Signed-off-by: Martin Robinson <mrobinson@igalia.com>
This change is a rework of #22478, originally authored by @vimpunk.
It adds parsing of CSS in parallel with the main script thread. The
big idea here is that when the transfer of stylesheet bytes is
finished, the actual parsing is pushed to a worker thread from the Stylo
thread pool. This also applies for subsequent loads triggered by
`@import` statements.
The design is quite similar to the previous PR with a few significant
changes:
- Error handling works properly. The `CSSErrorReporter` is a crossbeam
`Sender` and a `PipelineId` so it can be trivially cloned and sent to
the worker thread.
- Generation checking is done both before and after parsing, in order
to both remove the race condition and avoid extra work when the
generations do not match.
- The design is reworked a bit to avoid code duplication, dropping added
lines from 345 to 160.
- Now that `process_response_eof` gives up ownership to the
`FetchResponseListener`, this change avoids all extra copies.
Testing: This shouldn't change observable behavior, so is covered
by existing tests.
Fixes: #20721Closes: #22478
---------
Signed-off-by: Martin Robinson <mrobinson@igalia.com>
Co-authored-by: mandreyel <mandreyel@protonmail.com>
Tests seem to assume that resource timing is available during the "load"
event. Doing so causes some tests to pass and others to fail, which
appear to have been false passes.
This is a preparatory change for asynchronous stylesheet parsing, when
resource timing entires must be added before sending stylesheets to
another thread for parsing.
Testing: This change updates WPT test results.
Signed-off-by: Martin Robinson <mrobinson@igalia.com>
The goal of this change is to prevent having to copy so much data out of
listeners when a fetch completes, which will be particularly important
for off-the-main thread parsing of CSS (see #22478). This change has
pros and cons:
Pros:
- This makes the design of the `FetchResponseListener` a great deal
simpler.
They no longer individually store a dummy `ResourceFetchTiming` that is
only replaced right before `process_response_eof`.
- The creation of the `Arc<Mutex<FetchResponseListener>>` in the
`NetworkListener` is abstracted away from clients and now they just
pass the `FetchResponseListener` to the fetch methods in the global.
Cons:
- Now each `FetchResponseListener` must explicitly call `submit_timing`
instead of having the `NetworkListener` do it. This is arguably a bit
easier to follow in the code.
- Since the internal data of the `NetworkListener` is now an
`Arc<Mutex<Option<FetchResponseListener>>>`, when the fetching code
needs to share state with the `NetworkListener` it either needs to
share an `Option` or some sort of internal state. In one case I've
stored the `Option` and in another case, I've stored a new inner
shared value.
Testing: This should not change observable behavior and is thus covered
by existing tests.
Fixes: #22550
---------
Signed-off-by: Martin Robinson <mrobinson@igalia.com>
`FetchReponseListener` has traditionally lived in `net` even though it
is only used in `script` currently. Because of the two way dependency,
it has also use a lot of templating to implement something pretty basic
(call methods on a trait object).
This change moves the trait to `script` and removes several levels of
templating, making the code quite a bit shorter and easier to
understand.
This change is preparation for fixing #22550 and implementing
off-the-main-thread CSS parsing.
Testing: This should not change any behavior so is covered by existing
tests.
Signed-off-by: Martin Robinson <mrobinson@igalia.com>
Moves interfaces defined by the performance spec to the
`script/dom/performance/` module from `script/dom/`.
Testing: Just a refactor shouldn't need any testing
Fixes: Partially #38901
Signed-off-by: WaterWhisperer <waterwhisperer24@qq.com>
This aligns the request object more with the specification,
since the spec now has a `traversable_for_user_prompts` and
a separate field for the client. Before, they were present
in the same enum.
In doing so, new structs are added that are all required in
the new spec. With this we can add support for preloaded
resources in this client, which are only populated when
we have an applicable Global.
Since the spec moved things around a bit, it now has a
dedicated method to populate the client from the request.
Unfortunately none of the WPT preload tests pass, since
the requests are received out-of-order. The specification
requires us to wait for that to settle, but I haven't figured
out yet how to do that. Given that this PR is already quite
large, opted to do that in a follow-up.
Part of #35035
Signed-off-by: Tim van der Lippe <tvanderlippe@gmail.com>
For `@import` we already had a `Arc<Locked<MediaList>>`, so now we will
no longer pointlessly clone it into a `MediaList` in order to pass it to
`Stylesheet::from_bytes()`, just for it to be wrapped again into an
`Arc<Locked<MediaList>>`.
Testing: Unneeded, no change in behavior
Stylo PR: https://github.com/servo/stylo/pull/251
Signed-off-by: Oriol Brufau <obrufau@igalia.com>
We were previously using the unsound `StylesheetContents::from_data()`
to create a dummy stylesheet, which we were later replacing with the
actual imported stylesheet once it loaded.
This patch changes that to use `ImportSheet::Pending` instead. But then:
- We need to store the `MediaList` in `StylesheetContextSource`.
Previosuly we stored it in the dummy stylesheet.
- We also need to store an Arc pointer to the `ImportRule`, in order to
update its stylesheet to `ImportSheet::Sheet(stylesheet)` later on.
Testing: Unnecessary, there should be no behavior change
Fixes: #39710
Stylo PR: https://github.com/servo/stylo/pull/250
Signed-off-by: Oriol Brufau <obrufau@igalia.com>
Stylesheets loaded via the `<link>` element should block the rendering
of the page according to the HTML specification [1]. This change makes
it so that they do this and, in addition, we do not take reftest
screenshots until all no element is blocking the rendering.
This change does not add support for the `blocking` attribute of
`<link>`, but that can be added in a follow change. In addition to
fixing a few tests, this change likely makes other tests no longer
intermittent. We will need to watch CI runs after this lands in order to
verify that though.
Testing: This change fixes at least two WPT tests.
Fixes: #26424.
Signed-off-by: Martin Robinson <mrobinson@igalia.com>
`StylesheetLoader` implements the `StylesheetLoader` trait from Stylo.
This is pretty confusing as the names are the same. This change renames
the Servo version to `ElementStyleSheet` loader so that it's clearer
from reading the code what each of these things are.
Testing: This change just makes a few renames so shouldn't change test
results.
Signed-off-by: Martin Robinson <mrobinson@igalia.com>
For duplicate style sheets with identical content, `StylesheetContents`
can be reused to avoid redundant parsing of the inline style sheets.
Since duplicate stylesheets is a common case with web components, this
change will significantly improve performance. Additionally, the cache
hit rate of stylo's `CascadeDataCache` can now be significantly
improved.
When shared `StylesheetContents` is modified, copy-on-write will occur
to avoid affecting other sharers. And then updates the references to
`CssRule` or `PropertyDeclarationBlock` stored in the CSSOMs to ensure
that modifications are made only on the new copy.
Signed-off-by: sharpshooter_pt <ibluegalaxy_taoj@163.com>
The scripted_caller only has information if the context is coming
from a script. If an element fetch listener processes CSP
violations, then this information doesn't exist. Instead, we should
use the global URL and the line number. WPT tests don't appear
to expect a column number, as they are all zero. Not all elements
are updated, as I am not actually sure all of them need it.
The source position remains an Option, since there are also code
paths that don't correspond to element or script sources. Maybe
in the future we can always determine the source position, but
let's take small steps towards that.
Part of #4577
Signed-off-by: Tim van der Lippe <tvanderlippe@gmail.com>
Co-authored-by: Josh Matthews <josh@joshmatthews.net>
This refactoring moves various CSP-related methods away from GlobalScope
and Document into a dedicated entrypoint. It also reduces the amount of
imports of the CSP crate, so that types are consolidated into this one
entrypoint. That way, we control how CSP code interacts with the script
crate.
For reviewing purposes, I split up the refactoring into separate
distinct commits that all move 1 method(group) into the new file.
Testing: no change in behavior, only a build improvement + code cleanup
---------
Signed-off-by: Tim van der Lippe <tvanderlippe@gmail.com>
Signed-off-by: Tim van der Lippe <TimvdLippe@users.noreply.github.com>
This changes includes two semi-related things:
1. Fixes some specification compliance issues when parsing mime
types and charsets for `XMLHttpRequest`.
2. Implements a `<stylesheet>` parsing quirk involving mime types.
Testing: There are tests for these changes.
Signed-off-by: Martin Robinson <mrobinson@igalia.com>
Co-authored-by: Martin Robinson <mrobinson@igalia.com>
All logic is implemented in `report_csp_violations` to avoid
pulling in various element-logic into SecurityManager.
Update the `icon-blocked.sub.html` WPT test to ensure that
the document is the correct target (verified in Firefox and Chrome).
Fixes#36806
Signed-off-by: Tim van der Lippe <tvanderlippe@gmail.com>
It also updates the FetchResponseListener to process CSP violations to
ensure that iframe elements (amongst others) properly generate the CSP
events. These iframe elements are used in the Trusted Types tests
themselves and weren't propagating the violations before.
However, the tests themselves are still not passing since they also use
Websockets, which currently aren't using the fetch machinery itself.
That is fixed as part of [1].
[1]: https://github.com/servo/servo/issues/35028
---------
Signed-off-by: Tim van der Lippe <tvanderlippe@gmail.com>
Signed-off-by: Josh Matthews <josh@joshmatthews.net>
Co-authored-by: Josh Matthews <josh@joshmatthews.net>
Adds support for both the content and the IDL attribute.
Note this doesn't cover dynamic updates to `document.styleSheets` and
the owner node of the sheet.
Testing: Covered by WPT
Fixes: #26739
Signed-off-by: Oriol Brufau <obrufau@igalia.com>
This PR implements the HTML spec quirk for stylesheets:
https://html.spec.whatwg.org/multipage/#link-type-stylesheet
The implementation adds a check in `stylesheet_loader.rs` to handle this
quirk condition correctly, and adds a new WPT test to verify that
same-origin non-CSS MIME type resources are properly treated as CSS in
quirks mode.
Testing: Added a new WPT test (`quirk-origin-check-positive.html`) that
verifies the positive case for this quirk.
Fixes: https://github.com/servo/servo/issues/36324
---------
Signed-off-by: saku-1101 <sakupi1101@outlook.jp>
Propagate through documents a flag that represents if any of the
ancestor navigables has a potentially trustworthy origin.
The "potentially trustworthy origin" concept appears to have gotten
confused in a couple of places and we were instead testing if a URL had
"potentially trustworthy" properties.
The main test for the ancestor navigables is
[mixed-content/nested-iframes](https://github.com/web-platform-tests/wpt/blob/master/mixed-content/nested-iframes.window.js)
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by
`[X]` when the step is complete, and replace `___` with appropriate
data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix#36108
<!-- Either: -->
- [X] There are tests for these changes
---------
Signed-off-by: Sebastian C <sebsebmc@gmail.com>
* Migrate to 2024 edition
Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>
* Allow unsafe_op_in_unsafe_fn lint
This lint warns by default in the 2024
edition, but is *way* too noisy for servo.
We might enable it in the future, but not now.
Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>
* Compile using the 2024 edition
Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>
---------
Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>
* Update to rust 1.85
This is needed for cargo-deny
Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>
* Upgrade crown
Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>
* Clippy fixes
Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>
* Re-upgrade cargo-deny to 0.18
Keeping it locked to 0.18 just in case they
update their required rustc version again
Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>
---------
Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>
`EmbedderMsg` was previously paired with an implicit
`Option<WebViewId>`, even though almost all variants were either always
`Some` or always `None`, depending on whether there was a `WebView
involved.
This patch adds the `WebViewId` to as many `EmbedderMsg` variants as
possible, so we can call their associated `WebView` delegate methods
without needing to check and unwrap the `Option`. In many cases, this
required more changes to plumb through the `WebViewId`.
Notably, all `Request`s now explicitly need a `WebView` or not, in order
to ensure that it is passed when appropriate.
Signed-off-by: Delan Azabani <dazabani@igalia.com>
Co-authored-by: Martin Robinson <mrobinson@igalia.com>
This puts a few commonly used `Node` helpers into a trait (`NodeTraits`)
and gives them more descriptive names and documentation. The renames:
- `document_from_node` -> `NodeTraits::owner_document`
- `window_from_node` -> `NodeTraits::owner_window`
- `stylesheets_owner_from_node<T:` -> `NodeTraits::stylesheet_list_owner`
- `containing_shadow_root` -> `NodeTraits::containing_shadow_root`
Signed-off-by: Martin Robinson <mrobinson@igalia.com>
The new version of rust allows us to elide some lifetimes and clippy is
now complaining about this. This change elides them where possible and
removes the clippy exceptions.
Fixes#34804.
Signed-off-by: Martin Robinson <mrobinson@igalia.com>
* Remove the referrer policy from document and rely on its policy container
Signed-off-by: Shane Handley <shanehandley@fastmail.com>
* Make ReferrerPolicy non-optional, instead using a new enum value to represent the empty string case
Signed-off-by: Shane Handley <shanehandley@fastmail.com>
* Fix clippy issue
Signed-off-by: Shane Handley <shanehandley@fastmail.com>
* Fix usage of Option<ReferrerPolicy> in unit test
Signed-off-by: Shane Handley <shanehandley@fastmail.com>
---------
Signed-off-by: Shane Handley <shanehandley@fastmail.com>
Instead of creating a `ROUTER` for each fetch, create a fetch thread
which handles all incoming and outcoming fetch requests. Now messages
involving fetches carry a "request id" which indicates which fetch is
being addressed by the message. This greatly reduces the number of file
descriptors used by fetch.
In addition, the interface for kicking off fetches is simplified when
using the `Listener` with `Document`s and the `GlobalScope`.
This does not fix all leaked file descriptors / mach ports, but greatly
eliminates the number used. Now tests can be run without limiting
procesess on modern macOS systems.
Followup work:
1. There are more instances where fetch is done using the old method.
Some of these require more changes in order to be converted to the
`FetchThread` approach.
2. Eliminate usage of IPC channels when doing redirects.
3. Also eliminate the IPC channel used for cancel handling.
4. This change opens up the possiblity of controlling the priority of
fetch requests.
Fixes#29834.
Signed-off-by: Martin Robinson <mrobinson@igalia.com>
