Adding jobject null check before calling CalledByNative JNI functions

Removed all is_null check before calling any CalledByNative JNI functions.

BUG=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284776

[Abhishek, 2014-07-01 12:09:04]

PTAL!

[Yaron, 2014-07-01 22:41:58]

On 2014/07/01 12:09:04, Abhishek wrote:
> PTAL!

Please pick one reviewer and don't mail all OWNERS.

This seems reasonable in theory but I'd expect us to get a crash reports from
the field related to this unless we're covered by some other means. How did you
find this?

[Abhishek, 2014-07-02 00:47:27]

On 2014/07/01 22:41:58, Yaron wrote:
> On 2014/07/01 12:09:04, Abhishek wrote:
> > PTAL!
> 
> Please pick one reviewer and don't mail all OWNERS.
Sure, I'll take care from the next time. Thanks Yaron

> This seems reasonable in theory but I'd expect us to get a crash reports from
> the field related to this unless we're covered by some other means. How did
you
> find this?

As I can see all the other places in the code, before calling CalledByNative 
JNI function, we used to check the jobject for null, so I followed 
the same in missing areas.

PTAL!

[Yaron, 2014-07-02 17:36:16]

probably fine, just the one q

https://codereview.chromium.org/363563005/diff/1/chrome/browser/android/tab_a...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/363563005/diff/1/chrome/browser/android/tab_a...
chrome/browser/android/tab_android.cc:237: ScopedJavaLocalRef<jobject> obj =
weak_java_tab_.get(env);
I'm wondering about this one. Even if we don't have a java tab, two of the
notification types seemingly could function without hitting it. It's probably a
no-op cause the java tab is gone anyway but have you considered that case?

[Abhishek, 2014-07-02 19:44:56]

PTAL!

https://codereview.chromium.org/363563005/diff/1/chrome/browser/android/tab_a...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/363563005/diff/1/chrome/browser/android/tab_a...
chrome/browser/android/tab_android.cc:237: ScopedJavaLocalRef<jobject> obj =
weak_java_tab_.get(env);
On 2014/07/02 17:36:16, Yaron wrote:
> I'm wondering about this one. Even if we don't have a java tab, two of the
> notification types seemingly could function without hitting it. It's probably
a
> no-op cause the java tab is gone anyway but have you considered that case?

I think if java tab is gone, then there is no meaning of calling these
CalledByNative function for notifications(NOTIFICATION_FAVICON_UPDATED and
NOTIFICATION_NAV_ENTRY_CHANGED).

[Yaron, 2014-07-02 20:25:39]

https://codereview.chromium.org/363563005/diff/1/chrome/browser/android/tab_a...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/363563005/diff/1/chrome/browser/android/tab_a...
chrome/browser/android/tab_android.cc:237: ScopedJavaLocalRef<jobject> obj =
weak_java_tab_.get(env);
On 2014/07/02 19:44:56, Abhishek wrote:
> On 2014/07/02 17:36:16, Yaron wrote:
> > I'm wondering about this one. Even if we don't have a java tab, two of the
> > notification types seemingly could function without hitting it. It's
probably
> a
> > no-op cause the java tab is gone anyway but have you considered that case?
> 
> I think if java tab is gone, then there is no meaning of calling these
> CalledByNative function for notifications(NOTIFICATION_FAVICON_UPDATED and
> NOTIFICATION_NAV_ENTRY_CHANGED).

Agreed. I was referring to NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED

It also seems likely to not have impact but I haven't traced it through to see
what would happen

[Abhishek, 2014-07-03 05:10:42]

Yeah, I got it.
Please take a look at updated patch as suggested.

https://codereview.chromium.org/363563005/diff/1/chrome/browser/android/tab_a...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/363563005/diff/1/chrome/browser/android/tab_a...
chrome/browser/android/tab_android.cc:237: ScopedJavaLocalRef<jobject> obj =
weak_java_tab_.get(env);
On 2014/07/02 20:25:39, Yaron wrote:
> On 2014/07/02 19:44:56, Abhishek wrote:
> > On 2014/07/02 17:36:16, Yaron wrote:
> > > I'm wondering about this one. Even if we don't have a java tab, two of the
> > > notification types seemingly could function without hitting it. It's
> probably
> > a
> > > no-op cause the java tab is gone anyway but have you considered that case?
> > 
> > I think if java tab is gone, then there is no meaning of calling these
> > CalledByNative function for notifications(NOTIFICATION_FAVICON_UPDATED and
> > NOTIFICATION_NAV_ENTRY_CHANGED).
> 
> Agreed. I was referring to NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED
> 
> It also seems likely to not have impact but I haven't traced it through to see
> what would happen

Done.

[Yaron, 2014-07-07 17:14:01]

On 2014/07/03 05:10:42, Abhishek wrote:
> Yeah, I got it.
> Please take a look at updated patch as suggested.
> 
>
https://codereview.chromium.org/363563005/diff/1/chrome/browser/android/tab_a...
> File chrome/browser/android/tab_android.cc (right):
> 
>
https://codereview.chromium.org/363563005/diff/1/chrome/browser/android/tab_a...
> chrome/browser/android/tab_android.cc:237: ScopedJavaLocalRef<jobject> obj =
> weak_java_tab_.get(env);
> On 2014/07/02 20:25:39, Yaron wrote:
> > On 2014/07/02 19:44:56, Abhishek wrote:
> > > On 2014/07/02 17:36:16, Yaron wrote:
> > > > I'm wondering about this one. Even if we don't have a java tab, two of
the
> > > > notification types seemingly could function without hitting it. It's
> > probably
> > > a
> > > > no-op cause the java tab is gone anyway but have you considered that
case?
> > > 
> > > I think if java tab is gone, then there is no meaning of calling these
> > > CalledByNative function for notifications(NOTIFICATION_FAVICON_UPDATED and
> > > NOTIFICATION_NAV_ENTRY_CHANGED).
> > 
> > Agreed. I was referring to NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED
> > 
> > It also seems likely to not have impact but I haven't traced it through to
see
> > what would happen
> 
> Done.

Sorry if I was unclear but I was hoping you would actually investigate the
question I had. It could be that your previous patchset was more correct. What
I'm not sure about is whether any of the processing for
NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED should be done if the java tab isn't
available. I don't know the answer. This is safe as-is but since you're
modifying the code it's worth understanding what happens in that case.

[David Trainor- moved to gerrit, 2014-07-07 18:23:02]

Can the Tab really be gone without this class being gone?  If so Tab#destroy()
wasn't properly called.  I'd rather find that case than add these patches as
they're just hiding a bug.

[David Trainor- moved to gerrit, 2014-07-07 18:24:24]

On 2014/07/07 18:23:02, David Trainor wrote:
> Can the Tab really be gone without this class being gone?  If so Tab#destroy()
> wasn't properly called.  I'd rather find that case than add these patches as
> they're just hiding a bug.

I'd be tempted to add a finalizer to Tab to check to make sure we called
destroy, but I don't want to incur the extra GC step before actually cleaning up
the object... Might be worth it though.  Yaron WDYT?

[Abhishek, 2014-07-08 11:41:47]

On 2014/07/07 17:14:01, Yaron wrote:
> On 2014/07/03 05:10:42, Abhishek wrote:
> > Yeah, I got it.
> > Please take a look at updated patch as suggested.
> > 
> >
>
https://codereview.chromium.org/363563005/diff/1/chrome/browser/android/tab_a...
> > File chrome/browser/android/tab_android.cc (right):
> > 
> >
>
https://codereview.chromium.org/363563005/diff/1/chrome/browser/android/tab_a...
> > chrome/browser/android/tab_android.cc:237: ScopedJavaLocalRef<jobject> obj =
> > weak_java_tab_.get(env);
> > On 2014/07/02 20:25:39, Yaron wrote:
> > > On 2014/07/02 19:44:56, Abhishek wrote:
> > > > On 2014/07/02 17:36:16, Yaron wrote:
> > > > > I'm wondering about this one. Even if we don't have a java tab, two of
> the
> > > > > notification types seemingly could function without hitting it. It's
> > > probably
> > > > a
> > > > > no-op cause the java tab is gone anyway but have you considered that
> case?
> > > > 
> > > > I think if java tab is gone, then there is no meaning of calling these
> > > > CalledByNative function for notifications(NOTIFICATION_FAVICON_UPDATED
and
> > > > NOTIFICATION_NAV_ENTRY_CHANGED).
> > > 
> > > Agreed. I was referring to NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED
> > > 
> > > It also seems likely to not have impact but I haven't traced it through to
> see
> > > what would happen
> > 
> > Done.
> 
> Sorry if I was unclear but I was hoping you would actually investigate the
> question I had. It could be that your previous patchset was more correct. What
> I'm not sure about is whether any of the processing for
> NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED should be done if the java tab isn't
> available. I don't know the answer. This is safe as-is but since you're
> modifying the code it's worth understanding what happens in that case.

Sorry !
I investigated more related to NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED, 
its checking for content blockage was indicated to user, which is also of 
tab specific settings. If content blockage was not indicated then getting 
the PopupBlockerTabHelper from web content; creating and setting for 
content popup blockage has been indicated to "true" inside the 
TabSpecificContentSettings class.

So the above notification also related to current java tab only. 
Iff java tab is there then only its useful. 
Now I'm also thinking that my last patch was more correct, which you 
also mentioned.

I'll upload it again, once we are on same floor.

Thanks for the discussion.

PTAL!

[Yaron, 2014-07-08 21:44:44]

On 2014/07/08 11:41:47, Abhishek wrote:
> On 2014/07/07 17:14:01, Yaron wrote:
> > On 2014/07/03 05:10:42, Abhishek wrote:
> > > Yeah, I got it.
> > > Please take a look at updated patch as suggested.
> > > 
> > >
> >
>
https://codereview.chromium.org/363563005/diff/1/chrome/browser/android/tab_a...
> > > File chrome/browser/android/tab_android.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/363563005/diff/1/chrome/browser/android/tab_a...
> > > chrome/browser/android/tab_android.cc:237: ScopedJavaLocalRef<jobject> obj
=
> > > weak_java_tab_.get(env);
> > > On 2014/07/02 20:25:39, Yaron wrote:
> > > > On 2014/07/02 19:44:56, Abhishek wrote:
> > > > > On 2014/07/02 17:36:16, Yaron wrote:
> > > > > > I'm wondering about this one. Even if we don't have a java tab, two
of
> > the
> > > > > > notification types seemingly could function without hitting it. It's
> > > > probably
> > > > > a
> > > > > > no-op cause the java tab is gone anyway but have you considered that
> > case?
> > > > > 
> > > > > I think if java tab is gone, then there is no meaning of calling these
> > > > > CalledByNative function for notifications(NOTIFICATION_FAVICON_UPDATED
> and
> > > > > NOTIFICATION_NAV_ENTRY_CHANGED).
> > > > 
> > > > Agreed. I was referring to NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED
> > > > 
> > > > It also seems likely to not have impact but I haven't traced it through
to
> > see
> > > > what would happen
> > > 
> > > Done.
> > 
> > Sorry if I was unclear but I was hoping you would actually investigate the
> > question I had. It could be that your previous patchset was more correct.
What
> > I'm not sure about is whether any of the processing for
> > NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED should be done if the java tab
isn't
> > available. I don't know the answer. This is safe as-is but since you're
> > modifying the code it's worth understanding what happens in that case.
> 
> Sorry !
> I investigated more related to NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED, 
> its checking for content blockage was indicated to user, which is also of 
> tab specific settings. If content blockage was not indicated then getting 
> the PopupBlockerTabHelper from web content; creating and setting for 
> content popup blockage has been indicated to "true" inside the 
> TabSpecificContentSettings class.
> 
> So the above notification also related to current java tab only. 
> Iff java tab is there then only its useful. 
> Now I'm also thinking that my last patch was more correct, which you 
> also mentioned.
> 
> I'll upload it again, once we are on same floor.
> 
sgtm

