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.

Sign up for GitHub

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

Prevent the constellation from panicking if there is a deserialization error #15618

Merged
merged 1 commit into from Feb 20, 2017
Merged

Prevent the constellation from panicking if there is a deserialization error #15618

merged 1 commit into from Feb 20, 2017

Conversation

asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Feb 17, 2017 •
edited by larsbergstrom

At the moment, the constellation panics if there is a deserialization error on an ipc channel. This happens in /html/browsers/history/the-location-interface/location-protocol-setter-non-broken.html due to the URL deserialization errors caused by servo/rust-url#278.

This PR stops the constellation from panicking, so we can get a crash report for deserialization errors.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes do not require tests because we don't test panic recovery

This change is Reviewable

@asajeffrey asajeffrey added A-constellation Involves the constellation I-panic Servo encounters a panic. labels Feb 17, 2017
@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Feb 17, 2017
@nox
Copy link
Contributor

nox commented Feb 17, 2017

Let's not merge this now, I don't want any more bitrot on my serde bump, and the Error type changed.

@asajeffrey asajeffrey added the S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. label Feb 17, 2017
@asajeffrey asajeffrey mentioned this pull request Feb 17, 2017
Merged
@Manishearth
Copy link
Member

r=me

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #15588) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Feb 18, 2017
@antrik
Copy link
Contributor

antrik commented Feb 19, 2017

I'm a little confused here. Since the de-serialisation in ipc-channel is only done on messages created by the corresponding serialiser implementation, I don't think we can get actual de-serialisation errors, unless the serialiser or de-serialiser implementation is buggy, or if there are I/O errors in the IPC itself?...

Either way, it does seem a valid concern, since I/O errors at least can happen due to circumstances out of our control. (Such as the sender process dying in the middle of a large transfer; or possibly some kind of operating system resource exhaustion...) As such, I'd say this should be addressed in ipc-channel, rather then just working around it for this specific case?...

The downside is that switching between MPSC channels and IPC channels becomes less transparent this way -- but then again, when routing is needed, it's not really transparent anyway...

@nox
Copy link
Contributor

nox commented Feb 19, 2017

I'm a little confused here. Since the de-serialisation in ipc-channel is only done on messages created by the corresponding serialiser implementation, I don't think we can get actual de-serialisation errors, unless the serialiser or de-serialiser implementation is buggy, or if there are I/O errors in the IPC itself?...

It is indeed a bug in rust-url that causes the deserialisation error, but the constellation really should never panic ever, so ipc-channel must provide a way to not panic even when there is such a bug somewhere.

@asajeffrey
Copy link
Member Author

Indeed, this can only happen if there are errors in the deserializer, this is bullet-proofing the constellation against such errors. Or at least trying to, it's hard to protect against double-panicking if the deserializer for panic messages panics. We could add this function to ipc-channel at some point, I'd rather not hold up this PR though.

@antrik
Copy link
Contributor

antrik commented Feb 20, 2017

Well, this didn't sound like an urgent issue... But it's your call of course :-)

@asajeffrey
Copy link
Member Author

Rebased now that #15588 has landed.

@bors-servo r=Manishearth

@bors-servo
Copy link
Contributor

📌 Commit aee1291 has been approved by Manishearth

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Feb 20, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit aee1291 with merge 524a117...

bors-servo pushed a commit that referenced this pull request Feb 20, 2017
Auto merge of #15618 - asajeffrey:constellation-ipc-dont-panic, r=Man…
524a117 
…ishearth

Prevent the constellation from panicking if there is a deserialization error

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

At the moment, the constellation panics if there is a deserialization error on an ipc channel. This happens in `/html/browsers/history/the-location-interface/location-protocol-setter-non-broken.html` due to the URL deserialization errors caused by servo/rust-url#278.

This PR stops the constellation from panicking, so we can get a crash report for deserialization errors.

---
<!-- 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 do not require tests because we don't test panic recovery

<!-- 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="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15618)
<!-- Reviewable:end -->
@asajeffrey asajeffrey removed the S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. label Feb 20, 2017
@bors-servo
Copy link
Contributor

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
Approved by: Manishearth
Pushing 524a117 to master...

@bors-servo bors-servo merged commit aee1291 into servo:master Feb 20, 2017
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Feb 20, 2017
@jdm jdm mentioned this pull request Mar 23, 2017
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Manishearth Manishearth

Labels
A-constellation Involves the constellation I-panic Servo encounters a panic.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@asajeffrey @nox @Manishearth @bors-servo @antrik @highfive