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

Frequent webrender panics when shutting down #26876

Closed
jdm opened this issue Jun 11, 2020 · 3 comments
Closed

Frequent webrender panics when shutting down #26876

jdm opened this issue Jun 11, 2020 · 3 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented Jun 11, 2020

I often get backtraces that look like this:

called `Option::unwrap()` on a `None` value (thread WRSceneBuilder#0, at /Users/jdm/.cargo/git/checkouts/webrender-c3596abe1cf4f320/de39995/webrender/src/scene_builder_thread.rs:616)
   0: backtrace::backtrace::trace_unsynchronized
   1: <servo::backtrace::Print as core::fmt::Debug>::fmt
   2: core::fmt::write
   3: std::io::Write::write_fmt
   4: servo::backtrace::print
   5: servo::main::{{closure}}
   6: std::panicking::rust_panic_with_hook
   7: _rust_begin_unwind
   8: core::panicking::panic_fmt
             at /rustc/a74d1862d4d87a56244958416fd05976c58ca1a8/src/libcore/panicking.rs:89
   9: core::panicking::panic
             at /rustc/a74d1862d4d87a56244958416fd05976c58ca1a8/src/libcore/panicking.rs:52
  10: core::option::Option<T>::unwrap
  11: webrender::scene_builder_thread::SceneBuilderThread::process_transaction
  12: webrender::scene_builder_thread::SceneBuilderThread::run::{{closure}}
  13: core::iter::adapters::map_fold::{{closure}}
  14: core::iter::traits::iterator::Iterator::fold::ok::{{closure}}
  15: core::iter::traits::iterator::Iterator::try_fold
  16: core::iter::traits::iterator::Iterator::fold
  17: <core::iter::adapters::Map<I,F> as core::iter::traits::iterator::Iterator>::fold
  18: core::iter::traits::iterator::Iterator::for_each
  19: <alloc::vec::Vec<T> as alloc::vec::SpecExtend<T,I>>::spec_extend
  20: <alloc::vec::Vec<T> as alloc::vec::SpecExtend<T,I>>::from_iter
  21: <alloc::vec::Vec<T> as core::iter::traits::collect::FromIterator<T>>::from_iter
  22: core::iter::traits::iterator::Iterator::collect
  23: webrender::scene_builder_thread::SceneBuilderThread::run
  24: webrender::renderer::Renderer::new::{{closure}}
  25: std::sys_common::backtrace::__rust_begin_short_backtrace
  26: std::thread::Builder::spawn_unchecked::{{closure}}::{{closure}}
  27: <std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
  28: std::panicking::try::do_call
  29: ___rust_try
  30: std::panicking::try
  31: std::panic::catch_unwind
  32: std::thread::Builder::spawn_unchecked::{{closure}}
  33: core::ops::function::FnOnce::call_once{{vtable.shim}}
  34: std::sys::unix::thread::Thread::new::thread_start
  35: __pthread_start

I suspect this is because we end up sending webrender transactions to webrender after we've officially started shutting down, but the IPC router thread doesn't know about that so it continues to forward them.

@gterzian
Copy link
Member

@gterzian gterzian commented Jun 12, 2020

Do we call webrender_api::RenderApi.shut_down at all? I can't actually find it anywhere. Dropping the handle to the api sends a ApiMsg::ClearNamespace, but not a shutdown message, so maybe it should be done explicitly as part of shutdown.

@gterzian
Copy link
Member

@gterzian gterzian commented Jun 12, 2020

I assume the ApiMsg::ClearNamespace sent on drop of the API eventually triggers https://github.com/servo/webrender/blob/1175acad2d4f49fa712e105c84149ac7f394261d/webrender/src/scene_builder_thread.rs#L363

which would explain why any messages still received after would panic at line 613 as linked to above.

So I think we should indeed do a blocking shut_down(true) call somewhere as part of shutdown to prevent this.

bors-servo added a commit that referenced this issue Jun 15, 2020
Blockingly shut-down webrender api

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

FIX #26876

---
<!-- 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 added a commit that referenced this issue Jun 15, 2020
Blockingly shut-down webrender api

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

FIX #26876

---
<!-- 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 added a commit that referenced this issue Jun 15, 2020
Blockingly shut-down webrender api

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

FIX #26876

---
<!-- 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 added a commit that referenced this issue Jun 16, 2020
Blockingly shut-down webrender api

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

FIX #26876

---
<!-- 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 added a commit that referenced this issue Jun 16, 2020
Blockingly shut-down webrender api

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

FIX #26876

---
<!-- 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 added a commit that referenced this issue Jun 17, 2020
Blockingly shut-down webrender api

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

FIX #26876

---
<!-- 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 added a commit that referenced this issue Jun 17, 2020
Blockingly shut-down webrender api

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

FIX #26876

---
<!-- 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. -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants
You can’t perform that action at this time.