Do not present textures in FlutterMetalLayer if the drawable size changed and the texture's size does not match the new drawable size

[jason-simmons]

Also add locks around some methods that access texture and drawable state.

Fixes #175253

[gemini-code-assist]

Code Review

This pull request aims to prevent crashes when presenting textures after a drawable size change by adding a size check. It also enhances thread safety by adding locks around methods that access shared texture and drawable state. While the size check in presentTexture: should prevent the immediate crash, it introduces a subtle but significant issue where stale, incorrectly-sized textures are returned to the texture pool, leading to resource pollution. My review includes a high-severity comment detailing this issue in FlutterMetalLayer.mm and a medium-severity comment on the new test in FlutterMetalLayerTest.mm with a suggestion to modify it to expose this underlying bug.

[gemini-code-assist]

This check correctly prevents presenting a texture with a mismatched size, which should fix the reported crash. However, by simply returning, a new issue is introduced: the stale texture is eventually returned to the available textures pool.

Here's the sequence of events:

1. This method returns early.
2. The FlutterDrawable that holds this texture is not marked as _presented.
3. When the FlutterDrawable is deallocated, its dealloc method calls returnTexture: because _presented is false.
4. returnTexture: adds the stale, incorrectly-sized texture to _availableTextures.

This pollutes the texture pool, leading to memory being held by unusable textures and subsequent calls to nextDrawable potentially receiving these stale textures, which will then be rejected again by this check.

The root cause appears to be in setDrawableSize:, where _totalTextures is reset to 0, causing the layer to lose track of in-flight textures. A complete solution would involve correctly managing _totalTextures across resizes and ensuring stale textures are discarded rather than returned to the pool.

[jason-simmons]

Added a texture size check to returnTexture

[gemini-code-assist]

This test case is a great addition for the resize scenario. However, it doesn't currently expose the subtle issue of stale textures being returned to the pool because the oldSizeDrawable objects are held for the entire duration of the method.

If the old drawables were deallocated before the verification loop, nextDrawable would pick up the stale textures from the pool, and the assertion XCTAssertEqual(drawable.texture.width, newSize) would fail. To make this test more robust and catch this bug, you could scope the lifetime of the old drawables so they are deallocated before the verification loop.

For example:

- (void)testResizeAndPresent {
  FlutterMetalLayer* layer = [self addMetalLayer];
  TestCompositor* compositor = [[TestCompositor alloc] initWithLayer:layer];

  const CGFloat newSize = 200;

  {
    id<CAMetalDrawable> oldSizeDrawable1 = [layer nextDrawable];
    BAIL_IF_NO_DRAWABLE(oldSizeDrawable1);
    id<CAMetalDrawable> oldSizeDrawable2 = [layer nextDrawable];
    BAIL_IF_NO_DRAWABLE(oldSizeDrawable2);

    layer.drawableSize = CGSizeMake(newSize, newSize);

    // After resizing, present the drawables that were allocated using the old size.
    [oldSizeDrawable1 present];
    [compositor commitTransaction];
    [oldSizeDrawable2 present];
    [compositor commitTransaction];
  } // old drawables are deallocated here

  // Verify that textures with the old size are not reused.
  for (int i = 0; i < 4; i++) {
    id<CAMetalDrawable> drawable = [layer nextDrawable];
    [drawable present];
    [compositor commitTransaction];
    XCTAssertEqual(drawable.texture.width, newSize);
  }

  [self removeMetalLayer:layer];
}

With this structure, the test would fail with the current implementation, correctly identifying the texture pool pollution issue.

[chinmaygarde]

Can we add a comment near where the ivars _availableTextures, _totalTextures and _front are declared that access to these must be synchronized?

[jason-simmons]

done

[chinmaygarde]

Nit: nil check the texture as well? Before the at-synchronized.

[jason-simmons]

done

[chinmaygarde]

Some nits. But not presenting or recycling textures of an outdated size makes sense to me. Also, the lack of synchronization seems like an oversight.

Perhaps @knopp can also take another look?