Skip to content

Commit

Permalink
Merge pull request #3199 from plotly/catch-nan-while-loops
Browse files Browse the repository at this point in the history 
Guard against infinite loops during scattergl line/fill positions cleanup
  • Loading branch information
@etpinard
etpinard authored Oct 31, 2018
2 parents 6fda595 + c19e727 commit c1c0d04
Show file tree
Hide file tree
Showing 3 changed files with 109 additions and 35 deletions.
49 changes: 25 additions & 24 deletions src/traces/scattergl/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -395,18 +395,17 @@ function plot(gd, subplot, cdata) {
scene.line2d.update(scene.lineOptions);
scene.lineOptions = scene.lineOptions.map(function(lineOptions) {
if(lineOptions && lineOptions.positions) {
var pos = [], srcPos = lineOptions.positions;
var srcPos = lineOptions.positions;

var firstptdef = 0;
while(isNaN(srcPos[firstptdef]) || isNaN(srcPos[firstptdef + 1])) {
while(firstptdef < srcPos.length && (isNaN(srcPos[firstptdef]) || isNaN(srcPos[firstptdef + 1]))) {
firstptdef += 2;
}
var lastptdef = srcPos.length - 2;
while(isNaN(srcPos[lastptdef]) || isNaN(srcPos[lastptdef + 1])) {
lastptdef += -2;
while(lastptdef > firstptdef && (isNaN(srcPos[lastptdef]) || isNaN(srcPos[lastptdef + 1]))) {
lastptdef -= 2;
}
pos = pos.concat(srcPos.slice(firstptdef, lastptdef + 2));
lineOptions.positions = pos;
lineOptions.positions = srcPos.slice(firstptdef, lastptdef + 2);
}
return lineOptions;
});
Expand Down Expand Up @@ -437,36 +436,38 @@ function plot(gd, subplot, cdata) {
if(trace._nexttrace) fillData.push(i + 1);
if(fillData.length) scene.fillOrder[i] = fillData;

var pos = [], srcPos = (lineOptions && lineOptions.positions) || stash.positions;
var pos = [];
var srcPos = (lineOptions && lineOptions.positions) || stash.positions;
var firstptdef, lastptdef;

if(trace.fill === 'tozeroy') {
var firstpdef = 0;
while(isNaN(srcPos[firstpdef + 1])) {
firstpdef += 2;
firstptdef = 0;
while(firstptdef < srcPos.length && isNaN(srcPos[firstptdef + 1])) {
firstptdef += 2;
}
var lastpdef = srcPos.length - 2;
while(isNaN(srcPos[lastpdef + 1])) {
lastpdef += -2;
lastptdef = srcPos.length - 2;
while(lastptdef > firstptdef && isNaN(srcPos[lastptdef + 1])) {
lastptdef -= 2;
}
if(srcPos[firstpdef + 1] !== 0) {
pos = [ srcPos[firstpdef], 0 ];
if(srcPos[firstptdef + 1] !== 0) {
pos = [srcPos[firstptdef], 0];
}
pos = pos.concat(srcPos.slice(firstpdef, lastpdef + 2));
if(srcPos[lastpdef + 1] !== 0) {
pos = pos.concat([ srcPos[lastpdef], 0 ]);
pos = pos.concat(srcPos.slice(firstptdef, lastptdef + 2));
if(srcPos[lastptdef + 1] !== 0) {
pos = pos.concat([srcPos[lastptdef], 0]);
}
}
else if(trace.fill === 'tozerox') {
var firstptdef = 0;
while(isNaN(srcPos[firstptdef])) {
firstptdef = 0;
while(firstptdef < srcPos.length && isNaN(srcPos[firstptdef])) {
firstptdef += 2;
}
var lastptdef = srcPos.length - 2;
while(isNaN(srcPos[lastptdef])) {
lastptdef += -2;
lastptdef = srcPos.length - 2;
while(lastptdef > firstptdef && isNaN(srcPos[lastptdef])) {
lastptdef -= 2;
}
if(srcPos[firstptdef] !== 0) {
pos = [ 0, srcPos[firstptdef + 1] ];
pos = [0, srcPos[firstptdef + 1]];
}
pos = pos.concat(srcPos.slice(firstptdef, lastptdef + 2));
if(srcPos[lastptdef] !== 0) {
Expand Down
22 changes: 11 additions & 11 deletions test/jasmine/tests/config_test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -608,30 +608,30 @@ describe('config argument', function() {
gd = parent.childNodes[0];
}

it('should resize when the viewport width/height changes', function(done) {
it('@flaky should resize when the viewport width/height changes', function(done) {
fillParent(1, 1);
Plotly.plot(gd, data, {}, {responsive: true})
.then(testResponsive)
.then(done);
});

it('should still be responsive if the plot is edited', function(done) {
it('@flaky should still be responsive if the plot is edited', function(done) {
fillParent(1, 1);
Plotly.plot(gd, data, {}, {responsive: true})
.then(function() {return Plotly.restyle(gd, 'y[0]', data[0].y[0] + 2);})
.then(testResponsive)
.then(done);
});

it('should still be responsive if the plot is purged and replotted', function(done) {
it('@flaky should still be responsive if the plot is purged and replotted', function(done) {
fillParent(1, 1);
Plotly.plot(gd, data, {}, {responsive: true})
.then(function() {return Plotly.newPlot(gd, data, {}, {responsive: true});})
.then(testResponsive)
.then(done);
});

it('should only have one resize handler when plotted more than once', function(done) {
it('@flaky should only have one resize handler when plotted more than once', function(done) {
fillParent(1, 1);
var cntWindowResize = 0;
window.addEventListener('resize', function() {cntWindowResize++;});
Expand All @@ -650,15 +650,15 @@ describe('config argument', function() {
.then(done);
});

it('should become responsive if configured as such via Plotly.react', function(done) {
it('@flaky should become responsive if configured as such via Plotly.react', function(done) {
fillParent(1, 1);
Plotly.plot(gd, data, {}, {responsive: false})
.then(function() {return Plotly.react(gd, data, {}, {responsive: true});})
.then(testResponsive)
.then(done);
});

it('should stop being responsive if configured as such via Plotly.react', function(done) {
it('@flaky should stop being responsive if configured as such via Plotly.react', function(done) {
fillParent(1, 1);
Plotly.plot(gd, data, {}, {responsive: true})
// Check initial size
Expand All @@ -676,7 +676,7 @@ describe('config argument', function() {
});

// Testing fancier CSS layouts
it('should resize horizontally in a flexbox when responsive: true', function(done) {
it('@flaky should resize horizontally in a flexbox when responsive: true', function(done) {
parent.style.display = 'flex';
parent.style.flexDirection = 'row';
fillParent(1, 2, function() {
Expand All @@ -688,7 +688,7 @@ describe('config argument', function() {
.then(done);
});

it('should resize vertically in a flexbox when responsive: true', function(done) {
it('@flaky should resize vertically in a flexbox when responsive: true', function(done) {
parent.style.display = 'flex';
parent.style.flexDirection = 'column';
fillParent(2, 1, function() {
Expand All @@ -700,7 +700,7 @@ describe('config argument', function() {
.then(done);
});

it('should resize in both direction in a grid when responsive: true', function(done) {
it('@flaky should resize in both direction in a grid when responsive: true', function(done) {
var numCols = 2, numRows = 2;
parent.style.display = 'grid';
parent.style.gridTemplateColumns = 'repeat(' + numCols + ', 1fr)';
Expand All @@ -712,7 +712,7 @@ describe('config argument', function() {
.then(done);
});

it('should provide a fixed non-zero width/height when autosize/responsive: true and container\' size is zero', function(done) {
it('@flaky should provide a fixed non-zero width/height when autosize/responsive: true and container\' size is zero', function(done) {
fillParent(1, 1, function() {
this.style.display = 'inline-block';
this.style.width = null;
Expand Down Expand Up @@ -740,7 +740,7 @@ describe('config argument', function() {

// The following test is to guarantee we're not breaking the existing (although maybe not ideal) behaviour.
// In a future version, one may prefer responsive/autosize:true winning over an explicit width/height when embedded in a webpage.
it('should use the explicitly provided width/height even if autosize/responsive:true', function(done) {
it('@flaky should use the explicitly provided width/height even if autosize/responsive:true', function(done) {
fillParent(1, 1, function() {
this.style.width = '1000px';
this.style.height = '500px';
Expand Down
73 changes: 73 additions & 0 deletions test/jasmine/tests/gl2d_plot_interact_test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1186,6 +1186,79 @@ describe('Test gl2d plots', function() {
.catch(failTest)
.then(done);
});

it('@gl should not cause infinite loops when coordinate arrays start/end with NaN', function(done) {
function _assertPositions(msg, cont, exp) {
var pos = gd._fullLayout._plots.xy._scene[cont]
.map(function(opt) { return opt.positions; });
expect(pos).toBeCloseTo2DArray(exp, 2, msg);
}

Plotly.plot(gd, [{
type: 'scattergl',
mode: 'lines',
x: [1, 2, 3],
y: [null, null, null]
}, {
type: 'scattergl',
mode: 'lines',
x: [1, 2, 3],
y: [1, 2, null]
}, {
type: 'scattergl',
mode: 'lines',
x: [null, 2, 3],
y: [1, 2, 3]
}, {
type: 'scattergl',
mode: 'lines',
x: [null, null, null],
y: [1, 2, 3]
}, {
}])
.then(function() {
_assertPositions('base', 'lineOptions', [
[],
[1, 1, 2, 2],
[2, 2, 3, 3],
[]
]);

return Plotly.restyle(gd, 'fill', 'tozerox');
})
.then(function() {
_assertPositions('tozerox', 'lineOptions', [
[],
[1, 1, 2, 2],
[2, 2, 3, 3],
[]
]);
_assertPositions('tozerox', 'fillOptions', [
[0, undefined, 0, undefined],
[0, 1, 1, 1, 2, 2, 0, 2],
[0, 2, 2, 2, 3, 3, 0, 3],
[0, undefined, 0, undefined]
]);

return Plotly.restyle(gd, 'fill', 'tozeroy');
})
.then(function() {
_assertPositions('tozeroy', 'lineOptions', [
[],
[1, 1, 2, 2],
[2, 2, 3, 3],
[]
]);
_assertPositions('tozeroy', 'fillOptions', [
[undefined, 0, undefined, 0],
[1, 0, 1, 1, 2, 2, 2, 0],
[2, 0, 2, 2, 3, 3, 3, 0],
[undefined, 0, undefined, 0]
]);
})
.catch(failTest)
.then(done);
});
});

describe('Test scattergl autorange:', function() {
Expand Down

0 comments on commit c1c0d04

Please sign in to comment.