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

Prevent panic on layout send in finish-load #24818

Merged
merged 2 commits into from Nov 22, 2019

Conversation

@gterzian
Copy link
Member

gterzian commented Nov 21, 2019

Do not run the finish load steps if the pipeline has been discarded by the time the parser finishes.


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

highfive commented Nov 21, 2019

Heads up! This PR modifies the following files:

@highfive
Copy link

highfive commented Nov 21, 2019

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@gterzian gterzian force-pushed the gterzian:fix_panic_on_finish_load branch from 7295a41 to bc9c4d8 Nov 21, 2019
@gterzian
Copy link
Member Author

gterzian commented Nov 21, 2019

@bors-servo try=wpt

@jdm I think this one might actually fix #22955 (and hopefully not break other things)

@bors-servo
Copy link
Contributor

bors-servo commented Nov 21, 2019

Trying commit bc9c4d8 with merge 4902566...

bors-servo added a commit that referenced this pull request Nov 21, 2019
Prevent panic on layout send in finish-load

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

Do not run the finish load steps if the pipeline has been discarded by the time the parser finishes.

---
<!-- 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 #22955 (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. -->
@jdm
Copy link
Member

jdm commented Nov 21, 2019

I think a more robust fix would be calling ServoParser::abort on Document::current_parser (if it exists) when we know a pipeline is being closed.

@gterzian gterzian force-pushed the gterzian:fix_panic_on_finish_load branch from bc9c4d8 to 3ba3f37 Nov 21, 2019
@gterzian
Copy link
Member Author

gterzian commented Nov 21, 2019

@jdm Thanks, I have changed the fix accordingly.

@jdm
Copy link
Member

jdm commented Nov 21, 2019

@bors-servo try=wpt

@gterzian gterzian force-pushed the gterzian:fix_panic_on_finish_load branch from 3ba3f37 to 1fab241 Nov 21, 2019
@bors-servo
Copy link
Contributor

bors-servo commented Nov 21, 2019

Testing commit 1fab241 with merge d92d0c6...

bors-servo added a commit that referenced this pull request Nov 21, 2019
Prevent panic on layout send in finish-load

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

Do not run the finish load steps if the pipeline has been discarded by the time the parser finishes.

---
<!-- 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 #22955 (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 Nov 21, 2019

💔 Test failed - status-taskcluster

@jdm
Copy link
Member

jdm commented Nov 21, 2019

Sorry, the try build for #24821 stomped on the branch so all the test runners couldn't clone the appropriate git revision. I'm going to wait until the tests for #24821 start running to retry this.

@jdm
Copy link
Member

jdm commented Nov 21, 2019

@bors-servo retry

@jdm
Copy link
Member

jdm commented Nov 22, 2019

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

bors-servo commented Nov 22, 2019

Trying commit 1fab241 with merge 7afe06d...

bors-servo added a commit that referenced this pull request Nov 22, 2019
Prevent panic on layout send in finish-load

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

Do not run the finish load steps if the pipeline has been discarded by the time the parser finishes.

---
<!-- 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 #22955 (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 Nov 22, 2019

💔 Test failed - status-taskcluster

@gterzian
Copy link
Member Author

gterzian commented Nov 22, 2019

