Prevent guest views from issuing a search for empty text.

Prevent guest views from issuing a search for empty text.

Do the same for Android webview.

Set up roadblocks along the way to prevent this from happening again.

BUG=535086
Committed: https://crrev.com/6057a6b26a758d3542b9d74579e4e517b49244a9
Cr-Commit-Position: refs/heads/master@{#359419}

[Lei Zhang, 2015-09-26 00:08:26]

thestig@chromium.org changed reviewers:
+ creis@chromium.org, fsamuel@chromium.org, raymes@chromium.org

[Lei Zhang, 2015-09-26 00:08:26]

creis: contents
fsamuel: extensions
raymes: pdf, ppapi
paulmeyeer: FYI

[Lei Zhang, 2015-09-26 00:12:05]

And CC the right paulmeyer.

[Fady Samuel, 2015-09-26 00:14:33]

fsamuel@chromium.org changed reviewers:
+ paulmeyer@chromium.org

[Fady Samuel, 2015-09-26 00:14:34]

+paulmeyer@ for find API.

https://codereview.chromium.org/1365563003/diff/20001/extensions/browser/api/...
File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc
(right):

https://codereview.chromium.org/1365563003/diff/20001/extensions/browser/api/...
extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:637: if
(search_text.empty()) {
I don't think this is the correct solution. I think this breaks the <webview>
Find API. I believe you can provide find with an empty string to "FindNext".
paulmeyer@ wrote this API. Passing this bug along to him.

[Lei Zhang, 2015-09-26 00:22:01]

https://codereview.chromium.org/1365563003/diff/20001/extensions/browser/api/...
File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc
(right):

https://codereview.chromium.org/1365563003/diff/20001/extensions/browser/api/...
extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:637: if
(search_text.empty()) {
On 2015/09/26 00:14:34, Fady Samuel wrote:
> I don't think this is the correct solution. I think this breaks the <webview>
> Find API. I believe you can provide find with an empty string to "FindNext".
> paulmeyer@ wrote this API. Passing this bug along to him.

You are referring to https://developer.chrome.com/apps/tags/webview#method-find
right?

What's the point of searching for no text? I guess the alternative is to just
immediately reply "successfully" with no results.

[Fady Samuel, 2015-09-26 00:32:57]

On 2015/09/26 00:22:01, Lei Zhang wrote:
>
https://codereview.chromium.org/1365563003/diff/20001/extensions/browser/api/...
> File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc
> (right):
> 
>
https://codereview.chromium.org/1365563003/diff/20001/extensions/browser/api/...
> extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:637: if
> (search_text.empty()) {
> On 2015/09/26 00:14:34, Fady Samuel wrote:
> > I don't think this is the correct solution. I think this breaks the
<webview>
> > Find API. I believe you can provide find with an empty string to "FindNext".
> > paulmeyer@ wrote this API. Passing this bug along to him.
> 
> You are referring to
https://developer.chrome.com/apps/tags/webview#method-find
> right?
> 
> What's the point of searching for no text? I guess the alternative is to just
> immediately reply "successfully" with no results.

Yes, I think I'm wrong. If you search for no text currently, it'll clear the
find session. Maybe we should do that?

[Lei Zhang, 2015-09-26 01:06:27]

Oh good, we have some WebViewAsynchronousFindApisTests. Was getting worried
there with no bots going red. ;)

[raymes, 2015-09-27 22:50:37]

lgtm

[Charlie Reis, 2015-09-28 19:35:18]

content/ LGTM.

https://codereview.chromium.org/1365563003/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1365563003/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2728: NOTREACHED();
I take it there's some other code that returns early if you hit enter in an
empty Find box on a normal tab?

https://codereview.chromium.org/1365563003/diff/20001/extensions/browser/api/...
File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc
(right):

https://codereview.chromium.org/1365563003/diff/20001/extensions/browser/api/...
extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:637: if
(search_text.empty()) {
On 2015/09/26 00:22:01, Lei Zhang wrote:
> On 2015/09/26 00:14:34, Fady Samuel wrote:
> > I don't think this is the correct solution. I think this breaks the
<webview>
> > Find API. I believe you can provide find with an empty string to "FindNext".
> > paulmeyer@ wrote this API. Passing this bug along to him.
> 
> You are referring to
https://developer.chrome.com/apps/tags/webview#method-find
> right?
> 
> What's the point of searching for no text? I guess the alternative is to just
> immediately reply "successfully" with no results.

It does seem like a nonsensical case to me.  I'd be ok with either returning 0
results or an error, depending on what the <webview> folks want their API to do.

[Lei Zhang, 2015-09-28 20:01:51]

If I read testFindAllEmptyString correctly, the current expectation is to
succeed and return no results. We can keep this behavior, but skip the
round-trip to the renderer. I'll make a new patchset and throw it at the Android
trybot.

https://codereview.chromium.org/1365563003/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1365563003/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2728: NOTREACHED();
On 2015/09/28 19:35:17, Charlie Reis wrote:
> I take it there's some other code that returns early if you hit enter in an
> empty Find box on a normal tab?

Yes, the normal find text box has a check.

[Lei Zhang, 2015-10-07 23:41:50]

Paul, any thoughts on this?

[paulmeyer, 2015-10-08 13:49:41]

On 2015/10/07 23:41:50, Lei Zhang wrote:
> Paul, any thoughts on this?

I agree that searching for empty string should return successfully with no
results.

As for the failing tests, it looks like they depend on find requests for empty
string going through... so perhaps you have set up too many roadblocks. For
example, won't the check in web_contents_impl.cc stop any find request for empty
string from even starting, and therefore also preventing any results from
getting returned? Maybe that is causing the tests to fail.

I would suggest that you print out what is actually being returned by the
individual function calls in the asserts within the failing tests, and see if
they are just getting the wrong results, or if they are hanging because they are
never getting results. I would bet that they are never getting results.

Another question... if all you care about is preventing guest views from
searching for empty text (and web view is the only guest view with a find API),
wouldn't it be sufficient to just make the change you've made in
web_view_find_helper.cc, without all of the other checks in other places? Again,
it looks like those failing tests are depending on empty find requests going
through in some cases.

Another interesting thing I've noticed is that in the Browser Sample app,
navigated to a PDF, if you search for at least one non-empty string in the find
bar, and then empty it and continue hitting the arrows to search for empty
string again, the PDF never crashes in that case. So, it looks like the PDF only
crashes if the very first find request is with an empty string.

[Lei Zhang, 2015-11-11 07:37:31]

... and a month later, I finally got around to this CL and actually fixing the
Android tests.

On 2015/10/08 13:49:41, Paul Meyer wrote:
> On 2015/10/07 23:41:50, Lei Zhang wrote:
> > Paul, any thoughts on this?
> 
> I agree that searching for empty string should return successfully with no
> results.
> 
> As for the failing tests, it looks like they depend on find requests for empty
> string going through... so perhaps you have set up too many roadblocks. For
> example, won't the check in web_contents_impl.cc stop any find request for
empty
> string from even starting, and therefore also preventing any results from
> getting returned? Maybe that is causing the tests to fail.

No, the roadblocks were good to have. They helped me track down where it was
going wrong. I'll find an android_webview OWNER to review that portion. There's
now 10 patch set to show the incremental progress, with all my blind stabs in
the dark. I finally bit the bullet and figured out how to run the Android
emulator locally. (The tricks were to: go for an emulator with API level 23
instead of 19, and build for x86, not ARM)

> I would suggest that you print out what is actually being returned by the
> individual function calls in the asserts within the failing tests, and see if
> they are just getting the wrong results, or if they are hanging because they
are
> never getting results. I would bet that they are never getting results.
> 
> Another question... if all you care about is preventing guest views from
> searching for empty text (and web view is the only guest view with a find
API),
> wouldn't it be sufficient to just make the change you've made in
> web_view_find_helper.cc, without all of the other checks in other places?
Again,
> it looks like those failing tests are depending on empty find requests going
> through in some cases.

Nope, nobody should be sending empty search requests according to my checks.
I've plugged all the known holes. Let's see if all the trybots go green.

> Another interesting thing I've noticed is that in the Browser Sample app,
> navigated to a PDF, if you search for at least one non-empty string in the
find
> bar, and then empty it and continue hitting the arrows to search for empty
> string again, the PDF never crashes in that case. So, it looks like the PDF
only
> crashes if the very first find request is with an empty string.

Yes, there's an index that's initialized to -1...

[Lei Zhang, 2015-11-11 07:43:29]

Description was changed from

==========
Prevent guest views from issuing a search for empty text.

Set up roadblocks along the way to prevent this from happening again.

BUG=535086
==========

to

==========
Prevent guest views from issuing a search for empty text.

Do the same for Android webview.

Set up roadblocks along the way to prevent this from happening again.

BUG=535086
==========

[Lei Zhang, 2015-11-11 07:46:33]

thestig@chromium.org changed reviewers:
+ boliu@chromium.org

[Lei Zhang, 2015-11-11 07:47:53]

+boliu for android_webview/OWNERS, since he touched
android_webview/browser/find_helper.* in http://crrev.com/186827 and forgot to
remove some dead code. :-P

[boliu, 2015-11-11 16:35:01]

On 2015/11/11 07:47:53, Lei Zhang wrote:
> +boliu for android_webview/OWNERS, since he touched
> android_webview/browser/find_helper.* in http://crrev.com/186827 and forgot to
> remove some dead code. :-P

LGTM

[Lei Zhang, 2015-11-11 16:56:55]

Paul/Fady: PTAL at extensions/

[Fady Samuel, 2015-11-12 18:25:54]

On 2015/11/11 16:56:55, Lei Zhang wrote:
> Paul/Fady: PTAL at extensions/

I'll let Paul chime in on this one because he wrote the <webview> find API.

[paulmeyer, 2015-11-12 18:34:24]

On 2015/11/12 18:25:54, Fady Samuel wrote:
> On 2015/11/11 16:56:55, Lei Zhang wrote:
> > Paul/Fady: PTAL at extensions/
> 
> I'll let Paul chime in on this one because he wrote the <webview> find API.

extensions/ LGTM!

[Lei Zhang, 2015-11-12 21:41:05]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2015-11-12 21:41:06]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/1365563003/#ps200001
(title: "parens")

[commit-bot: I haz the power, 2015-11-12 21:41:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1365563003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1365563003/200001

[commit-bot: I haz the power, 2015-11-12 23:01:42]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-11-12 23:02:38]

Patchset 11 (id:??) landed as
https://crrev.com/6057a6b26a758d3542b9d74579e4e517b49244a9
Cr-Commit-Position: refs/heads/master@{#359419}