Skip to content
This repository was archived by the owner on Nov 12, 2022. It is now read-only.

Make Handle objects take lifetimes #153

Closed
michaelwu opened this issue Jun 5, 2015 · 6 comments
Closed

Make Handle objects take lifetimes #153

michaelwu opened this issue Jun 5, 2015 · 6 comments

Comments

@michaelwu
Copy link
Contributor

Handles are currently dangerous right now since they can outlive the Roots that they're taken from. We should make them take lifetimes to avoid that issue. This api is introduced in #150
@michaelwu michaelwu mentioned this issue Jun 5, 2015
Merged
@jdm jdm mentioned this issue Jul 1, 2015
Closed
mmatyas pushed a commit to mmatyas/rust-mozjs that referenced this issue Jul 30, 2015
@jdm
Merge pull request servo#153 from mmatyas/canvas_clones
3e5daf6 
Add Clone trait for some structs.
@nox
Copy link
Contributor

nox commented Aug 31, 2015

Tried doing this, encountered rust-lang/rust#10501 on the *Op function types.

@jdm
Copy link
Member

jdm commented Aug 31, 2015

My attempt lives at https://github.com/jdm/rust-mozjs/tree/handle and https://github.com/jdm/servo/tree/handles .

Ms2ger added a commit that referenced this issue Jan 12, 2017
@Ms2ger
Clear the value of a Rooted when the RootedGuard goes out of scope.
557a1ef 
This ensures no unrooted values linger in the Rooted when it is no longer
rooted.

It should be impossible to access the value after that, but Handles currently
lack the lifetime to prevent that (#153).
@jdm jdm mentioned this issue May 1, 2017
Closed
tschneidereit pushed a commit to tschneidereit/rust-mozjs that referenced this issue Aug 26, 2017
@Ms2ger @fitzgen
Clear the value of a Rooted when the RootedGuard goes out of scope.
c7d04f4 
This ensures no unrooted values linger in the Rooted when it is no longer
rooted.

It should be impossible to access the value after that, but Handles currently
lack the lifetime to prevent that (servo#153).
@marmistrz
Copy link
Contributor

I'll work on this as a part of my Bachelor's thesis.

@marmistrz
Copy link
Contributor

marmistrz commented Dec 8, 2017 •
edited
Loading

@jdm leaving a comment here since crowbot seems to be absent.

<marmistrz> Hi! I took a look at the following, unmerged try for lifetimed-Handle objects: https://github.com/servo/rust-mozjs/issues/153#issuecomment-136517685 
<marmistrz> What was the reason it was not merged straightaway?
<marmistrz> At first glance it looks just fine. (without deep digging through the code) Was it simply not tested enough?
<emilio> marmistrz: I bet it's because jdm didn't end up finding the time to fix all the usages in Servo... but you can ask him directly when he's back

Feel free to reply on IRC if you prefer, I have a bouncer ;)

@jdm
Copy link
Member

jdm commented Dec 8, 2017

There were build errors that I encountered that I was not sure how to solve. It would have been smart if I had left a record of them anywhere, but I am not smart unfortunately.

@jdm
Copy link
Member

jdm commented Dec 8, 2017

Rather, the rust-mozjs PR seemed fine. It was the Servo one that ran into difficulties.

This was referenced Mar 27, 2018
Merged
Merged
bors-servo pushed a commit to servo/servo that referenced this issue Mar 27, 2018
Auto merge of #20246 - marmistrz:test-lifetime-handles, r=<try>
ce6a8b5 
Use higher level handles from JS bindings

…reak servo

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

---
<!-- 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 servo/rust-mozjs#153

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because this is just migration to a lifetimed API
<!-- 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. -->

<!-- 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/20246)
<!-- Reviewable:end -->
bors-servo pushed a commit to servo/servo that referenced this issue Mar 28, 2018
Auto merge of #20246 - marmistrz:test-lifetime-handles, r=<try>
9761c62 
Use higher level handles from JS bindings

…reak servo

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

---
<!-- 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 servo/rust-mozjs#153

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because this is just migration to a lifetimed API
<!-- 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. -->

<!-- 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/20246)
<!-- Reviewable:end -->
bors-servo pushed a commit to servo/servo that referenced this issue Mar 28, 2018
Auto merge of #20246 - marmistrz:test-lifetime-handles, r=<try>
fbede9a 
Use higher level handles from JS bindings

…reak servo

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

---
<!-- 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 servo/rust-mozjs#153

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because this is just migration to a lifetimed API
<!-- 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. -->

<!-- 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/20246)
<!-- Reviewable:end -->
bors-servo pushed a commit to servo/servo that referenced this issue Mar 28, 2018
Auto merge of #20246 - marmistrz:test-lifetime-handles, r=jdm
9149524 
Use higher level handles from JS bindings

…reak servo

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

---
<!-- 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 servo/rust-mozjs#153

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because this is just migration to a lifetimed API
<!-- 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. -->

<!-- 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/20246)
<!-- Reviewable:end -->
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 29, 2018
@marmistrz
servo: Merge #20246 - Use higher level handles from JS bindings (from…
cd3af52 
… marmistrz:test-lifetime-handles); r=jdm

…reak servo

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

---
<!-- 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 servo/rust-mozjs#153

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because this is just migration to a lifetimed API
<!-- 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. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 914952487bcbba0a31db8aefc4a9487903721959

--HG--
extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear
extra : subtree_revision : 42a821264f87e446c0ea44718656b99fb2c3541c
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 2, 2019
@marco-c
servo: Merge #20246 - Use higher level handles from JS bindings (from…
89c9b07 
… marmistrz:test-lifetime-handles); r=jdm

…reak servo

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

---
<!-- 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 servo/rust-mozjs#153

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because this is just migration to a lifetimed API
<!-- 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. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 914952487bcbba0a31db8aefc4a9487903721959

UltraBlame original commit: ec59359a065b37de1a7afde62551fb858e70a0bc
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 2, 2019
@marco-c
servo: Merge #20246 - Use higher level handles from JS bindings (from…
a142f88 
… marmistrz:test-lifetime-handles); r=jdm

…reak servo

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

---
<!-- 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 servo/rust-mozjs#153

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because this is just migration to a lifetimed API
<!-- 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. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 914952487bcbba0a31db8aefc4a9487903721959

UltraBlame original commit: ec59359a065b37de1a7afde62551fb858e70a0bc
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 2, 2019
@marco-c
servo: Merge #20246 - Use higher level handles from JS bindings (from…
f169efd 
… marmistrz:test-lifetime-handles); r=jdm

…reak servo

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

---
<!-- 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 servo/rust-mozjs#153

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because this is just migration to a lifetimed API
<!-- 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. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 914952487bcbba0a31db8aefc4a9487903721959

UltraBlame original commit: ec59359a065b37de1a7afde62551fb858e70a0bc
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@jdm @nox @michaelwu @marmistrz