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

Ensure clean shutdown of JS threads #27016

Merged
merged 1 commit into from Jun 30, 2020

Conversation

gterzian
Copy link
Member

@gterzian gterzian commented Jun 22, 2020

FIX #26685
FIX #26996
FIX #9672
FIX #27027


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #___ (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___

@highfive
Copy link

highfive commented Jun 22, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/constellation/constellation.rs
  • @cbrewster: components/constellation/constellation.rs

@gterzian
Copy link
Member Author

gterzian commented Jun 22, 2020

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

bors-servo commented Jun 22, 2020

Trying commit 91c6c42 with merge dee9465...

bors-servo added a commit that referenced this issue Jun 22, 2020
constellation: don't make shutdown depend on having exited all pipelines

<!-- Please describe your changes on the following line: -->

FIX #26685

FIX #26996

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@gterzian gterzian changed the title constellation: don't make shutdown depend on having exited all pipelines Constellation: don't make shutdown depend on having exited all pipelines Jun 22, 2020
@gterzian
Copy link
Member Author

gterzian commented Jun 22, 2020

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

bors-servo commented Jun 22, 2020

Trying commit 0d4c16c with merge 019ac56...

bors-servo added a commit that referenced this issue Jun 22, 2020
Constellation: don't make shutdown depend on having exited all pipelines

<!-- Please describe your changes on the following line: -->

FIX #26685
FIX #26996
FIX #9672

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
// Randomly close a pipeline if --random-pipeline-closure-probability is set
// This is for testing the hardening of the constellation.
self.maybe_close_random_pipeline();
self.handle_request();
}

// Try to cleanly exit all pipelines, until a timeout hits.
let pipeline_exit_timeout = after(Duration::from_millis(200));
Copy link
Contributor

@paulrouget paulrouget Jun 22, 2020

Choose a reason for hiding this comment

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

If possible, can you make that a pref?

