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

Improve bytearray handling #2222

Open
MarcSchoenefeld opened this issue Oct 9, 2018 · 5 comments
Open

Improve bytearray handling #2222

MarcSchoenefeld opened this issue Oct 9, 2018 · 5 comments

Comments

@MarcSchoenefeld
Copy link

@MarcSchoenefeld MarcSchoenefeld commented Oct 9, 2018

NewByteArray can fail and return null, consider checking before using the returned value as array

dest->javaBuffer = env->NewByteArray(kStreamBufferSize);

@oprisnik

This comment has been minimized.

Copy link
Contributor

@oprisnik oprisnik commented Oct 10, 2018

Thanks for the suggestions! Feel free to submit a pull request!

@azisuazusa

This comment has been minimized.

Copy link

@azisuazusa azisuazusa commented Nov 14, 2018

Hi @oprisnik I'm trying to work on this issue, may I know what behaviour that we expect when NewByteArray return null? Is run safeThrow enough?

@oprisnik

This comment has been minimized.

Copy link
Contributor

@oprisnik oprisnik commented Dec 4, 2018

Hey! Sorry for the delayed response. I guess throwing should be fine yes. We already throw a couple lines below when the other buffer is null:

if (dest->buffer == NULL) {
    jpegSafeThrow(
        (j_common_ptr) cinfo,
        "Failed to allcoate memory for byte buffer.");
  }
azisuazusa added a commit to azisuazusa/fresco that referenced this issue Dec 5, 2018
azisuazusa added a commit to azisuazusa/fresco that referenced this issue Dec 5, 2018
@iadeelzafar

This comment has been minimized.

Copy link
Contributor

@iadeelzafar iadeelzafar commented Nov 10, 2019

@oprisnik I cannot find this path fresco/imagepipeline/src/main/jni/imagepipeline/jpeg/jpeg_stream_wrappers.cpp

Is this issue still valid? If yes, then where is jpeg_stream_wrappers.cpp moved?

@iadeelzafar iadeelzafar mentioned this issue Nov 11, 2019
4 of 4 tasks complete
facebook-github-bot added a commit that referenced this issue Nov 27, 2019
Summary:
Thanks for submitting a PR! Please read these instructions carefully:

- [x] Explain the **motivation** for making this change.
- [x] Provide a **test plan** demonstrating that the code is solid.
- [x] Match the **code formatting** of the rest of the codebase.
- [x] Target the `master` branch

## Motivation (required)

What existing problem does the pull request solve?

It solves #2222

If you have added code that should be tested, add tests.

This code doesn't needs to be tested.

## Next Steps

Sign the [CLA][2], if you haven't already.

Small pull requests are much easier to review and more likely to get merged. Make sure the PR does only one thing, otherwise please split it.

Make sure all **tests pass** on [Circle CI][4]. PRs that break tests are unlikely to be merged.

For more info, see the [Contributing guide][4].

[1]: https://medium.com/martinkonicek/what-is-a-test-plan-8bfc840ec171#.y9lcuqqi9
[2]: https://code.facebook.com/cla
[3]: http://circleci.com/gh/facebook/fresco
[4]: https://github.com/facebook/fresco/blob/master/CONTRIBUTING.md
Pull Request resolved: #2432

Reviewed By: defHLT

Differential Revision: D18475182

Pulled By: oprisnik

fbshipit-source-id: dfaa773154551d1b699dd41a215b0d3334852487
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.