> Thanks for the discussion.
> 
> PTAL!

David: I'd like to avoid a finalizer for this. In theory it can happen because
the native class only has a weak reference to java. In practice, I think you're
right that it can't happen.

I think Abhishek is trying to shore this up but we don't actually have evidence
of it happening.

[David Trainor- moved to gerrit, 2014-07-09 03:24:13]

On 2014/07/08 21:44:44, Yaron wrote:
> On 2014/07/08 11:41:47, Abhishek wrote:
> > On 2014/07/07 17:14:01, Yaron wrote:
> > > On 2014/07/03 05:10:42, Abhishek wrote:
> > > > Yeah, I got it.
> > > > Please take a look at updated patch as suggested.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/363563005/diff/1/chrome/browser/android/tab_a...
> > > > File chrome/browser/android/tab_android.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/363563005/diff/1/chrome/browser/android/tab_a...
> > > > chrome/browser/android/tab_android.cc:237: ScopedJavaLocalRef<jobject>
obj
> =
> > > > weak_java_tab_.get(env);
> > > > On 2014/07/02 20:25:39, Yaron wrote:
> > > > > On 2014/07/02 19:44:56, Abhishek wrote:
> > > > > > On 2014/07/02 17:36:16, Yaron wrote:
> > > > > > > I'm wondering about this one. Even if we don't have a java tab,
two
> of
> > > the
> > > > > > > notification types seemingly could function without hitting it.
It's
> > > > > probably
> > > > > > a
> > > > > > > no-op cause the java tab is gone anyway but have you considered
that
> > > case?
> > > > > > 
> > > > > > I think if java tab is gone, then there is no meaning of calling
these
> > > > > > CalledByNative function for
notifications(NOTIFICATION_FAVICON_UPDATED
> > and
> > > > > > NOTIFICATION_NAV_ENTRY_CHANGED).
> > > > > 
> > > > > Agreed. I was referring to NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED
> > > > > 
> > > > > It also seems likely to not have impact but I haven't traced it
through
> to
> > > see
> > > > > what would happen
> > > > 
> > > > Done.
> > > 
> > > Sorry if I was unclear but I was hoping you would actually investigate the
> > > question I had. It could be that your previous patchset was more correct.
> What
> > > I'm not sure about is whether any of the processing for
> > > NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED should be done if the java tab
> isn't
> > > available. I don't know the answer. This is safe as-is but since you're
> > > modifying the code it's worth understanding what happens in that case.
> > 
> > Sorry !
> > I investigated more related to NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED, 
> > its checking for content blockage was indicated to user, which is also of 
> > tab specific settings. If content blockage was not indicated then getting 
> > the PopupBlockerTabHelper from web content; creating and setting for 
> > content popup blockage has been indicated to "true" inside the 
> > TabSpecificContentSettings class.
> > 
> > So the above notification also related to current java tab only. 
> > Iff java tab is there then only its useful. 
> > Now I'm also thinking that my last patch was more correct, which you 
> > also mentioned.
> > 
> > I'll upload it again, once we are on same floor.
> > 
> sgtm
> 
> > Thanks for the discussion.
> > 
> > PTAL!
> 
> David: I'd like to avoid a finalizer for this. In theory it can happen because
> the native class only has a weak reference to java. In practice, I think
you're
> right that it can't happen.
> 
> I think Abhishek is trying to shore this up but we don't actually have
evidence
> of it happening.