loop {
if self.pipelines.is_empty() {
break;
}
Copy link
Contributor

@paulrouget paulrouget Jun 22, 2020

Choose a reason for hiding this comment

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

This loop + if could be turned into a while.

recv(pipeline_exit_timeout) -> _ => {
if !self.pipelines.is_empty() {
warn!("Failed to exit a number of pipelines {:?}", self.pipelines.len());
}
Copy link
Contributor

@paulrouget paulrouget Jun 22, 2020

Choose a reason for hiding this comment

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

Is there any way to get more information about why it's blocked?

Copy link
Contributor

@paulrouget paulrouget Jun 22, 2020

Choose a reason for hiding this comment

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

I'm afraid we will have that warning and never pay attention to it.

Maybe make it an error, or dump a stack, not sure…

Copy link
Member Author

@gterzian gterzian Jun 22, 2020

Choose a reason for hiding this comment

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

I tried sampling them via the BHM, and while it works it's a bit messy because the bhm messaging stuff is not really setup for that, I think it could potentially introduce a new hang since we don't match bhm senders to pipeline yet(so the constellation could end-up blocking on a receiver for an already exited pipeline if not careful), so perhaps best done as a follow-up, if at all.

Also someone could use the dedicated sampling API to do this manually in some way, the current changes are more aimed at ensuring the window actually closes when someone wants to close it.

Also this seems to mostly happen when JS is running and blocking the script-thread from handling the exit messages, so I think what we really need is a hook to kill the script-thread, see #27027

@bors-servo
Copy link
Contributor

bors-servo commented Jun 22, 2020

💔 Test failed - status-taskcluster

@gterzian
Copy link
Member Author

gterzian commented Jun 22, 2020

@paulrouget r?

@gterzian
Copy link
Member Author

gterzian commented Jun 23, 2020

Ok actually I will do this "properly" as per #27027, and remove the timeout since that doesn't actually solve the underlying problem...

@gterzian gterzian marked this pull request as draft Jun 23, 2020
@gterzian gterzian changed the title Constellation: don't make shutdown depend on having exited all pipelines Use BHM to terminate hanging JS threads when closing window Jun 24, 2020
@gterzian gterzian force-pushed the fix_closing_window branch 5 times, most recently from c8eaf18 to f083da4 Compare Jun 30, 2020
@gterzian
Copy link
Member Author

gterzian commented Jun 30, 2020

@bors-servo r=jdm,paulrouget

@bors-servo
Copy link
Contributor

bors-servo commented Jun 30, 2020

📌 Commit f083da4 has been approved by jdm,paulrouget

bors-servo added a commit that referenced this issue Jun 30, 2020
Ensure clean shutdown of JS threads

<!-- Please describe your changes on the following line: -->

FIX #26685
FIX #26996
FIX #9672
FIX #27027

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 30, 2020

Testing commit f083da4 with merge 9fb1a96...

@bors-servo
Copy link
Contributor

bors-servo commented Jun 30, 2020

💔 Test failed - status-taskcluster

@gterzian
Copy link
Member Author

gterzian commented Jun 30, 2020

  ▶ TIMEOUT [expected PASS] /_mozilla/mozilla/worklets/test_paint_worklet.html
  │ 
  │ error: XDG_RUNTIME_DIR not set in the environment.
  │ libEGL warning: No hardware driver found, falling back to software rendering
  │ internal error: entered unreachable code (thread <unnamed>, at components/script/dom/globalscope.rs:2733)
  │    0: servo::backtrace::print
  │    1: servo::main::{{closure}}
  │    2: std::panicking::rust_panic_with_hook
  │              at /rustc/a8cf3991177f30694200002cd9479ffbbe6d9a1a/src/libstd/panicking.rs:477
  │    3: std::panicking::begin_panic
  │    4: script::dom::globalscope::GlobalScope::can_continue_running
  │    5: script::dom::globalscope::GlobalScope::perform_a_microtask_checkpoint
  │    6: <script::dom::bindings::settings_stack::AutoEntryScript as core::ops::drop::Drop>::drop
  │    7: profile_traits::time::profile
  │    8: script::dom::globalscope::GlobalScope::evaluate_script_on_global_with_result
  │    9: script::dom::workletglobalscope::WorkletGlobalScope::evaluate_js
  │   10: script::dom::worklet::WorkletThread::process_control
  │   11: script::dom::worklet::WorkletThread::run
  │   12: std::sys_common::backtrace::__rust_begin_short_backtrace
  │   13: core::ops::function::FnOnce::call_once{{vtable.shim}}
  │   14: <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once
  │              at /rustc/a8cf3991177f30694200002cd9479ffbbe6d9a1a/src/liballoc/boxed.rs:1076
  │       <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once
  │              at /rustc/a8cf3991177f30694200002cd9479ffbbe6d9a1a/src/liballoc/boxed.rs:1076
  │       std::sys::unix::thread::Thread::new::thread_start
  │              at /rustc/a8cf3991177f30694200002cd9479ffbbe6d9a1a/src/libstd/sys/unix/thread.rs:87
  │   15: start_thread
  │   16: __clone
  └ [2020-06-30T05:11:54Z ERROR servo] internal error: entered unreachable code

Ok, didn't account for worklets...

@gterzian
Copy link
Member Author

gterzian commented Jun 30, 2020

@bors-servo r=jdm,paulrouget

@bors-servo
Copy link
Contributor

bors-servo commented Jun 30, 2020

📌 Commit 44ebca7 has been approved by jdm,paulrouget

@bors-servo
Copy link
Contributor

bors-servo commented Jun 30, 2020

Testing commit 44ebca7 with merge b9404fc...

@bors-servo
Copy link
Contributor

bors-servo commented Jun 30, 2020

☀️ Test successful - status-taskcluster
Approved by: jdm,paulrouget
Pushing b9404fc to master...

@bors-servo bors-servo merged commit b9404fc into servo:master Jun 30, 2020
2 checks passed
@gterzian gterzian deleted the fix_closing_window branch Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment