Don't let WebView crash on copy-paste

Don't let WebView crash on copy-paste

WebView would previously crash on copy-paste for certain devices. This more
carefully handles exceptions when getting ClipData.Items.

BUG=654802
Committed: https://crrev.com/6f3cda16c5ecca60d4eed94f094266baccfb9ed7
Cr-Commit-Position: refs/heads/master@{#425064}

[Nate Fischer, 2016-10-11 17:28:03]

Description was changed from

==========
Don't let WebView crash on copy-paste

WebView would previously crash on copy-paste for certain devices. This more
carefully handles exceptions when getting ClipData.Items.

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

to

==========
Don't let WebView crash on copy-paste

WebView would previously crash on copy-paste for certain devices. This more
carefully handles exceptions when getting ClipData.Items.

BUG=654802
==========

[Nate Fischer, 2016-10-11 17:31:53]

ntfschr@chromium.org changed reviewers:
+ twellington@chromium.org

[Nate Fischer, 2016-10-12 17:10:58]

Theresa, let me know if you'll be able to take a look at this. Although the
current implementation is technically correct according to Android APIs, it
appears that this code can lead to crashes for a handful of devices (possibly
due to modified Android APIs). The background discussion is in b/31501780.

I haven't been able to reproduce the error, since it only occurs on a few stock
devices, but it looks to me like either getItemAt or coerceToText must be
throwing a runtime exception like NPE (even though it wouldn't on most devices).

For testing this change, the best I was able to do was to modify the Android
APIs to intentionally return null. Without this patch, WebView crashes
immediately, taking the whole app down with it. With this patch, the app
survives multiple copy-paste attempts. Copy-paste is of course still broken in
this scenario, as expected with a broken API, but it seems to be an improvement
over crashing.

[Theresa, 2016-10-12 17:39:34]

https://codereview.chromium.org/2409963003/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/Clipboard.java (right):

https://codereview.chromium.org/2409963003/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/Clipboard.java:94: } catch
(RuntimeException e) {
Will it always be a RuntimeException or can other exceptions be thrown here too?
It's probably safest just to catch any Exception.


Also, does it makes sense to show the toast we show below in
setPrimaryClipNoException() when we're not able to copy so the user has an
indication that we tried to copy and failed?

[Nate Fischer, 2016-10-13 00:21:44]

On 2016/10/12 at 17:39:34, twellington wrote:
>
https://codereview.chromium.org/2409963003/diff/20001/ui/android/java/src/org...
> File ui/android/java/src/org/chromium/ui/base/Clipboard.java (right):
> 
>
https://codereview.chromium.org/2409963003/diff/20001/ui/android/java/src/org...
> ui/android/java/src/org/chromium/ui/base/Clipboard.java:94: } catch
(RuntimeException e) {
> Will it always be a RuntimeException or can other exceptions be thrown here
too? It's probably safest just to catch any Exception.

SGTM. Done

> Also, does it makes sense to show the toast we show below in
setPrimaryClipNoException() when we're not able to copy so the user has an
indication that we tried to copy and failed?

I think this function can return null for some expected cases as well, where we
weren't previously showing the toast. When I tried putting the toast in, it
seemed like the toast was showing up sometimes even when the copy actually
succeeded, which makes me think it doesn't belong here.

Also, I think depending on the exception that's actually being thrown on the
device, we'll already see this toast pop up, and we probably don't want to show
it a second time. So for those reasons, I'm leaning toward not putting the toast
in, but I can investigate further if you think it's an improvement.

[Theresa, 2016-10-13 00:24:48]

On 2016/10/13 00:21:44, Nate Fischer wrote:
> On 2016/10/12 at 17:39:34, twellington wrote:
> >
>
https://codereview.chromium.org/2409963003/diff/20001/ui/android/java/src/org...
> > File ui/android/java/src/org/chromium/ui/base/Clipboard.java (right):
> > 
> >
>
https://codereview.chromium.org/2409963003/diff/20001/ui/android/java/src/org...
> > ui/android/java/src/org/chromium/ui/base/Clipboard.java:94: } catch
> (RuntimeException e) {
> > Will it always be a RuntimeException or can other exceptions be thrown here
> too? It's probably safest just to catch any Exception.
> 
> SGTM. Done
> 
> > Also, does it makes sense to show the toast we show below in
> setPrimaryClipNoException() when we're not able to copy so the user has an
> indication that we tried to copy and failed?
> 
> I think this function can return null for some expected cases as well, where
we
> weren't previously showing the toast. When I tried putting the toast in, it
> seemed like the toast was showing up sometimes even when the copy actually
> succeeded, which makes me think it doesn't belong here.
> 
> Also, I think depending on the exception that's actually being thrown on the
> device, we'll already see this toast pop up, and we probably don't want to
show
> it a second time. So for those reasons, I'm leaning toward not putting the
toast
> in, but I can investigate further if you think it's an improvement.

That sgtm. I don't have much clipboard experience. We can always add it later if
we
run into cases where copying fails and there's no indication.

lgtm

[Nate Fischer, 2016-10-13 16:26:06]

The CQ bit was checked by ntfschr@chromium.org

[commit-bot: I haz the power, 2016-10-13 16:26:29]

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-13 16:57:09]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-13 17:00:01]

Description was changed from

==========
Don't let WebView crash on copy-paste

WebView would previously crash on copy-paste for certain devices. This more
carefully handles exceptions when getting ClipData.Items.

BUG=654802
==========

to

==========
Don't let WebView crash on copy-paste

WebView would previously crash on copy-paste for certain devices. This more
carefully handles exceptions when getting ClipData.Items.

BUG=654802

Committed: https://crrev.com/6f3cda16c5ecca60d4eed94f094266baccfb9ed7
Cr-Commit-Position: refs/heads/master@{#425064}
==========

[commit-bot: I haz the power, 2016-10-13 17:00:02]

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