Check pointer before native calls to avoid crashes.

Check pointer before native calls to avoid crashes.

BUG=727312
Review-Url: https://codereview.chromium.org/2910113002
Cr-Commit-Position: refs/heads/master@{#476159}
Committed: https://chromium.googlesource.com/chromium/src/+/8f09864d8e5e1702e719d594f05bad3e098f0a52

[Leo, 2017-05-30 03:31:17]

Description was changed from

==========
Check pointer before native calls to avoid crashes.

BUG=727312
==========

to

==========
Check pointer before native calls to avoid crashes.

BUG=727312
==========

[Leo, 2017-05-30 03:31:17]

googleo@chromium.org changed reviewers:
+ martiw@chromium.org, mdjones@chromium.org

[Leo, 2017-05-30 03:34:33]

Hey all,

I got a bug (https://bugs.chromium.org/p/chromium/issues/detail?id=727312) filed
by our QA. 

I haven't got a chance to reproduce it, but I believe it's caused by missing
pointer check.

Please review this change or share your options.

Thanks

[Marti Wong, 2017-05-30 03:42:08]

lgtm

[mdjones, 2017-05-30 15:56:54]

Is the UI still visible after native is destroyed? Do we know what the root
cause of this crash is?

https://codereview.chromium.org/2910113002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java
(right):

https://codereview.chromium.org/2910113002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:280:
if (explicitly && mNativeTranslateInfoBarPtr != 0 // Before native is destroyed.
nit: comment is vague and should be on the line above.

[Leo, 2017-05-31 00:44:03]

Move comment on the line above.

I asked our QA whether there is a way to reproduce it. Haven't got any response
and I can't reproduce it yet.

And the bug is filed based on the crash reports monitors.
https://crash.corp.google.com/browse?q=custom_data.ChromeCrashProto.magic_sig...

So one thing I can image is that in some cases, native is destroyed before UI
(eg. memory recycled, rotation...).

Anyway, I think adding the check is necessary. As 
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

Then we can check the crashes rate is back to normal after the fix is landed.

WDYT?

https://codereview.chromium.org/2910113002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java
(right):

https://codereview.chromium.org/2910113002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:280:
if (explicitly && mNativeTranslateInfoBarPtr != 0 // Before native is destroyed.
On 2017/05/30 15:56:54, mdjones wrote:
> nit: comment is vague and should be on the line above.

Acknowledged.

[mdjones, 2017-05-31 15:56:28]

lgtm % suggestion.

I'm fine with circling back to this; it seems like there is a bug in the logic
since we explicitly control native creation/destruction.

https://codereview.chromium.org/2910113002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java
(right):

https://codereview.chromium.org/2910113002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:281:
// Call native check before native is destroyed.
On second thought, just remove this comment. It's already pretty clear what this
check is.

[Leo, 2017-06-01 00:51:31]

Thanks for the review. 
There are some bugs for the native nullptr issue.
https://bugs.chromium.org/p/chromium/issues/detail?id=728230
It should happene when users is interacting with the UI, but there is a little
chance, native necessary resource is unavailable.

Basically, I believe it's not totally guaranteed the native part is always there
(eg. Leave chrome app for days then click infobar)

https://codereview.chromium.org/2910113002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java
(right):

https://codereview.chromium.org/2910113002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:281:
// Call native check before native is destroyed.
On 2017/05/31 15:56:28, mdjones wrote:
> On second thought, just remove this comment. It's already pretty clear what
this
> check is.

Acknowledged.

[Leo, 2017-06-01 00:55:47]

The CQ bit was checked by googleo@chromium.org

[Leo, 2017-06-01 00:55:47]

The patchset sent to the CQ was uploaded after l-g-t-m from martiw@chromium.org,
mdjones@chromium.org
Link to the patchset: https://codereview.chromium.org/2910113002/#ps40001
(title: "remove")

[commit-bot: I haz the power, 2017-06-01 00:56:31]

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, 2017-06-01 02:34:13]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496278547496680,
"parent_rev": "144fb7b80b8112e948a83c3fe64d45738b4450df", "commit_rev":
"8f09864d8e5e1702e719d594f05bad3e098f0a52"}

[commit-bot: I haz the power, 2017-06-01 02:34:19]

Description was changed from

==========
Check pointer before native calls to avoid crashes.

BUG=727312
==========

to

==========
Check pointer before native calls to avoid crashes.

BUG=727312

Review-Url: https://codereview.chromium.org/2910113002
Cr-Commit-Position: refs/heads/master@{#476159}
Committed:
https://chromium.googlesource.com/chromium/src/+/8f09864d8e5e1702e719d594f05b...
==========

[commit-bot: I haz the power, 2017-06-01 02:34:20]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/8f09864d8e5e1702e719d594f05b...