Rename JavaObjectWeakGlobalRef::is_empty to avoid misuse.

Rename JavaObjectWeakGlobalRef::is_empty to avoid misuse.

JavaObjectWeakGlobalRef::is_empty was being misused to attempt to check
if the object referred to has been collected or not (see
crbug.com/661444 for one case; others in this CL). The method is
valuable in some cases that actually just want to check if the weak
reference has been initialised, so rename it to is_uninitialized and add
an explanatory comment to make it more obvious what to do with it.

Fix a couple of existing uses that are incorrect or dubious, and rename
the "okay" uses to call the new function.

BUG=

Committed: https://crrev.com/aef040326a50569a1838d2df24b636a90f8179f1
Cr-Commit-Position: refs/heads/master@{#430417}

[Torne, 2016-11-07 15:14:25]

torne@chromium.org changed reviewers:
+ rmcilroy@chromium.org, sky@chromium.org, timvolodine@chromium.org

[Torne, 2016-11-07 15:14:25]

rmcilroy: general review of the CL
sky: OWNERS for ui
timvolodine: checking that my change to aw_contents_io_thread_client_impl
matches the original intent of the code.

[Torne, 2016-11-07 15:15:31]

torne@chromium.org changed reviewers:
+ boliu@chromium.org

[Torne, 2016-11-07 15:15:32]

Also +boliu who originally commented on this API being weird/bad.

[rmcilroy, 2016-11-07 16:12:14]

base/ LGTM

[timvolodine, 2016-11-07 16:31:20]

On 2016/11/07 15:14:25, Torne wrote:
> rmcilroy: general review of the CL
> sky: OWNERS for ui
> timvolodine: checking that my change to aw_contents_io_thread_client_impl
> matches the original intent of the code.

regarding android_webview/native/aw_contents_io_thread_client_impl.cc:

yes I think they are functionally equivalent, that is if
JNIEnv::NewLocalRef(obj) returns not null when obj is not null. Which it
probably does, so lgtm.

[boliu, 2016-11-07 16:34:39]

lgtm

[Torne, 2016-11-07 16:52:41]

On 2016/11/07 16:31:20, timvolodine wrote:
> On 2016/11/07 15:14:25, Torne wrote:
> > rmcilroy: general review of the CL
> > sky: OWNERS for ui
> > timvolodine: checking that my change to aw_contents_io_thread_client_impl
> > matches the original intent of the code.
> 
> regarding android_webview/native/aw_contents_io_thread_client_impl.cc:
> 
> yes I think they are functionally equivalent, that is if
> JNIEnv::NewLocalRef(obj) returns not null when obj is not null. Which it
> probably does, so lgtm.

They aren't functionally equivalent - the current code seems like it's wrong
since it will fail the DCHECK if the object has been garbage collected, and then
pass a null java object to the constructor. My fixed code returns null from the
function instead in this case. Is that the correct behaviour?

[timvolodine, 2016-11-07 17:13:02]

https://codereview.chromium.org/2485513003/diff/1/android_webview/native/aw_c...
File android_webview/native/aw_contents_io_thread_client_impl.cc (right):

https://codereview.chromium.org/2485513003/diff/1/android_webview/native/aw_c...
android_webview/native/aw_contents_io_thread_client_impl.cc:279: if
(java_delegate.is_null())
to be more precise, I was looking here and comparing
g_sw_instance_.Get().is_empty() with g_sw_instance_.Get().get(env).is_null() it
seemed they were equivalent modulo the comment about JNIEnv::NewLocalRef(obj)

[boliu, 2016-11-07 17:23:33]

https://codereview.chromium.org/2485513003/diff/1/android_webview/native/aw_c...
File android_webview/native/aw_contents_io_thread_client_impl.cc (right):

https://codereview.chromium.org/2485513003/diff/1/android_webview/native/aw_c...
android_webview/native/aw_contents_io_thread_client_impl.cc:279: if
(java_delegate.is_null())
On 2016/11/07 17:13:01, timvolodine wrote:
> to be more precise, I was looking here and comparing
> g_sw_instance_.Get().is_empty() with g_sw_instance_.Get().get(env).is_null()
it
> seemed they were equivalent modulo the comment about JNIEnv::NewLocalRef(obj)

They are not equivalent.

is_empty (now is_initialized) will return true even if the *referent* object has
been garbage collected; it's only checking whether the *reference* is
initialized with something non-null

so it's possible for is_empty/is_initialized to be true and get(env).is_null to
be false

[boliu, 2016-11-07 17:25:29]

https://codereview.chromium.org/2485513003/diff/1/android_webview/native/aw_c...
File android_webview/native/aw_contents_io_thread_client_impl.cc (right):

https://codereview.chromium.org/2485513003/diff/1/android_webview/native/aw_c...
android_webview/native/aw_contents_io_thread_client_impl.cc:279: if
(java_delegate.is_null())
On 2016/11/07 17:23:33, boliu wrote:
> On 2016/11/07 17:13:01, timvolodine wrote:
> > to be more precise, I was looking here and comparing
> > g_sw_instance_.Get().is_empty() with g_sw_instance_.Get().get(env).is_null()
> it
> > seemed they were equivalent modulo the comment about
JNIEnv::NewLocalRef(obj)
> 
> They are not equivalent.
> 
> is_empty (now is_initialized) will return true even if the *referent* object
has
> been garbage collected; it's only checking whether the *reference* is
> initialized with something non-null
> 
> so it's possible for is_empty/is_initialized to be true and get(env).is_null
to
> be false

darn it, got that backwards

it's possible for get(env).is_null to be true, and is_empty to be false

[timvolodine, 2016-11-07 17:25:35]

On 2016/11/07 17:23:33, boliu wrote:
>
https://codereview.chromium.org/2485513003/diff/1/android_webview/native/aw_c...
> File android_webview/native/aw_contents_io_thread_client_impl.cc (right):
> 
>
https://codereview.chromium.org/2485513003/diff/1/android_webview/native/aw_c...
> android_webview/native/aw_contents_io_thread_client_impl.cc:279: if
> (java_delegate.is_null())
> On 2016/11/07 17:13:01, timvolodine wrote:
> > to be more precise, I was looking here and comparing
> > g_sw_instance_.Get().is_empty() with g_sw_instance_.Get().get(env).is_null()
> it
> > seemed they were equivalent modulo the comment about
JNIEnv::NewLocalRef(obj)
> 
> They are not equivalent.
> 
> is_empty (now is_initialized) will return true even if the *referent* object
has
> been garbage collected; it's only checking whether the *reference* is
> initialized with something non-null
> 
> so it's possible for is_empty/is_initialized to be true and get(env).is_null
to
> be false

yes looks like JNIEnv::NewLocalRef can actually return true when the strong
object is garbage collected..

[timvolodine, 2016-11-07 17:26:48]

On 2016/11/07 17:25:35, timvolodine wrote:
> On 2016/11/07 17:23:33, boliu wrote:
> >
>
https://codereview.chromium.org/2485513003/diff/1/android_webview/native/aw_c...
> > File android_webview/native/aw_contents_io_thread_client_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2485513003/diff/1/android_webview/native/aw_c...
> > android_webview/native/aw_contents_io_thread_client_impl.cc:279: if
> > (java_delegate.is_null())
> > On 2016/11/07 17:13:01, timvolodine wrote:
> > > to be more precise, I was looking here and comparing
> > > g_sw_instance_.Get().is_empty() with
g_sw_instance_.Get().get(env).is_null()
> > it
> > > seemed they were equivalent modulo the comment about
> JNIEnv::NewLocalRef(obj)
> > 
> > They are not equivalent.
> > 
> > is_empty (now is_initialized) will return true even if the *referent* object
> has
> > been garbage collected; it's only checking whether the *reference* is
> > initialized with something non-null
> > 
> > so it's possible for is_empty/is_initialized to be true and get(env).is_null
> to
> > be false
> 
> yes looks like JNIEnv::NewLocalRef can actually return true when the strong
> object is garbage collected..

I meant : JNIEnv::NewLocalRef return null when garbage collected :/

[sky, 2016-11-07 20:15:38]

LGTM

[timvolodine, 2016-11-07 20:16:15]

https://codereview.chromium.org/2485513003/diff/1/android_webview/native/aw_c...
File android_webview/native/aw_contents_io_thread_client_impl.cc (right):

https://codereview.chromium.org/2485513003/diff/1/android_webview/native/aw_c...
android_webview/native/aw_contents_io_thread_client_impl.cc:279: if
(java_delegate.is_null())
On 2016/11/07 17:25:29, boliu wrote:
> On 2016/11/07 17:23:33, boliu wrote:
> > On 2016/11/07 17:13:01, timvolodine wrote:
> > > to be more precise, I was looking here and comparing
> > > g_sw_instance_.Get().is_empty() with
g_sw_instance_.Get().get(env).is_null()
> > it
> > > seemed they were equivalent modulo the comment about
> JNIEnv::NewLocalRef(obj)
> > 
> > They are not equivalent.
> > 
> > is_empty (now is_initialized) will return true even if the *referent* object
> has
> > been garbage collected; it's only checking whether the *reference* is
> > initialized with something non-null
> > 
> > so it's possible for is_empty/is_initialized to be true and get(env).is_null
> to
> > be false
> 
> darn it, got that backwards
> 
> it's possible for get(env).is_null to be true, and is_empty to be false

Just to follow-up: we chatted here a bit about this, ServiceWorkerClient
currently has singleton semantics so not much gc happening unless the user
clears it explicitly. In general it seems fine (and correct) to return null in
this case, we have code to handle null cases further down the road.

[Torne, 2016-11-07 22:39:09]

The CQ bit was checked by torne@chromium.org

[commit-bot: I haz the power, 2016-11-07 22:39:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-07 23:31:22]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-11-07 23:43:36]

