Fix memory leak in SimplePaletteItem

[barreeeiroo]

For some reason, App Inventor was creating 2 instances of components every time a component was being dragged. However, the extra instance was not being deleted or cleaned up, losing access to the instance as no pointer was ever referring to it again.
Tracing down this variable's usage, it seems to be restricted to this file, and no further usage of the "extra" component is being made. This code has been in AI2 since 2011, without major updates besides 2018 and 2023 being the most recent updates.

The current flow was:

• createMockComponent was invoked when the user started dragging the component. The created component resets its reference, hence SimplePaletteItem thinks it was not yet initialized.
• Any further event, like the component entering the mock area or just being dropped, causes isVisibleComponent to be triggered, and it creates another instance. This new instance is now preserved.
• This new instance is the one being triggered through updates, and the one being deleted.

See below the debug logs:

Two createMockComponentFromPalette events are triggered for the components, but just a single one triggers its delete method. One of the copies is left in memory somewhere, which would cause "long development sessions" to become laggier as those components are not cleaned up.

I'm just raising the PR for tracking purposes, as I would like to discuss it at Wednesday's meeting. It is very old code, so probably some context is needed here to understand the rationale for this null-pointer logic resetting it.

[barreeeiroo]

I know this code does not 100% work (it stops creating some new instances), but just sharing it as something's off with the actual component instantiation. 🙂
Every time a component gets dragged and dropped, two instances are created but just one fires its delete.

[ewpatton]

Thanks Diego. I'm not sure why the second instance isn't being GCed but if this resolves the memory pressure that's definitely helpful.  Sent from my iPhoneOn Jul 15, 2024, at 15:47, Diego Barreiro Perez ***@***.***> wrote: @barreeeiroo commented on this pull request. In appinventor/appengine/src/com/google/appinventor/client/editor/simple/palette/SimplePaletteItem.java:

@@ -127,7 +127,7 @@ public MockComponent createMockComponent() {

cacheInternalComponentPrototype(); MockComponent returnedComponentPrototype = componentPrototype; - componentPrototype = null; + // componentPrototype = null; I know this code does not 100% work (it stops creating some new instances), but just sharing it as something's off with the actual component instantiation. 🙂 Every time a component gets dragged and dropped, just two instances are created but just one fires its delete. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[barreeeiroo]

I also made a Memory Heap Profile from Chrome, to validate whether the objects are being retained, and indeed they are.

Took 3 different snapshots:

1. First one as baseline
2. Second one after dragging and dropping a Button component
3. Third one after deleting the created Button component

Between 1 and 2, you can see there is a +2 delta of MockButton instances:

Between 2 and 3, there is a -1 delta of MockButton, which I guess causes the extra one of not being cleaned up:

I did also repeat the same test with the same instance, just to maybe assume that a new "extra" one was not being created if I dropped "again" the Button1, but it was being created again. At 3rd snapshot I had +1 MockButton instances compared to baseline, and after retrying the test I had +1 MockButton compared to 3rd snapshot.

---

However, it seems to not be a global issue, but related to some specific components. I did the same test with LinearProgress and it also fired 2 construct events and only 1 delete event.
But in the profiler, it just created a single instance between snapshots 1 and 2, so it seems like the extra one was automatically cleaned up.

Could it be an issue with MockButtonBase which does not properly handle this?

[shreyashsaitwal]

Could it be an issue with MockButtonBase which does not properly handle this?

In MockButtonBase, there's ClonedButton which is used to calculate the preferred size of the button's mock whenever one of the button's properties changes. Could this be related?

[barreeeiroo]

In MockButtonBase, there's ClonedButton which is used to calculate the preferred size of the button's mock whenever one of the button's properties changes. Could this be related?

Could be. If that creates a reference somewhere, the GC may think that it is still being used so it doesn't get cleaned up.

---

I was investigating a couple of things, and despite some components are in fact being cleaned up by the GC, they don't trigger the delete method. This can break the component lifecycle as the intended delete method is not being invoked. Maybe it's not an issue for now, but just sharing for awareness.