Better error reporting (#5266)

* Don't suppress WebGL errors * Only supress abort error * Fix some tests * Mock terrain * Mock framebuffer * Use framebuffer error instead of mock * Fix more errors * Make all unit tests pass * Fix more tests * Add tests * Linting * Reenables test * Build size fix * Add changelog entry * Remove target try/catch * Address feedback * Undo sourcemap increase * Fix typo
maplibre · Dec 26, 2024 · d32542a · d32542a
1 parent e6d6923
commit d32542a
Show file tree

Hide file tree

Showing 15 changed files with 141 additions and 66 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,6 +11,8 @@
 - Fix center being incorrectly constrained when using globe ([#5186](https://github.com/maplibre/maplibre-gl-js/pull/5186))
 - Fix atmosphere improperly blending into the background ([#5235](https://github.com/maplibre/maplibre-gl-js/pull/5235))
 - Fix parsing wrong hash location ([#5131](https://github.com/maplibre/maplibre-gl-js/pull/5131))
+- Fix swallowing of errors ([#4532](https://github.com/maplibre/maplibre-gl-js/issues/4532))
+- Fix erroring requests not reported on `error` handler ([#4613](https://github.com/maplibre/maplibre-gl-js/issues/4613))
 - _...Add new stuff here..._
 
 ## 5.0.0-pre.10

diff --git a/src/gl/framebuffer.ts b/src/gl/framebuffer.ts
@@ -1,6 +1,7 @@
 import {ColorAttachment, DepthAttachment, DepthStencilAttachment} from './value';
 
 import type {Context} from './context';
+import {createFramebufferNotCompleteError} from '../util/framebuffer_error';
 
 /**
  * @internal
@@ -28,7 +29,7 @@ export class Framebuffer {
             throw new Error('Stencil cannot be set without depth');
         }
         if (gl.checkFramebufferStatus(gl.FRAMEBUFFER) !== gl.FRAMEBUFFER_COMPLETE) {
-            throw new Error('Framebuffer is not complete');
+            throw createFramebufferNotCompleteError();
         }
     }
 

diff --git a/src/render/painter.ts b/src/render/painter.ts
@@ -492,7 +492,7 @@ export class Painter {
         const coordsAscending: {[_: string]: Array<OverscaledTileID>} = {};
         const coordsDescending: {[_: string]: Array<OverscaledTileID>} = {};
         const coordsDescendingSymbol: {[_: string]: Array<OverscaledTileID>} = {};
-        const renderOptions: RenderOptions = {isRenderingToTexture: false, isRenderingGlobe: style.projection.transitionState > 0};
+        const renderOptions: RenderOptions = {isRenderingToTexture: false, isRenderingGlobe: style.projection?.transitionState > 0};
 
         for (const id in sourceCaches) {
             const sourceCache = sourceCaches[id];
@@ -539,7 +539,7 @@ export class Painter {
         }
 
         // Execute offscreen GPU tasks of the projection manager
-        this.style.projection.updateGPUdependent({
+        this.style.projection?.updateGPUdependent({
             context: this.context,
             useProgram: (name: string) => this.useProgram(name)
         });

diff --git a/src/render/terrain.ts b/src/render/terrain.ts
@@ -378,7 +378,7 @@ export class Terrain {
      * @returns the created regular mesh
      */
     getTerrainMesh(tileId: OverscaledTileID): Mesh {
-        const globeEnabled = this.painter.style.projection.transitionState > 0;
+        const globeEnabled = this.painter.style.projection?.transitionState > 0;
         const northPole = globeEnabled && tileId.canonical.y === 0;
         const southPole = globeEnabled && tileId.canonical.y === (1 << tileId.canonical.z) - 1;
         const key = `m_${northPole ? 'n' : ''}_${southPole ? 's' : ''}`;

diff --git a/src/style/style.ts b/src/style/style.ts
@@ -203,7 +203,7 @@ export class Style extends Evented {
     glyphManager: GlyphManager;
     lineAtlas: LineAtlas;
     light: Light;
-    projection: Projection;
+    projection: Projection | undefined;
     sky: Sky;
 
     _frameRequest: AbortController;

diff --git a/src/ui/map.ts b/src/ui/map.ts
@@ -64,6 +64,8 @@ import {MercatorTransform} from '../geo/projection/mercator_transform';
 import {type ITransform} from '../geo/transform_interface';
 import {type ICameraHelper} from '../geo/projection/camera_helper';
 import {MercatorCameraHelper} from '../geo/projection/mercator_camera_helper';
+import {isAbortError} from '../util/abort_error';
+import {isFramebufferNotCompleteError} from '../util/framebuffer_error';
 
 const version = packageJSON.version;
 
@@ -3182,7 +3184,7 @@ export class Map extends Camera {
     _render(paintStartTimeStamp: number) {
         const fadeDuration = this._idleTriggered ? this._fadeDuration : 0;
 
-        const isGlobeRendering = this.style.projection.transitionState > 0;
+        const isGlobeRendering = this.style.projection?.transitionState > 0;
 
         // A custom layer may have used the context asynchronously. Mark the state as dirty.
         this.painter.context.setDirty();
@@ -3220,14 +3222,14 @@ export class Map extends Camera {
             this.style.update(parameters);
         }
 
-        const globeRenderingChaged = this.style.projection.transitionState > 0 !== isGlobeRendering;
-        this.style.projection.setErrorQueryLatitudeDegrees(this.transform.center.lat);
-        this.transform.setTransitionState(this.style.projection.transitionState, this.style.projection.latitudeErrorCorrectionRadians);
+        const globeRenderingChanged = this.style.projection?.transitionState > 0 !== isGlobeRendering;
+        this.style.projection?.setErrorQueryLatitudeDegrees(this.transform.center.lat);
+        this.transform.setTransitionState(this.style.projection?.transitionState, this.style.projection?.latitudeErrorCorrectionRadians);
 
         // If we are in _render for any reason other than an in-progress paint
         // transition, update source caches to check for and load any tiles we
         // need for the current transform
-        if (this.style && (this._sourcesDirty || globeRenderingChaged)) {
+        if (this.style && (this._sourcesDirty || globeRenderingChanged)) {
             this._sourcesDirty = false;
             this.style._updateSources(this.transform);
         }
@@ -3246,7 +3248,7 @@ export class Map extends Camera {
             }
         }
 
-        this._placementDirty = this.style && this.style._updatePlacement(this.transform, this.showCollisionBoxes, fadeDuration, this._crossSourceCollisions, globeRenderingChaged);
+        this._placementDirty = this.style && this.style._updatePlacement(this.transform, this.showCollisionBoxes, fadeDuration, this._crossSourceCollisions, globeRenderingChanged);
 
         // Actually draw
         this.painter.render(this.style, {
@@ -3381,7 +3383,11 @@ export class Map extends Camera {
                 PerformanceUtils.frame(paintStartTimeStamp);
                 this._frameRequest = null;
                 this._render(paintStartTimeStamp);
-            }).catch(() => {}); // ignore abort error
+            }).catch((error: Error) => {
+                if (!isAbortError(error) && !isFramebufferNotCompleteError(error)) {
+                    throw error;
+                }
+            });
         }
     }
 

diff --git a/src/ui/map_tests/map_canvas.test.ts b/src/ui/map_tests/map_canvas.test.ts
@@ -93,7 +93,6 @@ describe('WebGLContextAttributes options', () => {
         Object.defineProperty(container, 'clientHeight', {value: 2048});
         const map = createMap({container, canvasContextAttributes});
         const mapContextAttributes = map.painter.context.gl.getContextAttributes();
-        console.log(mapContextAttributes);
         expect(mapContextAttributes.alpha).toBe(true);
         expect(mapContextAttributes.depth).toBe(true);
         expect(mapContextAttributes.stencil).toBe(true);

diff --git a/src/ui/map_tests/map_events.test.ts b/src/ui/map_tests/map_events.test.ts
@@ -1,12 +1,13 @@
 import {describe, beforeEach, test, expect, vi} from 'vitest';
 import simulate from '../../../test/unit/lib/simulate_interaction';
 import {type StyleLayer} from '../../style/style_layer';
-import {createMap, beforeMapTest, createStyle, sleep} from '../../util/test/util';
+import {createMap, beforeMapTest, createStyle, sleep, createTerrain} from '../../util/test/util';
 import {type MapGeoJSONFeature} from '../../util/vectortile_to_geojson';
 import {type MapLayerEventType, type MapLibreEvent} from '../events';
 import {Map, type MapOptions} from '../map';
 import {Event as EventedEvent, ErrorEvent} from '../../util/evented';
 import {GlobeProjection} from '../../geo/projection/globe_projection';
+import {type StyleSpecification} from '@maplibre/maplibre-gl-style-spec';
 
 type IsAny<T> = 0 extends T & 1 ? T : never;
 type NotAny<T> = T extends IsAny<T> ? never : T;
@@ -959,6 +960,19 @@ describe('map events', () => {
         expect(failSpy).not.toHaveBeenCalled();
     });
 
+    test('errors inside load event are not suppressed', async () => {
+        const map = new Map({container: window.document.createElement('div')} as any as MapOptions);
+
+        const loadHandler = vi.fn(() => {
+            throw new Error('Error in load handler');
+        });
+
+        map.on('load', loadHandler);
+        await sleep(1);
+
+        expect(loadHandler).toThrowError();
+    });
+
     test('no idle event during move', async () => {
         const style = createStyle();
         const map = createMap({style, fadeDuration: 0});
@@ -978,10 +992,7 @@ describe('map events', () => {
 
     test('getZoom on moveend is the same as after the map end moving, with terrain on', () => {
         const map = createMap({interactive: true, clickTolerance: 4});
-        map.terrain = {
-            pointCoordinate: () => null,
-            getElevationForLngLatZoom: () => 1000,
-        } as any;
+        map.terrain = createTerrain();
         let actualZoom: number;
         map.on('moveend', () => {
             // this can't use a promise due to race condition
@@ -1049,6 +1060,37 @@ describe('map events', () => {
             map.fire(new ErrorEvent(error));
             expect(spy).not.toHaveBeenCalled();
         });
+
+        test('throws error when request fails', async () => {
+            const style: StyleSpecification = {
+                ...createStyle(),
+                sources: {
+                    'source': {
+                        type: 'vector',
+                        url: 'maplibre://nonexistent'
+                    }
+                },
+                layers: [
+                    {
+                        id: 'layer',
+                        source: 'source',
+                        type: 'fill',
+                        'source-layer': 'test'
+                    }
+                ]
+            };
+            const map = createMap();
+            map.setStyle(style);
+
+            const errorHandler = vi.fn();
+            map.on('error', errorHandler);
+
+            map.triggerRepaint();
+            await sleep(100);
+
+            expect(errorHandler).toHaveBeenCalledTimes(1);
+
+        });
     });
 
     describe('projectiontransition event', () => {

diff --git a/src/ui/map_tests/map_style.test.ts b/src/ui/map_tests/map_style.test.ts
@@ -42,9 +42,9 @@ describe('#setStyle', () => {
             map.on('data', recordEvent);
             map.on('dataloading', recordEvent);
 
-            map.style.fire(new Event('error'));
-            map.style.fire(new Event('data'));
-            map.style.fire(new Event('dataloading'));
+            map.style.fire(new EventedEvent('error'));
+            map.style.fire(new EventedEvent('data'));
+            map.style.fire(new EventedEvent('dataloading'));
 
             expect(events).toEqual([
                 'error',

diff --git a/src/ui/map_tests/map_terrain.test.ts b/src/ui/map_tests/map_terrain.test.ts
@@ -4,13 +4,16 @@ import {LngLat} from '../../geo/lng_lat';
 import {fakeServer, type FakeServer} from 'nise';
 import {type Terrain} from '../../render/terrain';
 import {MercatorTransform} from '../../geo/projection/mercator_transform';
+import {type Map} from '../map';
 
 let server: FakeServer;
+let map: Map;
 
 beforeEach(() => {
     beforeMapTest();
     global.fetch = null;
     server = fakeServer.create();
+    map = createMap();
 });
 
 afterEach(() => {
@@ -27,8 +30,6 @@ describe('#setTerrain', () => {
             bounds: [-47, -7, -45, -5]
         }));
 
-        const map = createMap();
-
         map.on('load', () => {
             map.addSource('terrainrgb', {type: 'raster-dem', url: '/source.json'});
             server.respond();
@@ -53,7 +54,6 @@ describe('#getTerrain', () => {
 
 describe('getCameraTargetElevation', () => {
     test('Elevation is zero without terrain, and matches any given terrain', () => {
-        const map = createMap();
         expect(map.getCameraTargetElevation()).toBe(0);
 
         const terrainStub = {} as Terrain;
@@ -73,8 +73,6 @@ describe('getCameraTargetElevation', () => {
 
 describe('Keep camera outside terrain', () => {
     test('Try to move camera into terrain', () => {
-        const map = createMap();
-
         let terrainElevation = 10;
         const terrainStub = {} as Terrain;
         terrainStub.getElevationForLngLatZoom = vi.fn(

diff --git a/src/ui/map_tests/map_zoom.test.ts b/src/ui/map_tests/map_zoom.test.ts
@@ -1,5 +1,5 @@
 import {beforeEach, test, expect} from 'vitest';
-import {createMap, beforeMapTest} from '../../util/test/util';
+import {createMap, beforeMapTest, createTerrain} from '../../util/test/util';
 import simulate from '../../../test/unit/lib/simulate_interaction';
 
 beforeEach(() => {
@@ -85,10 +85,7 @@ test('recalculate zoom is done on the camera update transform', () => {
         clickTolerance: 4,
         transformCameraUpdate: ({zoom}) => ({zoom: zoom + 0.1})
     });
-    map.terrain = {
-        pointCoordinate: () => null,
-        getElevationForLngLatZoom: () => 1000
-    } as any;
+    map.terrain = createTerrain();
     const canvas = map.getCanvas();
     simulate.dragWithMove(canvas, {x: 100, y: 100}, {x: 100, y: 150});
     map._renderTaskQueue.run();