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

[RCTLog] Use getters and setters #2494

Closed
wants to merge 1 commit into from
Closed

[RCTLog] Use getters and setters #2494

wants to merge 1 commit into from

Conversation

paramaggarwal
Copy link
Contributor

As per discussion in #2423 - possible fix for crash. (cc: @javache)

Please share feedback regarding the PR, we are going to be using this diff in production to see if it fixes the crashes we are seeing.

(fixes #2423)

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Aug 30, 2015
@javache javache self-assigned this Sep 1, 2015
@javache
Copy link
Member

javache commented Sep 1, 2015

I'd avoid creating a class just to hook in on load. Putting the dispatch_once in load is not very useful either, since it will only be executed when the program is getting read into memory. The timing would actually be very similar to __attribute__((constructor)) and might not be caught by crash reporting frameworks like breakpad.

If possible, I'd try to insert this dispatch_once wherever we try to use the default log function (not everything is going to the accessor method right now, so that would need to be changed.

Looking forward to hearing whether this change helps fixing the crash in production.

@paramaggarwal
Copy link
Contributor Author

@javache Yes, that is exactly my goal at the moment. To avoid using the __attribute__((constructor)) API and instead use some Objective-C construct. The fact that this doesn't really change anything is exactly what I am going for.

Let's see if this fixes our crashes. Will update.

@paramaggarwal
Copy link
Contributor Author

@javache I have rethought these changes based on your comments and updated PR. Please do share feedback. There is no constructor now - it simply uses the existing getters and setters.

@paramaggarwal paramaggarwal changed the title Move RCTLog initialisation to dispatch_once [RCTLog] Use getters and setters Sep 11, 2015
@javache
Copy link
Member

javache commented Sep 11, 2015

@facebook-github-bot import

@paramaggarwal
Copy link
Contributor Author

Thanks @javache.

@ghost ghost closed this in b998e5a Sep 13, 2015
@paramaggarwal
Copy link
Contributor Author

Our crashes are resolved. Thanks @javache for your help.

MattFoley pushed a commit to skillz/react-native that referenced this pull request Nov 9, 2015
@paramaggarwal
Use getters and setters
2dc9bc8 
Summary: As per discussion in facebook#2423 - possible fix for crash. (cc: @​javache)

Please share feedback regarding the PR, we are going to be using this diff in production to see if it fixes the crashes we are seeing.

(fixes facebook#2423)
Closes facebook#2494

Reviewed By: @javache

Differential Revision: D2433515

Pulled By: @nicklockwood
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@javache javache

Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Crash] EXC_BAD_INSTRUCTION (SIGILL) RCTLog.m:37
3 participants
@paramaggarwal @javache @facebook-github-bot