Safety check has been added before calling native JNI functions to prevent random crashes.

Safety check has been added before calling native JNI functions to prevent random crashes.

BUG=

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

[AKVT, 2014-04-30 16:36:12]

PTAL

[hush (inactive), 2014-04-30 17:05:26]

Hi!
What are these random crashes? Are there any reproduce steps?
It is possible that these random crashes reveal actual problems in the code. The
documentation says "Embedders must call initialize() after constructing a
ContentViewCore and before using it". So if the embedder is doing things
correctly, mNativeContentViewCore should not be NULL.

[aurimas (slooooooooow), 2014-04-30 17:06:22]

+tedchoc

[AKVT, 2014-04-30 17:06:57]

On 2014/04/30 17:05:26, hush wrote:
> Hi!
> What are these random crashes? Are there any reproduce steps?
> It is possible that these random crashes reveal actual problems in the code.
The
> documentation says "Embedders must call initialize() after constructing a
> ContentViewCore and before using it". So if the embedder is doing things
> correctly, mNativeContentViewCore should not be NULL.

As I can see all the other places in the code, before calling JNi function, we
used to valuate the mNativeContentViewCore , so I followed the same in missing
areas.

[AKVT, 2014-04-30 17:17:36]

The CQ bit was checked by ajith.v@samsung.com

[AKVT, 2014-04-30 17:17:38]

The CQ bit was unchecked by ajith.v@samsung.com

[AKVT, 2014-04-30 17:17:49]

The CQ bit was checked by ajith.v@samsung.com

[AKVT, 2014-04-30 17:17:50]

The CQ bit was unchecked by ajith.v@samsung.com

[jdduke (slow), 2014-05-01 16:33:38]

https://codereview.chromium.org/269443003/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/269443003/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1642:
if (mNativeContentViewCore != 0) {
We should probably just early return here:

if (mNativeContentViewCore == 0) return false;

[AKVT, 2014-05-02 08:40:58]

@jdduke PTAL new patch set.

https://codereview.chromium.org/269443003/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/269443003/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1642:
if (mNativeContentViewCore != 0) {
On 2014/05/01 16:33:38, jdduke wrote:
> We should probably just early return here:
> 
> if (mNativeContentViewCore == 0) return false;

Done.

[jdduke (slow), 2014-05-02 15:39:32]

https://codereview.chromium.org/269443003/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/269443003/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1649:
event.getEventTime(),
I don't believe this is the standard argument indentation for Java, the
arguments should "wrap" against a width of 120, with new lines indented by 8
spaces.

https://codereview.chromium.org/269443003/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2123:
if (mNativeContentViewCore != 0) {
I believe this will fit on a single line, so we can get rid of the braces.

[AKVT, 2014-05-02 16:28:12]

@jdduke PTAL a new patch set.

https://codereview.chromium.org/269443003/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/269443003/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1649:
event.getEventTime(),
On 2014/05/02 15:39:33, jdduke wrote:
> I don't believe this is the standard argument indentation for Java, the
> arguments should "wrap" against a width of 120, with new lines indented by 8
> spaces.

Done.

[aurimas (slooooooooow), 2014-05-02 16:40:16]

https://codereview.chromium.org/269443003/diff/40001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/269443003/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1462:
ImeAdapter.getTextInputTypeNone());
This line should be indented by 8 spaces instead of 19.

https://codereview.chromium.org/269443003/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2125:
nativeClearSslPreferences(mNativeContentViewCore);
Move this up to be on the same line with the if check.

[AKVT, 2014-05-02 16:50:33]

@jdduke and aurimas, PTAL new patch set.

https://codereview.chromium.org/269443003/diff/40001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/269443003/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1462:
ImeAdapter.getTextInputTypeNone());
On 2014/05/02 16:40:17, aurimas wrote:
> This line should be indented by 8 spaces instead of 19.

Done.

https://codereview.chromium.org/269443003/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2125:
nativeClearSslPreferences(mNativeContentViewCore);
On 2014/05/02 16:40:17, aurimas wrote:
> Move this up to be on the same line with the if check.

Done.

[jdduke (slow), 2014-05-02 17:00:47]

https://codereview.chromium.org/269443003/diff/60001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/269443003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1341:
if (mNativeContentViewCore == 0) return 0;
This is new.  Is 0 equivalent to INVALID_RENDER_PROCESS_PID? I'm worried about
callers of this method, if they rely on a valid ID maybe we should be asserting
here... (i.e., we don't have control over the calling sequence for
onConfigurationChanged/onGenericMotionEvent, but we do over
getCurrentRenderProcessId).  I would prefer we left this alone unless we
actually found that the calling sequence was valid when mNativeContentViewCore
== 0.

[AKVT, 2014-05-02 17:11:59]

@jdduke and aurimas PTAL this new patch set.

https://codereview.chromium.org/269443003/diff/60001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/269443003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1341:
if (mNativeContentViewCore == 0) return 0;
On 2014/05/02 17:00:47, jdduke wrote:
> This is new.  Is 0 equivalent to INVALID_RENDER_PROCESS_PID? I'm worried about
> callers of this method, if they rely on a valid ID maybe we should be
asserting
> here... (i.e., we don't have control over the calling sequence for
> onConfigurationChanged/onGenericMotionEvent, but we do over
> getCurrentRenderProcessId).  I would prefer we left this alone unless we
> actually found that the calling sequence was valid when mNativeContentViewCore
> == 0.

Thanks for the correction. Inner method
GetRenderProcessIdFromRenderViewHost(RenderViewHost* host) is returning 0 when
renderProcess doesn't have a connection, so I thought of propagating back to the
caller with an early exit. But as per your opinion, caller may be relying on
valid ID, so we should avoid it. I am agreeing with your point.

[jdduke (slow), 2014-05-02 17:45:26]

Thanks, lgtm (but please for sign-off from aurimas@).

[aurimas (slooooooooow), 2014-05-02 17:46:43]

lgtm

[AKVT, 2014-05-02 17:48:36]

The CQ bit was checked by ajith.v@samsung.com

[commit-bot: I haz the power, 2014-05-02 17:48:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ajith.v@samsung.com/269443003/80001

[commit-bot: I haz the power, 2014-05-02 18:05:03]

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

[commit-bot: I haz the power, 2014-05-02 18:05:04]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium

[aurimas (slooooooooow), 2014-05-02 18:06:00]

The CQ bit was checked by aurimas@chromium.org

[AKVT, 2014-05-02 18:06:04]

The CQ bit was checked by ajith.v@samsung.com

[commit-bot: I haz the power, 2014-05-02 18:06:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ajith.v@samsung.com/269443003/80001

[commit-bot: I haz the power, 2014-05-02 18:13:39]

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

[commit-bot: I haz the power, 2014-05-02 18:13:40]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium

[AKVT, 2014-05-02 18:14:57]

The CQ bit was checked by ajith.v@samsung.com

[commit-bot: I haz the power, 2014-05-02 18:15:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ajith.v@samsung.com/269443003/80001

[commit-bot: I haz the power, 2014-05-02 18:17:28]

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

[commit-bot: I haz the power, 2014-05-02 18:17:28]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium

[AKVT, 2014-05-03 00:32:23]

The CQ bit was checked by ajith.v@samsung.com

[commit-bot: I haz the power, 2014-05-03 00:32:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ajith.v@samsung.com/269443003/80001

[commit-bot: I haz the power, 2014-05-03 02:41:38]

Change committed as 268016