Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit c1c0d04

Browse filesBrowse files
etpinardetpinard
authored
Merge pull request plotly#3199 from plotly/catch-nan-while-loops
Guard against infinite loops during scattergl line/fill positions cleanup
2 parents 6fda595 + c19e727 commit c1c0d04
Copy full SHA for c1c0d04

File tree

3 files changed

+109
-35
lines changed
Filter options

3 files changed

+109
-35
lines changed

‎src/traces/scattergl/index.js

Copy file name to clipboardExpand all lines: src/traces/scattergl/index.js
+25-24Lines changed: 25 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -395,18 +395,17 @@ function plot(gd, subplot, cdata) {
395395
scene.line2d.update(scene.lineOptions);
396396
scene.lineOptions = scene.lineOptions.map(function(lineOptions) {
397397
if(lineOptions && lineOptions.positions) {
398-
var pos = [], srcPos = lineOptions.positions;
398+
var srcPos = lineOptions.positions;
399399

400400
var firstptdef = 0;
401-
while(isNaN(srcPos[firstptdef]) || isNaN(srcPos[firstptdef + 1])) {
401+
while(firstptdef < srcPos.length && (isNaN(srcPos[firstptdef]) || isNaN(srcPos[firstptdef + 1]))) {
402402
firstptdef += 2;
403403
}
404404
var lastptdef = srcPos.length - 2;
405-
while(isNaN(srcPos[lastptdef]) || isNaN(srcPos[lastptdef + 1])) {
406-
lastptdef += -2;
405+
while(lastptdef > firstptdef && (isNaN(srcPos[lastptdef]) || isNaN(srcPos[lastptdef + 1]))) {
406+
lastptdef -= 2;
407407
}
408-
pos = pos.concat(srcPos.slice(firstptdef, lastptdef + 2));
409-
lineOptions.positions = pos;
408+
lineOptions.positions = srcPos.slice(firstptdef, lastptdef + 2);
410409
}
411410
return lineOptions;
412411
});
@@ -437,36 +436,38 @@ function plot(gd, subplot, cdata) {
437436
if(trace._nexttrace) fillData.push(i + 1);
438437
if(fillData.length) scene.fillOrder[i] = fillData;
439438

440-
var pos = [], srcPos = (lineOptions && lineOptions.positions) || stash.positions;
439+
var pos = [];
440+
var srcPos = (lineOptions && lineOptions.positions) || stash.positions;
441+
var firstptdef, lastptdef;
441442

442443
if(trace.fill === 'tozeroy') {
443-
var firstpdef = 0;
444-
while(isNaN(srcPos[firstpdef + 1])) {
445-
firstpdef += 2;
444+
firstptdef = 0;
445+
while(firstptdef < srcPos.length && isNaN(srcPos[firstptdef + 1])) {
446+
firstptdef += 2;
446447
}
447-
var lastpdef = srcPos.length - 2;
448-
while(isNaN(srcPos[lastpdef + 1])) {
449-
lastpdef += -2;
448+
lastptdef = srcPos.length - 2;
449+
while(lastptdef > firstptdef && isNaN(srcPos[lastptdef + 1])) {
450+
lastptdef -= 2;
450451
}
451-
if(srcPos[firstpdef + 1] !== 0) {
452-
pos = [ srcPos[firstpdef], 0 ];
452+
if(srcPos[firstptdef + 1] !== 0) {
453+
pos = [srcPos[firstptdef], 0];
453454
}
454-
pos = pos.concat(srcPos.slice(firstpdef, lastpdef + 2));
455-
if(srcPos[lastpdef + 1] !== 0) {
456-
pos = pos.concat([ srcPos[lastpdef], 0 ]);
455+
pos = pos.concat(srcPos.slice(firstptdef, lastptdef + 2));
456+
if(srcPos[lastptdef + 1] !== 0) {
457+
pos = pos.concat([srcPos[lastptdef], 0]);
457458
}
458459
}
459460
else if(trace.fill === 'tozerox') {
460-
var firstptdef = 0;
461-
while(isNaN(srcPos[firstptdef])) {
461+
firstptdef = 0;
462+
while(firstptdef < srcPos.length && isNaN(srcPos[firstptdef])) {
462463
firstptdef += 2;
463464
}
464-
var lastptdef = srcPos.length - 2;
465-
while(isNaN(srcPos[lastptdef])) {
466-
lastptdef += -2;
465+
lastptdef = srcPos.length - 2;
466+
while(lastptdef > firstptdef && isNaN(srcPos[lastptdef])) {
467+
lastptdef -= 2;
467468
}
468469
if(srcPos[firstptdef] !== 0) {
469-
pos = [ 0, srcPos[firstptdef + 1] ];
470+
pos = [0, srcPos[firstptdef + 1]];
470471
}
471472
pos = pos.concat(srcPos.slice(firstptdef, lastptdef + 2));
472473
if(srcPos[lastptdef] !== 0) {

‎test/jasmine/tests/config_test.js

Copy file name to clipboardExpand all lines: test/jasmine/tests/config_test.js
+11-11Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -608,30 +608,30 @@ describe('config argument', function() {
608608
gd = parent.childNodes[0];
609609
}
610610

611-
it('should resize when the viewport width/height changes', function(done) {
611+
it('@flaky should resize when the viewport width/height changes', function(done) {
612612
fillParent(1, 1);
613613
Plotly.plot(gd, data, {}, {responsive: true})
614614
.then(testResponsive)
615615
.then(done);
616616
});
617617

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

626-
it('should still be responsive if the plot is purged and replotted', function(done) {
626+
it('@flaky should still be responsive if the plot is purged and replotted', function(done) {
627627
fillParent(1, 1);
628628
Plotly.plot(gd, data, {}, {responsive: true})
629629
.then(function() {return Plotly.newPlot(gd, data, {}, {responsive: true});})
630630
.then(testResponsive)
631631
.then(done);
632632
});
633633

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

653-
it('should become responsive if configured as such via Plotly.react', function(done) {
653+
it('@flaky should become responsive if configured as such via Plotly.react', function(done) {
654654
fillParent(1, 1);
655655
Plotly.plot(gd, data, {}, {responsive: false})
656656
.then(function() {return Plotly.react(gd, data, {}, {responsive: true});})
657657
.then(testResponsive)
658658
.then(done);
659659
});
660660

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

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

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

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

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

741741
// The following test is to guarantee we're not breaking the existing (although maybe not ideal) behaviour.
742742
// In a future version, one may prefer responsive/autosize:true winning over an explicit width/height when embedded in a webpage.
743-
it('should use the explicitly provided width/height even if autosize/responsive:true', function(done) {
743+
it('@flaky should use the explicitly provided width/height even if autosize/responsive:true', function(done) {
744744
fillParent(1, 1, function() {
745745
this.style.width = '1000px';
746746
this.style.height = '500px';

‎test/jasmine/tests/gl2d_plot_interact_test.js

Copy file name to clipboardExpand all lines: test/jasmine/tests/gl2d_plot_interact_test.js
+73Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1186,6 +1186,79 @@ describe('Test gl2d plots', function() {
11861186
.catch(failTest)
11871187
.then(done);
11881188
});
1189+
1190+
it('@gl should not cause infinite loops when coordinate arrays start/end with NaN', function(done) {
1191+
function _assertPositions(msg, cont, exp) {
1192+
var pos = gd._fullLayout._plots.xy._scene[cont]
1193+
.map(function(opt) { return opt.positions; });
1194+
expect(pos).toBeCloseTo2DArray(exp, 2, msg);
1195+
}
1196+
1197+
Plotly.plot(gd, [{
1198+
type: 'scattergl',
1199+
mode: 'lines',
1200+
x: [1, 2, 3],
1201+
y: [null, null, null]
1202+
}, {
1203+
type: 'scattergl',
1204+
mode: 'lines',
1205+
x: [1, 2, 3],
1206+
y: [1, 2, null]
1207+
}, {
1208+
type: 'scattergl',
1209+
mode: 'lines',
1210+
x: [null, 2, 3],
1211+
y: [1, 2, 3]
1212+
}, {
1213+
type: 'scattergl',
1214+
mode: 'lines',
1215+
x: [null, null, null],
1216+
y: [1, 2, 3]
1217+
}, {
1218+
}])
1219+
.then(function() {
1220+
_assertPositions('base', 'lineOptions', [
1221+
[],
1222+
[1, 1, 2, 2],
1223+
[2, 2, 3, 3],
1224+
[]
1225+
]);
1226+
1227+
return Plotly.restyle(gd, 'fill', 'tozerox');
1228+
})
1229+
.then(function() {
1230+
_assertPositions('tozerox', 'lineOptions', [
1231+
[],
1232+
[1, 1, 2, 2],
1233+
[2, 2, 3, 3],
1234+
[]
1235+
]);
1236+
_assertPositions('tozerox', 'fillOptions', [
1237+
[0, undefined, 0, undefined],
1238+
[0, 1, 1, 1, 2, 2, 0, 2],
1239+
[0, 2, 2, 2, 3, 3, 0, 3],
1240+
[0, undefined, 0, undefined]
1241+
]);
1242+
1243+
return Plotly.restyle(gd, 'fill', 'tozeroy');
1244+
})
1245+
.then(function() {
1246+
_assertPositions('tozeroy', 'lineOptions', [
1247+
[],
1248+
[1, 1, 2, 2],
1249+
[2, 2, 3, 3],
1250+
[]
1251+
]);
1252+
_assertPositions('tozeroy', 'fillOptions', [
1253+
[undefined, 0, undefined, 0],
1254+
[1, 0, 1, 1, 2, 2, 2, 0],
1255+
[2, 0, 2, 2, 3, 3, 3, 0],
1256+
[undefined, 0, undefined, 0]
1257+
]);
1258+
})
1259+
.catch(failTest)
1260+
.then(done);
1261+
});
11891262
});
11901263

11911264
describe('Test scattergl autorange:', function() {

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.