[Android] Fix spellcheck JNI crash when a tab is closed.

[Android] Fix spellcheck JNI crash when a tab is closed.

Closing a tab while a spellcheck request is still running can result
in a JNI crash, because the native part has already been destroyed.
This typically happens when the results from the android spellchecking
service come in with some delay which depends on the size of the
request (see crbug.com/651612).

This patch fixes this issue by zeroing the native pointer and checking
it before sending the results to the native side. It also makes sure
that this happens on the correct threads to avoid any potential race
conditions.

BUG=651612, 583616, 629609
Committed: https://crrev.com/3c92f97e4e4ad473a0788a22820235c71c84ca00
Cr-Commit-Position: refs/heads/master@{#422428}

[timvolodine, 2016-09-29 22:41:37]

The CQ bit was checked by timvolodine@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 22:42:17]

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

[timvolodine, 2016-09-29 22:43:18]

Description was changed from

==========
[Android] Fix spellcheck JNI crash when a tab is closed.

Closing a tab while a spellcheck request is still running can result
in a JNI crash, because the native part has already been destroyed.
This typically happens when the results from the android spellchecking
service come in with some delay which depends on the size of the
request (see crbug.com/651612).

This patch fixes this issue by zeroing the native pointer and checking
it before sending the results to the native side. It also makes sure
that this happens on the correct threads to avoid any potential race
conditions.

BUG=583616, 629609, 651612
==========

to

==========
[Android] Fix spellcheck JNI crash when a tab is closed.

Closing a tab while a spellcheck request is still running can result
in a JNI crash, because the native part has already been destroyed.
This typically happens when the results from the android spellchecking
service come in with some delay which depends on the size of the
request (see crbug.com/651612).

This patch fixes this issue by zeroing the native pointer and checking
it before sending the results to the native side. It also makes sure
that this happens on the correct threads to avoid any potential race
conditions.

BUG=651612, 583616, 629609
==========

[timvolodine, 2016-09-29 22:43:40]

timvolodine@chromium.org changed reviewers:
+ tobiasjs@chromium.org, torne@chromium.org

[commit-bot: I haz the power, 2016-09-30 00:04:30]

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

[commit-bot: I haz the power, 2016-09-30 00:04:31]

Dry run: This issue passed the CQ dry run.

[Tobias Sargeant, 2016-09-30 09:18:58]

https://codereview.chromium.org/2384613002/diff/1/components/spellcheck/brows...
File components/spellcheck/browser/spellcheck_message_filter_platform.h (right):

https://codereview.chromium.org/2384613002/diff/1/components/spellcheck/brows...
components/spellcheck/browser/spellcheck_message_filter_platform.h:72:
std::unique_ptr<SpellingServiceClient> client_;
Unused on Android?

https://codereview.chromium.org/2384613002/diff/1/components/spellcheck/brows...
File components/spellcheck/browser/spellchecker_session_bridge_android.cc
(right):

https://codereview.chromium.org/2384613002/diff/1/components/spellcheck/brows...
components/spellcheck/browser/spellchecker_session_bridge_android.cc:105: if
(active_request_) {
This should handle java_object_ being null. If it's possible to continue after
DisconnectSession, then it should be reusing code from RequestTextCheck to set
up the java bridge object again.

It also seems a bit weird that OnToggleSpellcheck only disconnects, and doesn't
(allow) reconnect.

https://codereview.chromium.org/2384613002/diff/1/components/spellcheck/brows...
components/spellcheck/browser/spellchecker_session_bridge_android.cc:114:
java_object_.Reset();
This also needs to call Java_SpellCheckerSessionBridge_disconnect, because the
associated java object isn't necessarily GCed instantly, and after this we've
lost our ability to zero the java side pointer in the destructor.

[timvolodine, 2016-09-30 13:57:17]

The CQ bit was checked by timvolodine@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 13:57:29]

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

[timvolodine, 2016-09-30 13:57:55]

thanks for the comments!

https://codereview.chromium.org/2384613002/diff/1/components/spellcheck/brows...
File components/spellcheck/browser/spellcheck_message_filter_platform.h (right):

https://codereview.chromium.org/2384613002/diff/1/components/spellcheck/brows...
components/spellcheck/browser/spellcheck_message_filter_platform.h:72:
std::unique_ptr<SpellingServiceClient> client_;
On 2016/09/30 09:18:57, Tobias Sargeant wrote:
> Unused on Android?

Done.

https://codereview.chromium.org/2384613002/diff/1/components/spellcheck/brows...
File components/spellcheck/browser/spellchecker_session_bridge_android.cc
(right):

https://codereview.chromium.org/2384613002/diff/1/components/spellcheck/brows...
components/spellcheck/browser/spellchecker_session_bridge_android.cc:105: if
(active_request_) {
On 2016/09/30 09:18:57, Tobias Sargeant wrote:
> This should handle java_object_ being null. If it's possible to continue after
> DisconnectSession, then it should be reusing code from RequestTextCheck to set
> up the java bridge object again.
> 
> It also seems a bit weird that OnToggleSpellcheck only disconnects, and
doesn't
> (allow) reconnect.

I guess the reconnect part is 'on demand' in RequestTextCheck. So now the
DisconnectSession also cleans up the java side and I believe it's not possible
for ProcessSpellCheckResults to be executed after disconnect.

https://codereview.chromium.org/2384613002/diff/1/components/spellcheck/brows...
components/spellcheck/browser/spellchecker_session_bridge_android.cc:114:
java_object_.Reset();
On 2016/09/30 09:18:58, Tobias Sargeant wrote:
> This also needs to call Java_SpellCheckerSessionBridge_disconnect, because the
> associated java object isn't necessarily GCed instantly, and after this we've
> lost our ability to zero the java side pointer in the destructor.

yes sounds right, thanks for catching this. if we disconnect and then close tab
the same issue can arise.
Done.

[timvolodine, 2016-09-30 13:58:11]

PTAL )

