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

Fix bug in CFArray::to_untyped that made the retain count wrong #139

Merged
merged 4 commits into from Nov 28, 2017

Conversation

@faern
Copy link
Contributor

faern commented Nov 28, 2017

The following code segfaults on the 0.4.5 release:

CFArray::<CFBoolean>::from_CFTypes(&[]).to_untyped().to_untyped().to_untyped()

Because to_untyped don't increment the retain count, but when the typed array it's created from goes out of scope that one will decrement it.

And since we need to wrap under the get rule (call CFRetain) anyway I did not see any reason to have it as a consuming to_ method. It's more flexible as a borrowing as_ method.

Since you probably don't want to cause API breakage I kept the old version but fixed the bug and added a deprecation notice on it.


This change is Reviewable

@jdm
Copy link
Member

jdm commented Nov 28, 2017

Let's increase the patch version number and publish a new version as part of this PR. Can we add a test, too?

@faern
Copy link
Contributor Author

faern commented Nov 28, 2017

Sure. I'll fix that! Just a moment.

@faern
Copy link
Contributor Author

faern commented Nov 28, 2017

@jdm Done! I also fixed another bug with CFType::instance_of. At least I think it was a bug. @nox thought so too from a short description on IRC.

@faern
Copy link
Contributor Author

faern commented Nov 28, 2017

I realize I only tested as_untyped. As long as we keep the to_untyped version we might want to test that as well maybe? Adding to same test...

@faern faern force-pushed the faern:fix-to-untyped-array branch from 6d88b3d to c33598d Nov 28, 2017
@jdm
Copy link
Member

jdm commented Nov 28, 2017

@bors-servo: r+

@bors-servo
Copy link
Contributor

bors-servo commented Nov 28, 2017

📌 Commit c33598d has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Nov 28, 2017

Testing commit c33598d with merge 665fe0f...

bors-servo added a commit that referenced this pull request Nov 28, 2017
Fix bug in CFArray::to_untyped that made the retain count wrong

The following code segfaults on the `0.4.5` release:
```rust
CFArray::<CFBoolean>::from_CFTypes(&[]).to_untyped().to_untyped().to_untyped()
```
Because `to_untyped` don't increment the retain count, but when the typed array it's created from goes out of scope that one will decrement it.

And since we need to wrap under the get rule (call `CFRetain`) anyway I did not see any reason to have it as a consuming `to_` method. It's more flexible as a borrowing `as_` method.

Since you probably don't want to cause API breakage I kept the old version but fixed the bug and added a deprecation notice on it.

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

bors-servo commented Nov 28, 2017

☀️ Test successful - status-travis
Approved by: jdm
Pushing 665fe0f to master...

@bors-servo bors-servo merged commit c33598d into servo:master Nov 28, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@faern faern deleted the faern:fix-to-untyped-array branch Nov 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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