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

Fix a bug preventing map.loaded() from returning true with raster tiles. #3302

Merged
merged 1 commit into from
Oct 24, 2016
Merged

Fix a bug preventing map.loaded() from returning true with raster tiles. #3302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 5 additions & 2 deletions js/source/raster_tile_source.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,13 @@ class RasterTileSource extends Evented {
function done(err, img) {
delete tile.request;

if (tile.aborted)
return;
if (tile.aborted) {
this.state = 'unloaded';
return callback(null);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this fixing another bug? If so, thank you! Out of curiosity: what were the symptoms of this bug? I'm guessing something along the lines of "map.loaded() doesn't return true if there are any tiles were aborted."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, technically, it's a seperate bug that manifested itself in the same way (preventing loaded() from being true.)

I'm guessing something along the lines of "map.loaded() doesn't return true if there are any tiles were aborted."

Yes, exactly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm interested in working with you to fix one of those two blockers

Happy to help with some guidance on how best to write tests for this.


if (err) {
this.state = 'errored';
return callback(err);
}

Expand Down
12 changes: 10 additions & 2 deletions js/util/ajax.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ exports.getArrayBuffer = function(url, callback) {
callback(e);
};
xhr.onload = function() {
if (xhr.response.byteLength === 0 && xhr.status === 200) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

specifically, this bit

return callback(new Error('http status 200 returned without content.'));
}
if (xhr.status >= 200 && xhr.status < 300 && xhr.response) {
callback(null, xhr.response);
} else {
Expand All @@ -50,6 +53,8 @@ function sameOrigin(url) {
return a.protocol === window.document.location.protocol && a.host === window.document.location.host;
}

const transparentPngUrl = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAAC0lEQVQYV2NgAAIAAAUAAarVyFEAAAAASUVORK5CYII=';

exports.getImage = function(url, callback) {
return exports.getArrayBuffer(url, (err, imgData) => {
if (err) return callback(err);
Expand All @@ -59,7 +64,11 @@ exports.getImage = function(url, callback) {
(window.URL || window.webkitURL).revokeObjectURL(img.src);
};
const blob = new window.Blob([new Uint8Array(imgData)], { type: 'image/png' });
img.src = (window.URL || window.webkitURL).createObjectURL(blob);
if (imgData.byteLength) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you decide to branch on imgData.byteLength rather than "status is 204 No Content"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not that familiar with the spec but I figured it was possible a response could include an empty PNG (with byteLength > 0) and still return a 204.

Copy link
Contributor

@lucaswoj lucaswoj Oct 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

According to the w3c spec, a 204 response may not include any body

10.2.5 204 No Content

...

The 204 response MUST NOT include a message-body, and thus is always terminated by the first empty line after the header fields.

My intuition is that we should keep the behaviour you have implemented in response to a 204 header and fail loudly if any other type of response contains no data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know that about 204. With that said, is an empty response from a 200 still valid? My intuition is yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

10.2.1 200 OK

The request has succeeded. The information returned with the response is dependent on the method used in the request, for example:

GET an entity corresponding to the requested resource is sent in the response;
...

My interpretation is that a 200 response must include a body. Per the spec, a trivial transparent PNG would be acceptable but the lack of a body would be unacceptable. In practice, a 200 response with no body is more likely a bug than intended behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lucaswoj I updated the PR. is this better?

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM! Now we need to figure out a way to test this code path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lucaswoj any ideas how best to do that?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is unfortunate that we don't have any unit tests for this module yet 😬. I suggest that we create a test/js/util/ajax.test.js file and add at least a test for this case. Sinon's window. useFakeXMLHttpRequest should make it relatively easy to simulate this case.

img.src = (window.URL || window.webkitURL).createObjectURL(blob);
} else {
img.src = transparentPngUrl;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we don't do this, the tile becomes black and webGL warnings are logged.

}
img.getData = function() {
const canvas = window.document.createElement('canvas');
const context = canvas.getContext('2d');
Expand All @@ -68,7 +77,6 @@ exports.getImage = function(url, callback) {
context.drawImage(img, 0, 0);
return context.getImageData(0, 0, img.width, img.height).data;
};
return img;
});
};

Expand Down
30 changes: 30 additions & 0 deletions test/js/util/ajax.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
'use strict';

const test = require('mapbox-gl-js-test').test;
const ajax = require('../../../js/util/ajax');
const window = require('../../../js/util/window');

test('ajax', (t) => {
t.beforeEach(callback => {
window.useFakeXMLHttpRequest();
callback();
});

t.afterEach(callback => {
window.restore();
callback();
});
t.test('getArrayBuffer', (t) => {
const url = '/buffer-request';
window.server.respondWith(request => {
request.respond(200, {'Content-Type': 'image/png'}, '');
});
ajax.getArrayBuffer(url, (error) => {
t.pass('called getArrayBuffer');
t.ok(error, 'should error when the status is 200 without content.');
t.end();
});
window.server.respond();
});
t.end();
});