[Tobias Sargeant, 2016-09-30 14:13:21]

https://codereview.chromium.org/2384613002/diff/20001/components/spellcheck/b...
File components/spellcheck/browser/spellchecker_session_bridge_android.cc
(right):

https://codereview.chromium.org/2384613002/diff/20001/components/spellcheck/b...
components/spellcheck/browser/spellchecker_session_bridge_android.cc:73: void
SpellCheckerSessionBridge::ProcessSpellCheckResults(
DCHECK that this occurs on the UI thread, to document that expectation?

https://codereview.chromium.org/2384613002/diff/20001/components/spellcheck/b...
components/spellcheck/browser/spellchecker_session_bridge_android.cc:108: void
SpellCheckerSessionBridge::DisconnectSession() {
If there's an active request at this point that we'll no longer receive, do we
need to fire a SpellCheckMsg_RespondTextCheck?

If a reconnect can happen, we should probably also clear active_request_ and
pending_request_ here, especially as active_request_ will never return, and so
no other requests will be sent to the java bridge.

[commit-bot: I haz the power, 2016-09-30 14:37:09]

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

[commit-bot: I haz the power, 2016-09-30 14:37:10]

Dry run: This issue passed the CQ dry run.

[timvolodine, 2016-09-30 14:46:01]

https://codereview.chromium.org/2384613002/diff/20001/components/spellcheck/b...
File components/spellcheck/browser/spellchecker_session_bridge_android.cc
(right):

https://codereview.chromium.org/2384613002/diff/20001/components/spellcheck/b...
components/spellcheck/browser/spellchecker_session_bridge_android.cc:73: void
SpellCheckerSessionBridge::ProcessSpellCheckResults(
On 2016/09/30 14:13:21, Tobias Sargeant wrote:
> DCHECK that this occurs on the UI thread, to document that expectation?

Done.

https://codereview.chromium.org/2384613002/diff/20001/components/spellcheck/b...
components/spellcheck/browser/spellchecker_session_bridge_android.cc:108: void
SpellCheckerSessionBridge::DisconnectSession() {
On 2016/09/30 14:13:21, Tobias Sargeant wrote:
> If there's an active request at this point that we'll no longer receive, do we
> need to fire a SpellCheckMsg_RespondTextCheck?
> 
> If a reconnect can happen, we should probably also clear active_request_ and
> pending_request_ here, especially as active_request_ will never return, and so
> no other requests will be sent to the java bridge.

Don't think we need to send an (empty?) IPC in the disconnect case. This is
similar to the case when no spellchecker is available for some reason.

Yes added clearing of both active and pending requests --> done.

[Tobias Sargeant, 2016-09-30 14:51:56]

Ok, LGTM.

[timvolodine, 2016-09-30 15:13:26]

The CQ bit was checked by timvolodine@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 15:13:46]

Dry run: 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-09-30 16:21:01]

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

[commit-bot: I haz the power, 2016-09-30 16:21:02]

Dry run: This issue passed the CQ dry run.

[timvolodine, 2016-10-03 14:38:51]

The CQ bit was checked by timvolodine@chromium.org

[commit-bot: I haz the power, 2016-10-03 14:38:58]

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-10-03 15:45:31]

Description was changed from

==========
[Android] Fix spellcheck JNI crash when a tab is closed.

Closing a tab while a spellcheck request is still running can result
in a JNI crash, because the native part has already been destroyed.
This typically happens when the results from the android spellchecking
service come in with some delay which depends on the size of the
request (see crbug.com/651612).

This patch fixes this issue by zeroing the native pointer and checking
it before sending the results to the native side. It also makes sure
that this happens on the correct threads to avoid any potential race
conditions.

BUG=651612, 583616, 629609
==========

to

==========
[Android] Fix spellcheck JNI crash when a tab is closed.

Closing a tab while a spellcheck request is still running can result
in a JNI crash, because the native part has already been destroyed.
This typically happens when the results from the android spellchecking
service come in with some delay which depends on the size of the
request (see crbug.com/651612).

This patch fixes this issue by zeroing the native pointer and checking
it before sending the results to the native side. It also makes sure
that this happens on the correct threads to avoid any potential race
conditions.

BUG=651612, 583616, 629609
==========

[commit-bot: I haz the power, 2016-10-03 15:45:33]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-03 15:47:39]

Description was changed from

==========
[Android] Fix spellcheck JNI crash when a tab is closed.

Closing a tab while a spellcheck request is still running can result
in a JNI crash, because the native part has already been destroyed.
This typically happens when the results from the android spellchecking
service come in with some delay which depends on the size of the
request (see crbug.com/651612).

This patch fixes this issue by zeroing the native pointer and checking
it before sending the results to the native side. It also makes sure
that this happens on the correct threads to avoid any potential race
conditions.

BUG=651612, 583616, 629609
==========

to

==========
[Android] Fix spellcheck JNI crash when a tab is closed.

Closing a tab while a spellcheck request is still running can result
in a JNI crash, because the native part has already been destroyed.
This typically happens when the results from the android spellchecking
service come in with some delay which depends on the size of the
request (see crbug.com/651612).

This patch fixes this issue by zeroing the native pointer and checking
it before sending the results to the native side. It also makes sure
that this happens on the correct threads to avoid any potential race
conditions.

BUG=651612, 583616, 629609

Committed: https://crrev.com/3c92f97e4e4ad473a0788a22820235c71c84ca00
Cr-Commit-Position: refs/heads/master@{#422428}
==========

[commit-bot: I haz the power, 2016-10-03 15:47:40]

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