Aah ok.  Hmm I would almost rather just let it crash.  If we ever have a native
Tab without a Java tab we're in a horrible horrible state and we're leaking
memory.

[Abhishek, 2014-07-09 09:20:39]

On 2014/07/09 03:24:13, David Trainor wrote:
> On 2014/07/08 21:44:44, Yaron wrote:
> > On 2014/07/08 11:41:47, Abhishek wrote:
> > > On 2014/07/07 17:14:01, Yaron wrote:
> > > > On 2014/07/03 05:10:42, Abhishek wrote:
> > > > > Yeah, I got it.
> > > > > Please take a look at updated patch as suggested.
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/363563005/diff/1/chrome/browser/android/tab_a...
> > > > > File chrome/browser/android/tab_android.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/363563005/diff/1/chrome/browser/android/tab_a...
> > > > > chrome/browser/android/tab_android.cc:237: ScopedJavaLocalRef<jobject>
> obj
> > =
> > > > > weak_java_tab_.get(env);
> > > > > On 2014/07/02 20:25:39, Yaron wrote:
> > > > > > On 2014/07/02 19:44:56, Abhishek wrote:
> > > > > > > On 2014/07/02 17:36:16, Yaron wrote:
> > > > > > > > I'm wondering about this one. Even if we don't have a java tab,
> two
> > of
> > > > the
> > > > > > > > notification types seemingly could function without hitting it.
> It's
> > > > > > probably
> > > > > > > a
> > > > > > > > no-op cause the java tab is gone anyway but have you considered
> that
> > > > case?
> > > > > > > 
> > > > > > > I think if java tab is gone, then there is no meaning of calling
> these
> > > > > > > CalledByNative function for
> notifications(NOTIFICATION_FAVICON_UPDATED
> > > and
> > > > > > > NOTIFICATION_NAV_ENTRY_CHANGED).
> > > > > > 
> > > > > > Agreed. I was referring to NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED
> > > > > > 
> > > > > > It also seems likely to not have impact but I haven't traced it
> through
> > to
> > > > see
> > > > > > what would happen
> > > > > 
> > > > > Done.
> > > > 
> > > > Sorry if I was unclear but I was hoping you would actually investigate
the
> > > > question I had. It could be that your previous patchset was more
correct.
> > What
> > > > I'm not sure about is whether any of the processing for
> > > > NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED should be done if the java tab
> > isn't
> > > > available. I don't know the answer. This is safe as-is but since you're
> > > > modifying the code it's worth understanding what happens in that case.
> > > 
> > > Sorry !
> > > I investigated more related to NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED, 
> > > its checking for content blockage was indicated to user, which is also of 
> > > tab specific settings. If content blockage was not indicated then getting 
> > > the PopupBlockerTabHelper from web content; creating and setting for 
> > > content popup blockage has been indicated to "true" inside the 
> > > TabSpecificContentSettings class.
> > > 
> > > So the above notification also related to current java tab only. 
> > > Iff java tab is there then only its useful. 
> > > Now I'm also thinking that my last patch was more correct, which you 
> > > also mentioned.
> > > 
> > > I'll upload it again, once we are on same floor.
> > > 
> > sgtm
> > 
> > > Thanks for the discussion.
> > > 
> > > PTAL!
> > 
> > David: I'd like to avoid a finalizer for this. In theory it can happen
because
> > the native class only has a weak reference to java. In practice, I think
> you're
> > right that it can't happen.
> > 
> > I think Abhishek is trying to shore this up but we don't actually have
> evidence
> > of it happening.
> 
> Aah ok.  Hmm I would almost rather just let it crash.  If we ever have a
native
> Tab without a Java tab we're in a horrible horrible state and we're leaking
> memory.

@yaron:

I have reverted my patch set#2 changes to original state of this patch.

