Fix remaining memory leaks on Android

[johnzhou721]

Fixes all remaining memory leaks by way of weakref.proxy. Manual checks are added to catch ReferenceError just to be safe.

Refs #2849.

PR Checklist:

• All new features have been tested
• All new features have been documented
• I have read the CONTRIBUTING.md file
• I will abide by the code of conduct

[freakboy3742]

Conceptually I think this makes sense; I'd like @mhsmith to take a look in case I'm missing something obvious from the Android side of things. He's on leave at the moment as well, so this will likely need to wait until he returns.

[johnzhou721]

FYI for @mhsmith and @freakboy3742: Even after changes after the last review by @freakboy3742 this is still ready for review, I just consistentialized a pattenr after #3973 landed/

[johnzhou721]

@mhsmith Could this be taken a look at? Thank you and sorry for the message -- just want to make sure it's not forgotten since memory leaks are kind of serious.

[mhsmith]

This looks good to me, but there's a lot of duplicate code in the explanatory comments, the pragmas and the default return values. This could be factored out into a context manager based on contextlib.suppress. For example, instead of this:

        def onNavigationItemSelected(self, item):
            try:
                for index, option in enumerate(self.impl.options):
                    if option.menu_item == item:
                        self.impl.set_current_tab_index(index, programmatic=False)
                        return True

                # You shouldn't be able to select an item that isn't isn't selectable.
                return False  # pragma: no cover
            # This is a defensive safety catch, just in case if the impl object
            # has already been collected, but the native widget is still
            # emitting an event to the listener.
            except ReferenceError:  # pragma: no cover
                return False

We could have this:

        from ... import suppressReferenceError

        def onNavigationItemSelected(self, item):
            with suppressReferenceError():
                for index, option in enumerate(self.impl.options):
                    if option.menu_item == item:
                        self.impl.set_current_tab_index(index, programmatic=False)
                        return True

            # You shouldn't be able to select an item that isn't isn't selectable.
            return False  # pragma: no cover

The explanatory comment and the pragma now only need to exist at the definition of suppressReferenceError.

[johnzhou721]

@mhsmith

Not quite sure about this... my rationale behind not context-managing was so that it's easy to find the comment explaining why this is needed. However, since you requested this, I will make the change. Should I add "see comment at suppressReferenceError" before the use of suppressReferenceError? Thank you.

[mhsmith]

No, the point of defining a function rather than using contextlib.suppress directly is that it gives us a single place to hold the comment, and it's obvious enough to look there if you don't know what the suppression is for.

[johnzhou721]

@mhsmith Ready for review. My attempts to rerun CI only led to more intermittent fials.

[HalfWhitt]

My attempts to rerun CI only led to more intermittent fials.

When a test or two is failing and it seems likely to be an intermittent CI issue, we can rerun just the one that failed (or just ignore it). There's no need to push an update just to rerun the whole suite.

[mhsmith]

I think you need to be a project member to rerun CI. But if you're confident that the failure has nothing to do with your changes, then you can just post a comment saying so, and the reviewer will rerun the tests if necessary.

[HalfWhitt]

Correct — I could have been clearer than just "we".

[HalfWhitt]

The iOS error is the same one I also got last night — twice in a row — on #4057. It only happened after I merged main, but I'm not sure if that's the cause, or a coincidence with a change in the GitHub testing environment.

I've tried running it twice today (on that PR), and both times it's timed out. So I suspect it's something timing-related, but I'm not sure. I imagine it's probably also related to #4043 — although, of course, it's been passing since that was merged, before yesterday.

[johnzhou721]

I think you need to be a project member to rerun CI. But if you're confident that the failure has nothing to do with your changes, then you can just post a comment saying so, and the reviewer will rerun the tests if necessary.

In this case, I was not quite confident... since coincidentally, it was an Android CI fail. So I'm sorry if I wasted resources.

[johnzhou721]

@HalfWhitt It may be slow machines... we'd need to move the navigation tests to a timeout approach rather than a hard delay. It's a bit down my todo list so far, though.

[HalfWhitt]

@HalfWhitt It may be slow machines... we'd need to move the navigation tests to a timeout approach rather than a hard delay. It's a bit down my todo list so far, though.

Aaaaand rerunning it again just now on this PR times out. So yes, GitHub's iOS runners are definitely having trouble — and it may indeed be worth adding a timeout to those tests. But in any event, it's not tied to either this PR or #4057.

@mhsmith, are the test failures the reason you waited to merge?

[mhsmith]

I see you've created #4103, so let's continue the discussion there, or in a separate issue if you think it's an independent problem.

[mhsmith]

In this case, I was not quite confident... since coincidentally, it was an Android CI fail.

If you mean this run, that looks like the IndexOutOfBoundsException from #3685, so it's not related to this PR.