[webview_flutter_android] Fix bugs in Java Implementation of the Pigeon API

[bparrishMines]

Split of #4455

The pigeon implementation and current implementation has potential leaking callbacks when a WebView is disposed. This is a possible cause of flaky tests for WebView, so this PR attempts to prevent callbacks when the corresponding WebView object is disposed. In #4455, the integration tests now seem to pass consistently. Along with fixing the resize test: #4460

This also fixes some trivial bugs found though the integration and widget tests in #4455.

Dart Portion: #4461

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the relevant style guides and ran the auto-formatter. (Note that unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
• I signed the CLA.
• The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
• I listed at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
• I updated CHANGELOG.md to add a description of the change.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[stuartmorgan-g]

I think this would benefit from a comment explaining what it's for, so people don't have to go read the implementation to understand it.

[stuartmorgan-g]

Same.

[stuartmorgan-g]

This should have a summary fragment

[stuartmorgan-g]

Does Java have the ability to do subclass-restricted templates? You have this repeated pattern; it would be nice if you could instead have a simple smart pointer class that can only be created for things that inherit from Releasable (but is still typed with the correct specific type), that would handle the call-release-if-necessary-then-assign logic internally.

[bparrishMines]

I wasn't sure what you meant by this. But, my assumption is that you were thinking something like this

private static class ReleasableChild<T> {
    @Nullable private T child;

    private ReleasableChild(@Nullable T child) {
      this.child = child;
    }

    private void release() {
      if (child instanceof Releasable) {
        ((Releasable) child).release();
      }
      child = null;
    }
  }

I replaced all the variables with this value.

[stuartmorgan-g]

(It looks like the Java name for what I was looking for support for is "bounded types".)

This still relies on release being called manually on every assignment, which is error-prone. I was looking for something like (probably doesn't compile as written since it's freehand):

  private static class ReleasableValue<T extends Releasable> {
    @Nullable private T value;

    ReleasableValue(@Nullable T value) {
      this.value = value;
    }

    void set(@Nullable T newValue) {
      release();
      value = newValue;
    }

    @Nullable T get() {
      return value;
    }

    void release() {
      if (value != null) {
        value.release();
      }
      value = null;
    }

    void finalize() {
      release();
    }
  }

[bparrishMines]

ah ok that makes sense. I changed it to:

private static class ReleasableValue<T> {
    @Nullable private T value;

    ReleasableValue() {}

    ReleasableValue(@Nullable T value) {
      this.value = value;
    }

    void set(@Nullable T newValue) {
      release();
      value = newValue;
    }

    @Nullable
    T get() {
      return value;
    }

    void release() {
      if (value instanceof Releasable) {
        ((Releasable) value).release();
      }
      value = null;
    }
  }

I removed finalize since it was only calling release. I also changed ReleasableValue<T extends Releasable> to ReleasableValue<T> because the WebView.setWebViewClient will take either a WebViewClientImpl or WebViewClientCompatImpl and their only shared interface is WebViewClient; which doesn't extend Releasable.

[bparrishMines]

But I suppose I could just cast WebViewClient to Releasable as I set it. Maybe that is a better solution?

[stuartmorgan-g]

Why isn't this using bounded types?

[stuartmorgan-g]

(Which would avoid the need for the type check in release, in addition to making sure incorrect use of ReleasableValue wouldn't compile.)

[stuartmorgan-g]

If this replaces a value, the old values won't get release() called until the next GC cycle; is that okay, or does any previous value need to be explicitly found and released here?

[bparrishMines]

Good catch. Release actually needs to be called because the Dart Object would still need to be disposed.

[stuartmorgan-g]

This is why I think ReleasableValue should have a finalize that calls release.

[bparrishMines]

I started moving away from finalize because I remembered that it is deprecated in Java 9+: https://docs.oracle.com/javase/9/docs/api/java/lang/Object.html#finalize--. And we are already using Java 8.

[stuartmorgan-g]

LGTM modulo the two comments above.