Skip to content

/ servo

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

Jump to bottom

Uses of panic! in script_thread #11685

Closed
asajeffrey opened this issue Jun 9, 2016 · 3 comments
Closed

Uses of panic! in script_thread #11685

asajeffrey opened this issue Jun 9, 2016 · 3 comments
Assignees
@asajeffrey
Labels
A-content/script I-panic

Comments

@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Jun 9, 2016

There are uses of panic! in script_thread.rs which have race conditions:

        panic!("resize sent to nonexistent pipeline");
        panic!("Page rect message sent to nonexistent pipeline");
        panic!("Set scroll state message message sent to nonexistent pipeline: {:?}", id);
        panic!("freeze sent to nonexistent pipeline");
        panic!("thaw sent to nonexistent pipeline");

the pipeline might have been torn down before the message was sent. These should probably all be warn!

cc @Ms2ger
@asajeffrey asajeffrey self-assigned this Jun 9, 2016
This was referenced Jun 9, 2016
Closed
Merged
@cbrewster cbrewster mentioned this issue Jun 23, 2016
Closed
bors-servo added a commit that referenced this issue Jun 24, 2016
@bors-servo
Auto merge of #11695 - asajeffrey:script-thread-dont-panic, r=Ms2ger
654104e 
Removed some sources of panic from script thread.

<!-- Please describe your changes on the following line: -->
**This PR needs some thought!**

It removes some sources of panic from script_thread.rs, caused by pipeline lookup failure. It's incomplete, as there are some uses of `get_browsing_context` elsewhere in the code.

---
<!-- 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 #11693 and #11685 (and probably some other intermittents)
- [X] These changes do not require tests because it is fixing intermittent panic

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

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11695)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jun 24, 2016
@bors-servo
Auto merge of #11695 - asajeffrey:script-thread-dont-panic, r=Ms2ger
66d508d 
Removed some sources of panic from script thread.

<!-- Please describe your changes on the following line: -->
**This PR needs some thought!**

It removes some sources of panic from script_thread.rs, caused by pipeline lookup failure. It's incomplete, as there are some uses of `get_browsing_context` elsewhere in the code.

---
<!-- 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 #11693 and #11685 (and probably some other intermittents)
- [X] These changes do not require tests because it is fixing intermittent panic

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

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11695)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jun 24, 2016
@bors-servo
Auto merge of #11695 - asajeffrey:script-thread-dont-panic, r=Ms2ger
Loading status checks…
0427112 
Removed some sources of panic from script thread.

<!-- Please describe your changes on the following line: -->
**This PR needs some thought!**

It removes some sources of panic from script_thread.rs, caused by pipeline lookup failure. It's incomplete, as there are some uses of `get_browsing_context` elsewhere in the code.

---
<!-- 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 #11693 and #11685 (and probably some other intermittents)
- [X] These changes do not require tests because it is fixing intermittent panic

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

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11695)
<!-- Reviewable:end -->
@timvandermeij
Copy link
Contributor

@timvandermeij timvandermeij commented Jun 25, 2016

Fixed by #11695?
@cbrewster
Copy link
Member

@cbrewster cbrewster commented Jun 25, 2016

@timvandermeij not yet, there still some uses of panic! in the script thread.
@asajeffrey
Copy link
Member Author

@asajeffrey asajeffrey commented Jan 12, 2017

The one remaining panic! which has a race condition is:

panic!("Layout attached to empty script thread."),

which should be fixed by #14885.
@asajeffrey asajeffrey closed this Jan 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@asajeffrey asajeffrey

Labels
A-content/script I-panic
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
@asajeffrey @timvandermeij @cbrewster
You can’t perform that action at this time.