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

Added a safety check before an unsafe array operation #4006

Merged
merged 2 commits into from
Jun 10, 2019

Conversation

UberNick
Copy link
Contributor

@UberNick UberNick commented May 22, 2019

Issue Link 🔗

#4001

Goals ⚽

This code change helps safeguard against hard crashes caused by an array operation. It doesn't fix potential issues that could cause such a condition in the first place.

Testing Details 🔍

I manually stepped through the attached demo project and verified the PieChart views looked and behaved as expected.

@@ -880,7 +880,9 @@ open class PieChartRenderer: DataRenderer

// Prepend selected slices before the already rendered unselected ones.
// NOTE: - This relies on drawDataSet() being called before drawHighlighted in PieChartView.
accessibleChartElements.insert(contentsOf: highlightedAccessibleElements, at: 1)
if !accessibleChartElements.isEmpty {
Copy link
Member

@liuxuan30 liuxuan30 May 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A) I checked the code, it seems accessibleChartElements will have at least one element in drawDataSet normally, unless drawDataSet exits early, such as

    @objc open func drawDataSet(context: CGContext, dataSet: IPieChartDataSet)
    {
        guard let chart = chart else {return }

otherwise it should have at least 1 default element and insert(:_, at: 1) won't crash then. I don't think a low memory case would cause the crash here, if it's really true, then I think it's Swift bug rather than ours.

Now since you said it's crashed, I suppose above guard failed probably. Can you double check?

B) The safe check seems harmless to me, and I'm ok to merge here. But we need to discuss a little more before we merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this, @liuxuan30. I agree with your assessment that this condition shouldn't happen through normal use, and the rarity of the error among our user base combined with my inability to reproduce the problem reinforces that conclusion.

In my use case, Charts elements are being used within recycled TableViewCells and being driven by asynchronous data requests. Perhaps the error only occurs due to a rare race-condition or thread-related problem. One that may be more likely, perhaps even necessary, on devices where asynchronous processes are being suspended or terminated due to out-of-memory issues.

This is total speculation from me, and I don't have data to support the above statement. In the absence of other data, though, I think it's a good guess that's reasonably addressed by this otherwise innocuous PR.

@liuxuan30 liuxuan30 merged commit e850593 into ChartsOrg:master Jun 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants