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.

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

TileMapServiceImageryProvider defaults can cause the browser to hang. #8448

Merged

Conversation

DanielLeone
Copy link
Contributor

Hi all, we came across this exciting state a while back where the browser would just hang because of a bad imagery provider configuration.

Here's a sandcastle which will almost hang the browser. If you increase the minimumDetail to >=9 and run it again, you're in trouble.

And here's the code from the sandcastle for reference:

var viewer = new Cesium.Viewer('cesiumContainer');
var imageryProvider = new Cesium.TileMapServiceImageryProvider({
    url : 'some/imagery/provider/that/used/to/exist',
    minimumLevel: 7, // this server has layers at level 4 and 5, but they were too low quality to use
    rectangle: undefined // I'm not sure of the exact coordinates, but I know the rectangle is only going to be small
});

viewer.imageryLayers.addImageryProvider(imageryProvider);

Basically what's happening here is we've got a TMS layer configuration that's forcing the minimum detail level to 7 (for whatever reason), but not providing the rectangle. I understand that's probably not a good start, but I think the use case is still valid (as explained in the code comments).
Then what happens is the tilemapresource.xml request for the imagery provider fails, and the failure path currently uses some default values and continues on.

So, if you have minimum detail set to something >= ~9, and the metadata request fails, then you end up with a config of:
bounding-box: the whole globe (cesium failure path default)
minimum-detail: 9 (static config passed by user which trumps the defaults)

This just causes way too many tiles to be requested, and the browser locks up for a very long time.
This PR fixes this issue by just rejecting the readyPromise if the metadata request fails, but there's obviously a bunch of other ways to fix it too, that's just the fix we're currently using ourselves and it's working fine.

Other ways I could think of were:

  • copy the code which forces the minimumDetail back to zero if there's going to be too many tiles requested; this code exists in the success code path but not the failure code path.
  • just make the browser not lock up (some more chunking/timeouts/magic in the tile request logic)

The reason I've opted for this solution is that it brings the TileMapServiceImageryProvider inline with the other ImageryProviders that also request some kind of metadata.

ArcGisMapServer - fails
image

GoogleEarth - fails
image

BingMaps - fails
image

...plus some more

Apologies for the changing the tests so much, but it's really just removing some duplication since most of the tests were previously relying on this behaviour I've now changed, so there's nothing too exciting going on there.

Cheers! 👍 😄

… metadata request fails. This brings it inline with all the other ImageryProviders that use a metadata resource.
@cesium-concierge
Copy link

Thank you so much for the pull request @DanielLeone! I noticed this is your first pull request and I wanted to say welcome to the Cesium community!

The Pull Request Guidelines is a handy reference for making sure your PR gets accepted quickly, so make sure to skim that.

  • ❌ Missing CONTRIBUTORS.md entry.
  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@cesium-concierge
Copy link

Thanks again for your contribution @DanielLeone!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@hpinkos
Copy link
Contributor

hpinkos commented Jan 29, 2020

Thanks for the PR @DanielLeone, we will try to review this soon. @kring do you have time to look at this?

@kring
Copy link
Member

kring commented Jan 30, 2020

Thanks for the PR @DanielLeone!

Cesium has always treated tilemapresource.xml as optional, and it would potentially break people to change that. I think a reasonable solution is to copy (or better: extract to a separate function and call) this logic in the success path to the failure path as well:

        // Check the number of tiles at the minimum level.  If it's more than four,
        // try requesting the lower levels anyway, because starting at the higher minimum
        // level will cause too many tiles to be downloaded and rendered.
        var swTile = tilingScheme.positionToTileXY(Rectangle.southwest(rectangle), minimumLevel);
        var neTile = tilingScheme.positionToTileXY(Rectangle.northeast(rectangle), minimumLevel);
        var tileCount = (Math.abs(neTile.x - swTile.x) + 1) * (Math.abs(neTile.y - swTile.y) + 1);
        if (tileCount > 4) {
            minimumLevel = 0;
        }

Of course this isn't great, because it will just silently ignore that minimumLevel. But it's better than crashing!

Longer term, it would be nice to have a way to tell Cesium to simply not render the imagery provider at all until you reach a particular zoom level.

…forced to zero for time map service imagery provider

updates CHANGES.md
added another test
…e-rejection

# Conflicts:
#	CHANGES.md
#	CONTRIBUTORS.md
@DanielLeone
Copy link
Contributor Author

Done @kring 😄 . I agree it's a bit of a bold move to make the resource non-optional; that's fair.

I've done what you suggested, and added another test.
Hopefully the CI gods serve this PR a little nicer this time around; I'm not sure what the failures were last time 😛

🌮

@hpinkos
Copy link
Contributor

hpinkos commented Feb 26, 2020

@DanielLeone can you please merge in master?

@kring
Copy link
Member

kring commented Mar 11, 2020

Thanks again @DanielLeone!

@kring kring merged commit d48ac3e into CesiumGS:master Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants