RFR: 8342782: AWTEventMulticaster throws StackOverflowError using AquaButtonUI

[Jeremy]

On Thu, 7 Nov 2024 21:52:20 GMT, Phil Race <prr at openjdk.org> wrote:

>> The AWTEventMulticaster is a tree node with 2 children. This PR proposes rebalancing that tree after 500 additions to avoid potential StackOverflowErrors when trying to interact with a large AWTEventMulticaster.
>> 
>> In the original headful test:
>> We added 8,000 checkboxes, and when their parent panel was hidden the stack needed to grow to 24,000 lines. It took 8,000 lines to recursively call `java.awt.AWTEventMulticaster.componentHidden`, and then 16,000 additional lines to call two recursive methods to remove a listener:
>> 
>> java.desktop/java.awt.AWTEventMulticaster.removeInternal()
>> java.desktop/java.awt.AWTEventMulticaster.remove()
>> 
>> 
>> With this current PR the max stack size reaches 1,267 instead. (If we rebalanced at EVERY addition, then that same scenario would reach a max stack size of 71.)
>> 
>> JDK-8342782 included a headful test case, but I think the main problem it demonstrated can be represented by the headless test case attached to this PR.
>> 
>> Depending on how this PR is received I may submit a separate ticket & PR to modify AquaButtonUI so it doesn't always attach an AncestorListener. (That is: if my GUI includes 8,000 checkboxes then I don't need 8,000 AncestorListeners.) But JDK-8342782's test case is currently written in a way that should reproduce across all L&F's, so that can be discussed separately.
>
> This seems like a good idea. I will run this through our tests but I'd be quite surprised if except for your new test, there's anything that exercises the new code  or is changed by it.
> 
> I did ponder a bit about whether what you are doing invalidates the comment that "AWTEventMulticaster is immutable." 
> But you aren't mutating them .. just creating new ones.

@prrace wrote:
> But you aren't mutating them .. just creating new ones.

Right. And to keep things simple: the new methods here are private and static. It is possible people have subclassed AWTEventMulticaster over the years, but any subclasses should not be impacted by this change.

@mrserb wrote:
> This bug and its test could be related: https://bugs.openjdk.org/browse/JDK-8165863

Great catch, thanks. I agree they look suspiciously similar, but I couldn't reproduce that one on my Mac using OpenJDK 23. I even increased the `10001` constant to `20001` and the test still passed.

I increased it to `30001` and the test failed with a StackOverflowError. This PR *does* resolve that StackOverflowError, but that ticket is focused on performance.

The headful test originally attached to 8342782 does include some really terrible performance if you use a mouse scrollwheel over the JScrollPane. I want to write that up as a separate ticket, though, because that performance problem persists even with this PR. I think all of these problems ultimately are rooted in AquaButtonUI adding an AncestorListener to *every button* just to help highlight the JRootPane's default button.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/21962#issuecomment-2464096372