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 issues #4572 and #4703 #4959

Merged
merged 1 commit into from
Dec 12, 2017

Conversation

jcopperfield
Copy link
Contributor

Fixes issues #4572 and #4703

The endless loop that the browser ended up in was caused by the minNotZero being null.
The new determineDataLimits code should catch the edge cases.

Removed the options.relativePoints, as it is undocumented and didn't do anything useful. The same options is present in the file scales/scale.linear.js#L84; I doubt it does anything useful other than setting the max to 100.

 - issue chartjs#4572: logarithmic type if all numbers are zero browser crashes "Out of memory"
 - issue chartjs#4703: [BUG] Browser unresponsive on bubble chart with logarithmic axes
me.max = DEFAULT_MAX;
}
}
if (me.min === null) {
Copy link
Member

Choose a reason for hiding this comment

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

the case of me.min === null and me.max === null will be covered in the previous if, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct, it would be caught in if (me.min === me.max) { and would result in {min: 1, max: 10, minNotZero: 1}.


describe('when', function() {
var config = [
{
Copy link
Member

Choose a reason for hiding this comment

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

would it make sense to test ticks.min == 0 and ticks.max == 0 at the same time? or is that covered in another test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The determineDataLimits function would generate {min: 1, max: 10, minNotZero: 1} values. However the core.ticks.js#137 would create lastTick = 0 in this case causing the ticks to be: [0,1,2,3,4,5,6,7,8,9,0]. The same would happen with other invalid input.

The better question would be. Should Chart.js have a checking function for the configuration as it would allow invalid input ticks: {min: 10, max: 0} and this would bubble through the determineDataLimits and would cause problems in the tick generator?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's OK to not handle that case. The user would have had to explicitly set it that way which would not make sense.

In terms of general strategy, I'm not sure how much checking we want to do here. @simonbrunel @benmccann thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe, although the explicitly set option might be the result of some other function. Sanitizing the data and config, could make the code not having to deal with such situations.

@jcopperfield
Copy link
Contributor Author

@etimberg is anything missing in this PR that prevents merging?

@etimberg
Copy link
Member

@jcopperfield it looks good to me. Would love to get @simonbrunel to take a look too.

@etimberg etimberg merged commit f0bf395 into chartjs:master Dec 12, 2017
@jcopperfield jcopperfield deleted the PR-20171116-4572-4703 branch December 13, 2017 06:27
yofreke pushed a commit to yofreke/Chart.js that referenced this pull request Dec 30, 2017
- issue chartjs#4572: logarithmic type if all numbers are zero browser crashes "Out of memory"
 - issue chartjs#4703: [BUG] Browser unresponsive on bubble chart with logarithmic axes
@simonbrunel simonbrunel added this to the Version 2.7.2 milestone Mar 1, 2018
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
- issue chartjs#4572: logarithmic type if all numbers are zero browser crashes "Out of memory"
 - issue chartjs#4703: [BUG] Browser unresponsive on bubble chart with logarithmic axes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants