Fix unhandled framebuffer error when tab wakes from sleep#7303
Fix unhandled framebuffer error when tab wakes from sleep#7303johanrd wants to merge 7 commits intomaplibre:mainfrom
Conversation
…dded a `checkFramebufferStatus()` method that callers use after attachments are set (at which point `ColorAttachment.set` has already bound the correct FBO). Added the call at all 5 existing callsites. Added try/catch to `redraw()` matching the existing pattern in `triggerRepaint()`.
c97e047 to
40f7ff2
Compare
3128c87 to
c725d1d
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7303 +/- ##
==========================================
- Coverage 92.71% 92.71% -0.01%
==========================================
Files 288 288
Lines 24042 24053 +11
Branches 5093 5094 +1
==========================================
+ Hits 22291 22301 +10
- Misses 1751 1752 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
| } | ||
|
|
||
| checkFramebufferStatus() { |
There was a problem hiding this comment.
This isn't really checking status, does it?
There was a problem hiding this comment.
@HarelM Good catch — I think this name is papering over a deeper question about whether the check should exist at all. Some history:
It started as a debug-only assert:
maplibre-gl-js/src/gl/framebuffer.js
Line 26 in 00d7026
#1485 turned it into a production throw, and you flagged exactly this risk on that PR:
The only main "issue" I see work this approach is that there is a functionally change in this commit: before this code simply didn't exist in production so if there was an assert that might be triggered but didn't cause any actual error the user wouldn't know as opposed to now where there might be "throws" that cause actual problems.
That seems to reflect what's showing up in Sentry now.
Then #5266 tightened the
Line 3384 in e6d6923
Zooming out
Given all that, I'd suggest deleting ensureFramebufferComplete() and all 5 call sites instead of renaming it. After that, isFramebufferNotCompleteError and the redraw() try/catch guard an error that can no longer be thrown from our code, and can go too.
The alternative is developing an active recovery path (force context loss/restore on failure), but honestly WebGL may handle a transient framebuffer blip fine on its own if we just get out of the way. Happy to restructure the PR either direction.
There was a problem hiding this comment.
Given the history you dug, I trust you understand this better than me, so if you think a direction with worth pursuing, feel free to do so.
I would like to get @birkskyum and @ToHold 's input here since @birkskyum recently fixed a long lasting bug with framebuffer misalignment and @ToHold did a lot of work around recovery from context loss.
There was a problem hiding this comment.
CC: @mvanhorn who's also added some PRs around context restored.
Is there a sudden interest in making this work well which wasn't a big motivation before?
I saw this error in Sentry from a production app:
14:21:04— user navigates to a page with map, map loads with satellite tiles + heatmap14:46:38— last user interaction07:07:49— next morning, tile fetches resume (tab wakes up after ~16 hours)07:38:34—Error: Framebuffer is not completefires, unhandledTwo possible root causes:
Framebufferconstructor checks the wrong FBO.gl.createFramebuffer()doesn't bind the new FBO, socheckFramebufferStatus(gl.FRAMEBUFFER)validates whatever was previously bound. After a long sleep the previously-bound FBO can be stale, causing a false-positive throw.redraw()has no error handling.triggerRepaint()already catches framebuffer errors, butredraw()— the resize observer path — does not. The error reaches the global handler.Design decisions
Silent swallowing vs error event. This PR matches
triggerRepaint()'s existing behavior: catch and discard. For transient errors (tab sleep) this is fine — the map shows the previous good frame and recovers on the next interaction. The downside is that consumers have no signal when a frame fails, so persistent errors would show stale content silently. An alternative would be tothis.fire(new ErrorEvent(error))before swallowing — happy to add this if preferred, and it could be applied totriggerRepaint()too.Explicit
checkFramebufferStatus()calls. The constructor check was moved to an explicit method that binds its own FBO before checking. Called at all 5 callsites after attachments are set. Automating this (e.g. insideColorAttachment.set()) would be a refactor beyond scope.Test plan
Framebufferconstructor with stale bound FBOcheckFramebufferStatus()on incomplete FBOredraw()with framebuffer errorChecklist
Include before/after visuals or gifs if this PR includes visual changes.— N/A, no visual changesDocument any changes to public APIs.— N/A, no public API changesPost benchmark scores.— N/ACHANGELOG.mdunder the## mainsection.Cowritten by claude