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 3 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
47 changes: 25 additions & 22 deletions src/traces/scattergl/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,15 +395,16 @@ 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 pos = [];
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 > -1 && (isNaN(srcPos[lastptdef]) || isNaN(srcPos[lastptdef + 1]))) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

even better: while(lastptdef > firstptdef && ... would skip this second loop altogether in the failing case.

BTW do we need this pos = pos.concat... thing at all? Can't we just do lineOptions.positions = srcPos.slice(...)? It's needed in the fill section below but I don't see why it'd be needed here. In fact, even that slice seems like it could be omitted when we didn't drop any points on either end.

lastptdef -= 2;
}
pos = pos.concat(srcPos.slice(firstptdef, lastptdef + 2));
lineOptions.positions = pos;
Expand Down Expand Up @@ -437,36 +438,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 > -1 && 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 > -1 && 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
2 changes: 1 addition & 1 deletion test/jasmine/tests/config_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -615,7 +615,7 @@ describe('config argument', function() {
.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);})
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