PTAL!

[David Trainor- moved to gerrit, 2014-07-09 20:40:04]

On 2014/07/09 09:20:39, Abhishek wrote:
> On 2014/07/09 03:24:13, David Trainor wrote:
> > On 2014/07/08 21:44:44, Yaron wrote:
> > > On 2014/07/08 11:41:47, Abhishek wrote:
> > > > On 2014/07/07 17:14:01, Yaron wrote:
> > > > > On 2014/07/03 05:10:42, Abhishek wrote:
> > > > > > Yeah, I got it.
> > > > > > Please take a look at updated patch as suggested.
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/363563005/diff/1/chrome/browser/android/tab_a...
> > > > > > File chrome/browser/android/tab_android.cc (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/363563005/diff/1/chrome/browser/android/tab_a...
> > > > > > chrome/browser/android/tab_android.cc:237:
ScopedJavaLocalRef<jobject>
> > obj
> > > =
> > > > > > weak_java_tab_.get(env);
> > > > > > On 2014/07/02 20:25:39, Yaron wrote:
> > > > > > > On 2014/07/02 19:44:56, Abhishek wrote:
> > > > > > > > On 2014/07/02 17:36:16, Yaron wrote:
> > > > > > > > > I'm wondering about this one. Even if we don't have a java
tab,
> > two
> > > of
> > > > > the
> > > > > > > > > notification types seemingly could function without hitting
it.
> > It's
> > > > > > > probably
> > > > > > > > a
> > > > > > > > > no-op cause the java tab is gone anyway but have you
considered
> > that
> > > > > case?
> > > > > > > > 
> > > > > > > > I think if java tab is gone, then there is no meaning of calling
> > these
> > > > > > > > CalledByNative function for
> > notifications(NOTIFICATION_FAVICON_UPDATED
> > > > and
> > > > > > > > NOTIFICATION_NAV_ENTRY_CHANGED).
> > > > > > > 
> > > > > > > Agreed. I was referring to
NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED
> > > > > > > 
> > > > > > > It also seems likely to not have impact but I haven't traced it
> > through
> > > to
> > > > > see
> > > > > > > what would happen
> > > > > > 
> > > > > > Done.
> > > > > 
> > > > > Sorry if I was unclear but I was hoping you would actually investigate
> the
> > > > > question I had. It could be that your previous patchset was more
> correct.
> > > What
> > > > > I'm not sure about is whether any of the processing for
> > > > > NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED should be done if the java
tab
> > > isn't
> > > > > available. I don't know the answer. This is safe as-is but since
you're
> > > > > modifying the code it's worth understanding what happens in that case.
> > > > 
> > > > Sorry !
> > > > I investigated more related to
NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED, 
> > > > its checking for content blockage was indicated to user, which is also
of 
> > > > tab specific settings. If content blockage was not indicated then
getting 
> > > > the PopupBlockerTabHelper from web content; creating and setting for 
> > > > content popup blockage has been indicated to "true" inside the 
> > > > TabSpecificContentSettings class.
> > > > 
> > > > So the above notification also related to current java tab only. 
> > > > Iff java tab is there then only its useful. 
> > > > Now I'm also thinking that my last patch was more correct, which you 
> > > > also mentioned.
> > > > 
> > > > I'll upload it again, once we are on same floor.
> > > > 
> > > sgtm
> > > 
> > > > Thanks for the discussion.
> > > > 
> > > > PTAL!
> > > 
> > > David: I'd like to avoid a finalizer for this. In theory it can happen
> because
> > > the native class only has a weak reference to java. In practice, I think
> > you're
> > > right that it can't happen.
> > > 
> > > I think Abhishek is trying to shore this up but we don't actually have
> > evidence
> > > of it happening.
> > 
> > Aah ok.  Hmm I would almost rather just let it crash.  If we ever have a
> native
> > Tab without a Java tab we're in a horrible horrible state and we're leaking
> > memory.
> 
> @yaron:
> 
> I have reverted my patch set#2 changes to original state of this patch.
> 
> PTAL!

If there's no way this should happen (and I can't see one), maybe this CL should
just remove the other is_null checks instead of adding more.  That might be
cleaner.

