Thanks for the clarification Bo. 
> Can follow CVC's pattern, and make native hold a weak reference to java WebContentsImpl and null check before ever native->java jni call.

This seems like a way to go. Assigning it to myself since this needs addressing before migrating more stuff to WebContentsImpl. 

A couple of issues I ran into (from https://crrev.com/c/590630):

- Java WebContentsImpl (WCI) is created by the native WebContentsAndroid which holds a strong ref to it. If this is turned into a weak ref to avoid gc root issue mentioned above, there will be a period of time when no one is owning WCI (right after it is created, till a certain Java obj like CVC owns it). WCI can be gc'ed anytime in that period. This actually happens and breaks many tests.

Feedback: If we want to make native WebContentsAndroid only hold a weak ref, then the only option is to create the java object first, and then have the java object create the native object. That way something always holds onto the java object. Might be quite a bit of refactoring though.

- WebContentsObserverProxy.destroy() expects its observer list to be empty after calling all the observers' |destroy()| since each observer.destroy() removes itself from the list. This doesn't hold true anymore. It is possible that WCI is already gone by the time proxy.destroy() is called, which leads each observer to not being able to get WCI instance used to remove itself from the observer list (observer also only has a weak ref to WCI).

Feedback: The strong ref from native proxy to java is kind of a problem too, and should never have been introduced in the first place. One way is just to fold proxy into web_contents_android, which seem to make most sense.


Any other input that helps move the refactoring forward would be welcome.

So converting everything to create java WebContents first is probably super complicated, and I'm not even sure entirely possible, given some of them are probably in shared native code.

Maybe it's easier to make the weakref in java. I'm imagining some api like:
class WebContents {
  void setDataHolder(Callable<Map<...>> holder)
}

Java WebContentsImpl then has to store all of its member variables in a map (or SparseArray). When setDataHolder is called, all members can be transferred to the passed in Map. Only webview needs to call setDataHolder, and ensure the map returned is only held weakly. But still going to need null checks in WebContentsImpl though

I got a couple of questions wrt the setDataHolder suggested in #c3

1) What is Callable for? 
2) Is the map used here for any kinds of data WebView is supposed to inject into WebContents in general, not specifically for Javascript API?

> 1) What is Callable for? 

well, WebContents can't hold directly onto the thing that's then holding onto everything else... and if we only want the WeakReference to be in webview code, then WebContents will need to hold onto something else.

> 2) Is the map used here for any kinds of data WebView is supposed to inject into WebContents in general, not specifically for Javascript API?

I'm thinking of the map as a kind of SupportsUserData equivalent. Doesn't have to be map exactly, and data inside should be opaque to the SupportsUserData itself.

Anyway, it's just an idea in my head. I don't really have a good idea how practical it is.

There is a rough design for this in https://chromium-review.googlesource.com/c/chromium/src/+/590630

Look at AwContents, WebContents, WebContentsImpl, WebContentsInternals

High level summary:
* WebContentsInternals is an opaque object to content embedder that holds what would otherwise be member variables of WebContents
* There's a Holder object used to hold Internals, and can be overridden by embedder
* the AwContents overrides the Holder object to use WeakReference
* null checks still required in java object

benefits:
* WeakReference is in webview-only code
* any issues will causejava exceptions, which should be easier to hunt down (?)

downside:
* oh so complicated
* naming... got suggestions for better names?

Landed r507582.