Skip to content

Fix RecordDrawRectRenderDecorator closing#2864

Open
MatkovIvan wants to merge 2 commits intojb-mainfrom
ivan.matkov/close-bounds-recorder
Open

Fix RecordDrawRectRenderDecorator closing#2864
MatkovIvan wants to merge 2 commits intojb-mainfrom
ivan.matkov/close-bounds-recorder

Conversation

@MatkovIvan
Copy link
Member

CMP-6590 Memory leak: RecordDrawRectRenderDecorator is not closed

Release Notes

Fixes - Desktop

  • Fix memory leak in dialogs with non-default compose.layers.type setting

@MatkovIvan MatkovIvan requested a review from m-sasha March 12, 2026 14:59
Comment on lines +153 to +155
}.also {
drawBoundsRecorder?.close()
drawBoundsRecorder = it
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks strange. We're returning an object but continue owning it, and its lifecycle is the same as the DesktopComposeSceneLayer, unless someone asks for a new one.

Maybe something like this instead?

    protected var targetRenderDelegate: SkikoRenderDelegate? = null
    protected val drawBoundsRecorder = RecordDrawRectRenderDecorator(
        decorated = { canvas, width, height, nanoTime ->
            targetRenderDelegate?.onRender(canvas, width, height, nanoTime)
        }
    ) { canvasBoundsInPx ->
        val currentCanvasOffset = drawBounds.topLeft
        val drawBoundsInWindow = canvasBoundsInPx.roundToIntRect().translate(currentCanvasOffset)
        maxDrawInflate = maxInflate(boundsInWindow, drawBoundsInWindow, maxDrawInflate)
        drawBounds = IntRect(
            left = boundsInWindow.left - maxDrawInflate.left,
            top = boundsInWindow.top - maxDrawInflate.top,
            right = boundsInWindow.right + maxDrawInflate.right,
            bottom = boundsInWindow.bottom + maxDrawInflate.bottom
        )
        onDrawBoundsChanged()
    }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumes that the function is called only once and we're having single targetRenderDelegate. This is kinda true for now, but seems more dangerous than "closing during close".

I'll think about different ways...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made it more generic, ptal

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumes that the function is called only once and we're having single targetRenderDelegate. This is kinda true for now, but seems more dangerous than "closing during close".

I think it's better for the code to reflect the current state rather than some future possible state.

With the latest changes it creates the illusion that recordDrawBounds can be called multiple times to wrap different, concurrently "alive" SkikoRenderDelegates , but this is not actually the case, as it will cause race conditions (the decorator writes to "global" state).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants