1665009 - MOZ_CRASH(index out of bounds) in update_texture_crash on memory pressure event if gfx.webrender.debug.texture-cache is enabled

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Jamie Nicol [:jnicol]]

When firefox for android is minimized, we receive a memory pressure event, which calls TextureCache::clear_all(). This calls TextureCache::free() for each entry, which results in pending_updates.push_debug_clear() if the texture cache debug view is enabled.

At the end of clear_all(), we then call PictureTextures::clear(), which results in a pending_updates.push_reset().

Later, in Renderer::update_texture_cache() we process the allocations, and the picture cache texture is reset (with fewer layers than before). Then we process the updates, and we attempt to debug_clear the specified layers. But because the texture was reset, this will reference non-existent layers. So when we go texture.fbos[layer] to attempt to bind the FBO to perform the clear, we crash because the index is out of bounds.

Comment 1

[Jamie Nicol [:jnicol]]

This is easy to reproduce on android, because we get a memory pressure event as soon as the app is minimized. But it should affect any platform which gives us memory pressure events. Also, I've only noticed this for the PictureTextures and debug_clears. But in clear_all() we also push_free() for the SharedTextures TextureArrays. So if we tried to do an upload on the same frame as a memory pressure, I think we'd also crash (by not being able to find the texture in the texture_cache_map. So this might be an issue even without the texture cache debug enabled. Glenn, Dzmitry, do you know if that is indeed a potential issue, or do we have a defence mechanism already against that?

To fix this, can we just clear the pending updates for a given texture during push_reset and push_free?

Comment 2

[Glenn Watson [:gw]]

Sounds like there might be something a bit more subtle going on here, if I understand the comments above.

The code in push_free should already be removing any other pending updates for the specified texture [1].

Similarly, in push_reset it coalesces any matching pending texture operations and removes them [2].

It sounds like maybe the logic in those methods might need to be extended to also handle the pending debug clears array or something like that?

[1] https://searchfox.org/mozilla-central/source/gfx/wr/webrender/src/internal_types.rs#490
[2] https://searchfox.org/mozilla-central/source/gfx/wr/webrender/src/internal_types.rs#465

Comment 3

[Jamie Nicol [:jnicol]]

[1] certainly seems like it would prevent this bug, but I'm not sure if [2] is relevant to this. So it seems like this can only occur in push_reset() rather than push_free(), which is good as it means it can't occur unless the debug view is enabled.

It sounds like maybe the logic in those methods might need to be extended to also handle the pending debug clears array or something like that?

The debug clears use the same updates array as regular texture uploads [3], so I think we just need to add a call to self.updates.remove(&id) to push_reset() to match the one that is already in push_free().

[3] https://searchfox.org/mozilla-central/rev/30e70f2fe80c97bfbfcd975e68538cefd7f58b2a/gfx/wr/webrender/src/internal_types.rs#417

Comment 4

[Jamie Nicol [:jnicol]]

Attachment: Bug 1665009 - Don't attempt any texture cache updates for reset textures. r?gw

When a texture cache texture is freed, we remove the list of pending updates for
that texture so that the renderer does not attempt to upload to it. We should do
the same for reset textures too. Not doing so was causing us to crash sometimes
when the android app was minimised (leading to a memory pressure event, and the
picture cache texture being reset) whilst the gfx.webrender.debug.texture-cache
was enabled (which caused us to issue debug clear updates).

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/45d706665f0d
Don't attempt any texture cache updates for reset textures. r=gw

Comment 6

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/45d706665f0d