[Yaron, 2014-07-09 20:41:47]

I think that's reasonable. tbh, if it works, I prefer it because it's
easier to reason about


On Wed, Jul 9, 2014 at 1:40 PM, <dtrainor@chromium.org> wrote:

> On 2014/07/09 09:20:39, Abhishek wrote:
>
>> On 2014/07/09 03:24:13, David Trainor wrote:
>> > On 2014/07/08 21:44:44, Yaron wrote:
>> > > On 2014/07/08 11:41:47, Abhishek wrote:
>> > > > On 2014/07/07 17:14:01, Yaron wrote:
>> > > > > On 2014/07/03 05:10:42, Abhishek wrote:
>> > > > > > Yeah, I got it.
>> > > > > > Please take a look at updated patch as suggested.
>> > > > > >
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/363563005/diff/1/chrome/
> browser/android/tab_android.cc
>
>> > > > > > File chrome/browser/android/tab_android.cc (right):
>> > > > > >
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/363563005/diff/1/chrome/
> browser/android/tab_android.cc#newcode237
>
>> > > > > > chrome/browser/android/tab_android.cc:237:
>>
> ScopedJavaLocalRef<jobject>
>
>> > obj
>> > > =
>> > > > > > weak_java_tab_.get(env);
>> > > > > > On 2014/07/02 20:25:39, Yaron wrote:
>> > > > > > > On 2014/07/02 19:44:56, Abhishek wrote:
>> > > > > > > > On 2014/07/02 17:36:16, Yaron wrote:
>> > > > > > > > > I'm wondering about this one. Even if we don't have a java
>>
> tab,
>
>> > two
>> > > of
>> > > > > the
>> > > > > > > > > notification types seemingly could function without
>> hitting
>>
> it.
>
>> > It's
>> > > > > > > probably
>> > > > > > > > a
>> > > > > > > > > no-op cause the java tab is gone anyway but have you
>>
> considered
>
>> > that
>> > > > > case?
>> > > > > > > >
>> > > > > > > > I think if java tab is gone, then there is no meaning of
>> calling
>> > these
>> > > > > > > > CalledByNative function for
>> > notifications(NOTIFICATION_FAVICON_UPDATED
>> > > > and
>> > > > > > > > NOTIFICATION_NAV_ENTRY_CHANGED).
>> > > > > > >
>> > > > > > > Agreed. I was referring to
>>
> NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED
>
>> > > > > > >
>> > > > > > > It also seems likely to not have impact but I haven't traced
>> it
>> > through
>> > > to
>> > > > > see
>> > > > > > > what would happen
>> > > > > >
>> > > > > > Done.
>> > > > >
>> > > > > Sorry if I was unclear but I was hoping you would actually
>> investigate
>> the
>> > > > > question I had. It could be that your previous patchset was more
>> correct.
>> > > What
>> > > > > I'm not sure about is whether any of the processing for
>> > > > > NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED should be done if the
>> java
>>
> tab
>
>> > > isn't
>> > > > > available. I don't know the answer. This is safe as-is but since
>>
> you're
>
>> > > > > modifying the code it's worth understanding what happens in that
>> case.
>> > > >
>> > > > Sorry !
>> > > > I investigated more related to
>>
> NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED,
>
>> > > > its checking for content blockage was indicated to user, which is
>> also
>>
> of
>
>> > > > tab specific settings. If content blockage was not indicated then
>>
> getting
>
>> > > > the PopupBlockerTabHelper from web content; creating and setting for
>> > > > content popup blockage has been indicated to "true" inside the
>> > > > TabSpecificContentSettings class.
>> > > >
>> > > > So the above notification also related to current java tab only.
>> > > > Iff java tab is there then only its useful.
>> > > > Now I'm also thinking that my last patch was more correct, which you
>> > > > also mentioned.
>> > > >
>> > > > I'll upload it again, once we are on same floor.
>> > > >
>> > > sgtm
>> > >
>> > > > Thanks for the discussion.
>> > > >
>> > > > PTAL!
>> > >
>> > > David: I'd like to avoid a finalizer for this. In theory it can happen
>> because
>> > > the native class only has a weak reference to java. In practice, I
>> think
>> > you're
>> > > right that it can't happen.
>> > >
>> > > I think Abhishek is trying to shore this up but we don't actually have
>> > evidence
>> > > of it happening.
>> >
>> > Aah ok.  Hmm I would almost rather just let it crash.  If we ever have a
>> native
>> > Tab without a Java tab we're in a horrible horrible state and we're
>> leaking
>> > memory.
>>
>
>  @yaron:
>>
>
>  I have reverted my patch set#2 changes to original state of this patch.
>>
>
>  PTAL!
>>
>
> If there's no way this should happen (and I can't see one), maybe this CL
> should
> just remove the other is_null checks instead of adding more.  That might be
> cleaner.
>
> https://chromiumcodereview.appspot.com/363563005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Trainor- moved to gerrit, 2014-07-22 07:17:30]

