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

Added a safety check before an unsafe array operation #4006

Merged
merged 2 commits into from
Jun 10, 2019
Merged

Added a safety check before an unsafe array operation #4006

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
03616c9
Added a safety check before an unsafe array operation
UberNick May 22, 2019
e6b01f7
Update Info.plist
liuxuan30 Jun 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion Source/Charts/Renderers/PieChartRenderer.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

accessibleChartElements.insert(contentsOf: highlightedAccessibleElements, at: 1)
}

context.restoreGState()
}
Expand Down