Description was changed from

==========
Rename JavaObjectWeakGlobalRef::is_empty to avoid misuse.

JavaObjectWeakGlobalRef::is_empty was being misused to attempt to check
if the object referred to has been collected or not (see
crbug.com/661444 for one case; others in this CL). The method is
valuable in some cases that actually just want to check if the weak
reference has been initialised, so rename it to is_uninitialized and add
an explanatory comment to make it more obvious what to do with it.

Fix a couple of existing uses that are incorrect or dubious, and rename
the "okay" uses to call the new function.

BUG=
==========

to

==========
Rename JavaObjectWeakGlobalRef::is_empty to avoid misuse.

JavaObjectWeakGlobalRef::is_empty was being misused to attempt to check
if the object referred to has been collected or not (see
crbug.com/661444 for one case; others in this CL). The method is
valuable in some cases that actually just want to check if the weak
reference has been initialised, so rename it to is_uninitialized and add
an explanatory comment to make it more obvious what to do with it.

Fix a couple of existing uses that are incorrect or dubious, and rename
the "okay" uses to call the new function.

BUG=

Committed: https://crrev.com/aef040326a50569a1838d2df24b636a90f8179f1
Cr-Commit-Position: refs/heads/master@{#430417}
==========

[commit-bot: I haz the power, 2016-11-07 23:43:37]

Patchset 1 (id:??) landed as
https://crrev.com/aef040326a50569a1838d2df24b636a90f8179f1
Cr-Commit-Position: refs/heads/master@{#430417}