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

Guard against infinite loops during scattergl line/fill positions cleanup #3199

Merged
merged 7 commits into from
Oct 31, 2018
Merged

Guard against infinite loops during scattergl line/fill positions cleanup #3199

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4aca315
lint in scattergl.plot
etpinard Oct 31, 2018
0ce79bc
guard against infinite loops
etpinard Oct 31, 2018
8f627dc
add flaky to "responsive after update" test
etpinard Oct 31, 2018
dfdb1f4
(arrg) make the whole responsive suite flaky
etpinard Oct 31, 2018
5441ef9
simplify line position cleanup
etpinard Oct 31, 2018
2c8a290
aj-proof downward line position loop
etpinard Oct 31, 2018
c19e727
mv flaky tags from 'describe' -> 'it'
etpinard Oct 31, 2018
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
49 changes: 25 additions & 24 deletions src/traces/scattergl/index.js
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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