0 matches
{
    "status": "CRASH", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": null, 
    "test": "/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/url.window.html", 
    "line": 65380, 
    "action": "test_result", 
    "expected": "OK"
}
{
    "status": "CRASH", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": null, 
    "test": "/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/abort-refresh-immediate.window.html", 
    "line": 65933, 
    "action": "test_result", 
    "expected": "TIMEOUT"
}
{
    "status": "CRASH", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": null, 
    "test": "/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/tasks.window.html", 
    "line": 66212, 
    "action": "test_result", 
    "expected": "TIMEOUT"
}

  │    7: <html5ever::tree_builder::TreeBuilder<Handle,Sink> as html5ever::tokenizer::interface::TokenSink>::process_token
  │    8: _ZN9html5ever9tokenizer21Tokenizer$LT$Sink$GT$13process_token17hdf12db064f2abc2dE.llvm.14290903763764066736
  │    9: html5ever::tokenizer::Tokenizer<Sink>::end
  │   10: script::dom::servoparser::ServoParser::abort
  │   11: script::script_thread::ScriptThread::handle_exit_pipeline_msg
  │   12: script::script_thread::ScriptThread::handle_msg_from_constellation
  │   13: _ZN6script13script_thread12ScriptThread11handle_msgs17h702aaab883c81f98E.llvm.14290903763764066736
  │   14: script::script_thread::ScriptThread::start
  │   15: profile_traits::mem::ProfilerChan::run_with_memory_reporting
  │   16: std::sys_common::backtrace::__rust_begin_short_backtrace
  │   17: _ZN3std9panicking3try7do_call17h0956d3ff2ca15edbE.llvm.2944147132392252601
  │   18: __rust_maybe_catch_panic
  │              at src/libpanic_unwind/lib.rs:81
  │   19: core::ops::function::FnOnce::call_once{{vtable.shim}}
  │   20: <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once
  │              at /rustc/1bd30ce2aac40c7698aa4a1b9520aa649ff2d1c5/src/liballoc/boxed.rs:942
  │   21: <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once
  │              at /rustc/1bd30ce2aac40c7698aa4a1b9520aa649ff2d1c5/src/liballoc/boxed.rs:942
  │       std::sys_common::thread::start_thread
  │              at src/libstd/sys_common/thread.rs:13
  │       std::sys::unix::thread::Thread::new::thread_start
  │              at src/libstd/sys/unix/thread.rs:79
  │   22: start_thread
  │   23: __clone
  │ 
  │ [2019-11-22T05:35:06Z ERROR servo] called `Result::unwrap()` on an `Err` value: "SendError(..)"
  └ Pipeline failed in hard-fail mode.  Crashing!

Looks like aborting the parser when the pipeline exits results in other crashes.

@jdm
Copy link
Member

jdm commented Nov 22, 2019

The async parser doesn't get used unless dom.servoparser.async_html_tokenizer.enabled is true. I think we want to abort the parser before we start tearing down the window to avoid this.

@jdm
Copy link
Member

jdm commented Nov 22, 2019

In particular, because aborting the parser can cause additional parser activity, I think we need to do that before we shut down the layout thread.

@gterzian
Copy link
Member Author

gterzian commented Nov 22, 2019

@bors-servo try=wpt

Ok, trying before shutting down layout...

@bors-servo
Copy link
Contributor

bors-servo commented Nov 22, 2019

Trying commit f28ffac with merge 1b707ee...

bors-servo added a commit that referenced this pull request Nov 22, 2019
Prevent panic on layout send in finish-load

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

Do not run the finish load steps if the pipeline has been discarded by the time the parser finishes.

---
<!-- 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 #22955 (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 Nov 22, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
State: approved= try=True

@jdm
Copy link
Member

jdm commented Nov 22, 2019

@bors-servo
Copy link
Contributor

bors-servo commented Nov 22, 2019

📌 Commit f28ffac has been approved by jdm

@highfive highfive assigned jdm and unassigned nox Nov 22, 2019
@bors-servo
Copy link
Contributor

bors-servo commented Nov 22, 2019

Testing commit f28ffac with merge 1b524ea...

bors-servo added a commit that referenced this pull request Nov 22, 2019
Prevent panic on layout send in finish-load

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

Do not run the finish load steps if the pipeline has been discarded by the time the parser finishes.

---
<!-- 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 #22955 (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 Nov 22, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: jdm
Pushing 1b524ea to master...

@bors-servo bors-servo merged commit f28ffac into servo:master Nov 22, 2019
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
@gterzian gterzian deleted the gterzian:fix_panic_on_finish_load branch Nov 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.