Make sure 204 image requests reject when using ImageBitmap

[OmarShehata]

This fixes #7906 (and most likely #7817 but the author didn't submit a reproducible example).

The issue was that Resource._Implementations.createImage was not rejecting the promise when it received an undefined blob (which happened when the request was 204, empty content). In fact, it was neither resolving not rejecting. This led to the imagery provider launching more and more requests for this image that gets neither resolved nor rejected, which starves out any other requests. So once you hit a single 204 request no new imagery would ever load.

How to test this

Paste this code in Sandcastle in master:

var viewer = new Cesium.Viewer('cesiumContainer');

Sandcastle.addToolbarButton('Add bad layer', function() {
    var layer = viewer.imageryLayers.addImageryProvider(new Cesium.UrlTemplateImageryProvider({
        url: "https://tiles.openaerialmap.org/5cba78c0761d2e00050690d9/0/5cba78c0761d2e00050690da/{z}/{x}/{y}"
    }));

    viewer.flyTo(layer);
});

Notice that once you add this layer, and move anywhere else on the globe, the default Bing layer never loads any new tiles (zoom in somewhere, it never loads a higher resolution).

In this branch, it should load fine, and the console should give you errors like An error occurred in "UrlTemplateImageryProvider": Failed to obtain image tile X: 10 Y: 6 Level: 4. which is correct.

Why I refactored the promise chain

The fix is rather obvious, and I'm surprised we didn't catch it earlier. Below is what the createImage in master looks like. Notice the if (!defined(blob)) returning nothing, and not rejecting or resolving.

Resource.supportsImageBitmapOptions()
            .then(function(supportsImageBitmap) {
                // We can only use ImageBitmap if we can flip on decode.
                // See: https://github.com/AnalyticalGraphicsInc/cesium/pull/7579#issuecomment-466146898
                if (!(supportsImageBitmap && preferImageBitmap)) {
                    loadImageElement(url, crossOrigin, deferred);
                    return;
                }

                return Resource.fetchBlob({
                    url: url
                });
            })
            .then(function(blob) {
                if (!defined(blob)) {
                    return;
                }

                return Resource.createImageBitmapFromBlob(blob, {
                    flipY: flipY,
                    premultiplyAlpha: false
                });
            })
            .then(function(imageBitmap) {
                if (!defined(imageBitmap)) {
                    return;
                }

                deferred.resolve(imageBitmap);
            })
            .otherwise(deferred.reject);

The solution is simple right? Just throw a deferred.reject in that block. But this is incorrect. This promise chain is deceiving because you have to think about the two code-paths it's handling simultaneously when reasoning about it. There are 2 situations that cause blob to be undefined:

• You fetched something that has no content,
• Or you are NOT using ImageBitmap at all.

If preferImageBitmap is false or if it's just not supported, it loads the image using loadImageElement, which handles the promise resolving/rejecting, so the rest of the promise chain must not resolve/reject.

We decided to use this pass-through style of chain to be more consistent with the rest of the code-base and it avoids nesting promises which are generally hard to follow - but in this case I think it causes more confusion. The real issue here I think is the mixing of the deferred pattern (since the old way of loading images isn't a promise) with ImageBitmap's API which is an actual promise.

I think what I have in this PR makes it more obvious what's happening, and would help avoid mistakes like this (in this case, even after I figured out the issue, I still solved it incorrectly and this was only caught because of the test I wrote).

[cesium-concierge]

Thanks for the pull request @OmarShehata!

• ✔️ 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.

[OmarShehata]

@mramato I think you're most familiar with this code - would you like to review? Here is our previous conversation about this promise chain structuring: #7579 (comment) where you warned this type of issue might happen. Let me know what you think of my suggestion above.

This is what the fix would look to keep the old promise chain style:

.then(function(blob) {
    if (!defined(blob)) {
        // We only reject if the blob is undefined due to empty content. It's expected for the blob
        // to be undefined if we're not using ImageBitmap or if it's not supported.
    	if (supportsImageBitmap && preferImageBitmap) {
    	  deferred.reject("Failed to load blob");
    	}
        return;
    }

    // ....
})

[mramato]

Thanks!