Skip to content
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

constellation: crash to a new “sad tab” error page #30290

Merged
merged 10 commits into from Sep 6, 2023

Conversation

delan
Copy link
Member

@delan delan commented Sep 4, 2023

Servo historically recovered from crashes by navigating forwards to an about:failure page, but we never got around to reimplementing about:failure in the fetch-based network stack, so it turned into an “Unexpected scheme” error.

That approach was less than ideal though, because navigating forwards trashes your forward history. Even if we were to navigate with replacement, the location bar would show “about:failure” instead of the original URL, and we would lose the original LoadData containing the request body and headers and so on. In both cases, it would be tricky to make the reload command work properly or add a reload button to the error page.

This patch replaces our use of about:failure with an error page that behaves similarly to a network error.

image

When the constellation detects a panic, we create a replacement pipeline as before, but we now reload the page in place with the same LoadData except with .crash set to the crash details. This gets plumbed through RequestBuilder and Request to the fetch code, which converts it to a NetworkError::Crash, which the HTML parser renders as a new kind of error page. The user can then view the reason and backtrace if any, and click a button to reload the page.

To test this manually, go to data:text/html,<script>document.write()</script> after applying the patch below:

diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs
index c3d83be696..bbaab2182d 100644
--- a/components/script/dom/document.rs
+++ b/components/script/dom/document.rs
@@ -5127,6 +5127,7 @@ impl DocumentMethods for Document {
 
     // https://html.spec.whatwg.org/multipage/#dom-document-write
     fn Write(&self, text: Vec<DOMString>) -> ErrorResult {
+        panic!();
         if !self.is_html_document() {
             // Step 1.
             return Err(Error::InvalidState);

  • There are tests for these changes OR
    • feedback on how we can test this would be appreciated :)
  • These changes do not require tests because ___

resources/crash.html Outdated Show resolved Hide resolved
@delan delan marked this pull request as ready for review September 5, 2023 10:11
@delan delan changed the title constellation: crash to a new “sad tab” page constellation: crash to a new “sad tab” error page Sep 5, 2023
@jdm
Copy link
Member

jdm commented Sep 5, 2023

For testing, it might be possible to have an iframe that calls window.trap() (

unsafe { ::std::intrinsics::breakpoint() }
), and then the outer window could check that there's a cross-origin frame?

Copy link
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this change! This looks fine to me though I have a couple nit-picky comments.

components/constellation/constellation.rs Outdated Show resolved Hide resolved
components/constellation/constellation.rs Outdated Show resolved Hide resolved
components/net_traits/request.rs Show resolved Hide resolved
@delan
Copy link
Member Author

delan commented Sep 6, 2023

For testing, it might be possible to have an iframe that calls window.trap() ([…]), and then the outer window could check that there's a cross-origin frame?

This is interesting! std::intrinsics::trap and std::intrinsics::abort just kill the whole process, unless we are in multiprocess mode, but we can add a window.panic() function gated behind dom.servo_helpers.enabled. I’m not sure if the crash error page will count as cross-origin though, looking into it.

@delan delan requested a review from mrobinson September 6, 2023 03:58
@delan
Copy link
Member Author

delan commented Sep 6, 2023

For testing, it might be possible to have an iframe that calls window.trap() ([…]), and then the outer window could check that there's a cross-origin frame?

This is interesting! std::intrinsics::trap and std::intrinsics::abort just kill the whole process, unless we are in multiprocess mode, but we can add a window.panic() function gated behind dom.servo_helpers.enabled. I’m not sure if the crash error page will count as cross-origin though, looking into it.

<!-- tests/wpt/mozilla/tests/crash-page.sub.html -->
<iframe style=border:solid
src="{{location[scheme]}}://{{hosts[alt][]}}:{{location[port]}}/_mozilla/crash-page-inner.html">
</iframe>

<!-- tests/wpt/mozilla/tests/crash-page-inner.html -->
<script>panic()</script>

When the iframe crashes, we close all of the pipelines under the top-level browsing context and navigate that, so the page containing the iframe is gone too.

Copy link
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! It sounds like testing this is tricky for now. Just a couple tiny comments:

components/net_traits/lib.rs Outdated Show resolved Hide resolved
components/net_traits/request.rs Outdated Show resolved Hide resolved
components/net_traits/request.rs Outdated Show resolved Hide resolved
components/script_traits/lib.rs Outdated Show resolved Hide resolved
resources/crash.html Show resolved Hide resolved
@delan delan enabled auto-merge September 6, 2023 09:23
@delan delan added this pull request to the merge queue Sep 6, 2023
Merged via the queue into servo:master with commit c3c6c95 Sep 6, 2023
10 checks passed
@delan delan deleted the sad-tab branch September 6, 2023 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

about:failure doesn't exist any more
4 participants