lgtm

[Abhishek, 2014-07-22 08:33:19]

The CQ bit was checked by abhishek.a21@samsung.com

[commit-bot: I haz the power, 2014-07-22 08:34:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/abhishek.a21@samsung.com/363563005/60001

[commit-bot: I haz the power, 2014-07-22 10:33:45]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-07-22 11:34:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-22 11:34:22]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[Abhishek, 2014-07-22 12:26:47]

On 2014/07/22 11:34:22, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   win_chromium_x64_rel on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

From the build logs its crashing with below logs:

[  PASSED  ] 445 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] 
backends.chrome.inspector_runtime_unittest.InspectorRuntimeTest.testIFrame

1 FAILED TEST

exit code (as seen by runtest.py): 1
@@@STEP_FAILURE@@@
@@@STEP_TEXT@telemetry_unittests@@@
@@@STEP_TEXT@crashed or hung@@@
Confused: 66 files were deleted from c:\users\chrome~2\appdata\local\temp during
the test run

Please help me to resolve this issue, it would be a gr8 help !

[Yaron, 2014-07-22 17:20:33]

On 2014/07/22 12:26:47, Abhishek wrote:
> On 2014/07/22 11:34:22, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   win_chromium_x64_rel on tryserver.chromium
> >
>
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
> 
> From the build logs its crashing with below logs:
> 
> [  PASSED  ] 445 tests.
> [  FAILED  ] 1 test, listed below:
> [  FAILED  ] 
> backends.chrome.inspector_runtime_unittest.InspectorRuntimeTest.testIFrame
> 
> 1 FAILED TEST
> 
> exit code (as seen by runtest.py): 1
> @@@STEP_FAILURE@@@
> @@@STEP_TEXT@telemetry_unittests@@@
> @@@STEP_TEXT@crashed or hung@@@
> Confused: 66 files were deleted from c:\users\chrome~2\appdata\local\temp
during
> the test run
> 
> Please help me to resolve this issue, it would be a gr8 help !

That has to be an unrelated flake. This change is to an android file and isn't
used on windows.

[Yaron, 2014-07-22 17:20:52]

That said, please update the CL description because the underlying code has
changed

[Abhishek, 2014-07-22 18:48:06]

The CQ bit was checked by abhishek.a21@samsung.com

[commit-bot: I haz the power, 2014-07-22 18:49:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/abhishek.a21@samsung.com/363563005/60001

[commit-bot: I haz the power, 2014-07-22 21:21:08]

Change committed as 284776