Skip to content

core: Fix "too much recursion" with arcgis flex sdk (close #22860)#23355

Open
falpi wants to merge 1 commit intoruffle-rs:masterfrom
falpi:fix-arcgis-flex-sdk-too-much-recursion
Open

core: Fix "too much recursion" with arcgis flex sdk (close #22860)#23355
falpi wants to merge 1 commit intoruffle-rs:masterfrom
falpi:fix-arcgis-flex-sdk-too-much-recursion

Conversation

@falpi
Copy link
Copy Markdown

@falpi falpi commented Mar 30, 2026

A solution proposed by Claude Code for the problem described in issue #22860

@danielhjacobs danielhjacobs added A-core Area: Core player, where no other category fits T-fix Type: Bug fix (in something that's supposed to work already) llm The PR contains mostly LLM-generated code labels Mar 30, 2026
/// Dispatch the `removed` event on a child and log any errors encountered
/// whilst doing so.
///
/// This function is protected against re-entrant calls: if an AS3 event listener
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How does the recursion happen when the event is dispatched after removing the child? There's 3 call sites: AVM1 button, AVM2 button, and MC. Looks like in all of them the child is removed first, and then the event is being dispatched.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hi, I asked Claude to do an analysis of the problem and the solution in a PDF that I attached to the issue here:
https://github.com/user-attachments/files/26325924/claude_bugfix_report.pdf

I bring you the highlights of the PDF:

3.1 How display-list removal works in Flash
When ActionScript 3 removes a child from a display container, the Flash Player runtime fires two events on
the removed object:
• removed — bubbles up the display tree (dispatched while the child is still technically attached, so the
listener can inspect its parent).
• removedFromStage — does not bubble; dispatched on the child and every one of its descendants.
Adobe Flash Player dispatches these events synchronously but protects against reentrancy: if a listener
attempts to remove the same child again while its events are being dispatched, Flash silently ignores the
second request. This guard was never implemented in Ruffle."

3.3 Why ArcGIS Flex triggers the loop
During a zoom operation the ArcGIS Flex SDK rebuilds the tile layer: it removes all existing tiles
DisplayObjects and adds fresh ones for the new extent. The tile container listens for removed events on its
children to perform housekeeping (clearing bitmaps, canceling pending HTTP requests, etc.). That listener
itself calls removeChild on sibling tiles — perfectly legal in Flash, because Flash silently skips the second
removal for any child already being removed. Without the guard, Ruffle dispatches the event, the listener
runs, Ruffle dispatches the event again, ad infinitum

@kjarosh
Copy link
Copy Markdown
Member

kjarosh commented Mar 30, 2026

We'll need a test for that, because (1) it's not obvious how the recursion happens, and (2) without the test this can regress.

@kjarosh kjarosh changed the title web: Fix "too much recursion" with arcgis flex sdk (close #22860) core: Fix "too much recursion" with arcgis flex sdk (close #22860) Mar 31, 2026
@kjarosh kjarosh added squash-on-merge Marks a PR to be squashed before merging. waiting-on-author Waiting on the PR author to make the requested changes labels Mar 31, 2026
@falpi falpi force-pushed the fix-arcgis-flex-sdk-too-much-recursion branch from f2617b4 to 307057e Compare April 6, 2026 17:45
@falpi
Copy link
Copy Markdown
Author

falpi commented Apr 13, 2026

Hi @kjarosh, sorry for my ignorance, but I don't understand what stage the PR is at. I saw you tagged it with "waiting-on-author." What does that mean? Do I have to do something?

@kjarosh
Copy link
Copy Markdown
Member

kjarosh commented Apr 13, 2026

Hi, no worries. So there are few things:

  1. As mentioned in core: Fix "too much recursion" with arcgis flex sdk (close #22860) #23355 (comment), we need a test for that.
  2. The question from core: Fix "too much recursion" with arcgis flex sdk (close #22860) #23355 (comment) hasn't really been answered, you just pasted an LLM-generated analysis.
  3. It looks like the submission is fully LLM-based, and we generally don't accept contributions without a human in the loop. While it's fine to use LLM to produce the code, we at least require a human to take responsibility: to understand the contribution and approve it.

So these 3 points above need to be solved to continue with the review. Without them, it's fine for the PR to be posted and to wait for someone else (either a maintainer or another human contributor) to take it over (it happens regularly).

@falpi
Copy link
Copy Markdown
Author

falpi commented Apr 13, 2026

Hi, no worries. So there are few things:

1. As mentioned in [core: Fix "too much recursion" with arcgis flex sdk (close #22860) #23355 (comment)](https://github.com/ruffle-rs/ruffle/pull/23355#issuecomment-4158798300), we need a test for that.

2. The question from [core: Fix "too much recursion" with arcgis flex sdk (close #22860) #23355 (comment)](https://github.com/ruffle-rs/ruffle/pull/23355#discussion_r3012679331) hasn't really been answered, you just pasted an LLM-generated analysis.

3. It looks like the submission is fully LLM-based, and we generally don't accept contributions without a human in the loop. While it's fine to use LLM to produce the code, we at least require a human to take responsibility: to understand the contribution and approve it.

So these 3 points above need to be solved to continue with the review. Without them, it's fine for the PR to be posted and to wait for someone else (either a maintainer or another human contributor) to take it over (it happens regularly).

Okay, I understand your caution in accepting LLM-centric contributions. In this case, the proposed changes are very simple (just a few lines of code), but the effect is invaluable, as it re-enables the ArcGIS SDK. So, I hope someone can quickly validate the PR. Regards

@falpi falpi force-pushed the fix-arcgis-flex-sdk-too-much-recursion branch from 307057e to f88bbed Compare April 19, 2026 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-core Area: Core player, where no other category fits llm The PR contains mostly LLM-generated code squash-on-merge Marks a PR to be squashed before merging. T-fix Type: Bug fix (in something that's supposed to work already) waiting-on-author Waiting on the PR author to make the requested changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants