Filter out datalist values for textarea field from renderer side.

Filter out datalist values for textarea field from renderer side.

R=isherman@chromium.org
BUG=364727
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267667

[ziran.sun, 2014-04-22 12:41:40]

Add checks on element to fix NULL pointer dereference crash. Please review.
Thanks!

[Ilya Sherman, 2014-04-22 21:08:44]

https://codereview.chromium.org/246833006/diff/1/components/autofill/content/...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/246833006/diff/1/components/autofill/content/...
components/autofill/content/renderer/autofill_agent.cc:352: return;
Why is this code reachable?

https://codereview.chromium.org/246833006/diff/1/components/autofill/content/...
components/autofill/content/renderer/autofill_agent.cc:424: return;
Why is this code reachable?

[ziran.sun, 2014-04-24 17:25:34]

On 2014/04/22 21:08:44, Ilya Sherman wrote:
>
https://codereview.chromium.org/246833006/diff/1/components/autofill/content/...
> File components/autofill/content/renderer/autofill_agent.cc (right):
> 
>
https://codereview.chromium.org/246833006/diff/1/components/autofill/content/...
> components/autofill/content/renderer/autofill_agent.cc:352: return;
> Why is this code reachable?
> 
>
https://codereview.chromium.org/246833006/diff/1/components/autofill/content/...
> components/autofill/content/renderer/autofill_agent.cc:424: return;
> Why is this code reachable?

Just added the script to repo the use case in the bug page. 

textarea picks up datalist values from any latest invoked input element as popup
suggestions. I'm still trying to finger out why.

The datalist suggestions shouldn't be used by textarea as suggestion popup in
the first place though.

[Ilya Sherman, 2014-04-24 22:50:31]

On 2014/04/24 17:25:34, ziran.sun wrote:
> Just added the script to repo the use case in the bug page. 
> 
> textarea picks up datalist values from any latest invoked input element as
popup
> suggestions. I'm still trying to finger out why.
> 
> The datalist suggestions shouldn't be used by textarea as suggestion popup in
> the first place though.

Thanks, the repro case is very helpful!  It does seem like the issue is that the
<textarea> is incorrectly picking up datalist suggestions.  Once we fix that,
the DCHECKs in AutofillAgent ought to be correct.

[ziran.sun, 2014-04-28 15:47:09]

Updated code. Please review. Thanks!

[Ilya Sherman, 2014-04-28 21:23:14]

Thanks.  This is an improvement, but we're still not quite addressing the core
issue:

https://codereview.chromium.org/246833006/diff/40001/components/autofill/core...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/246833006/diff/40001/components/autofill/core...
components/autofill/core/browser/autofill_external_delegate.cc:93:
InsertDataListValues(&values, &labels, &icons, &ids);
We should filter out datalist values on the renderer side, not on the browser
side.  We should figure out why it's currently possible for the renderer to
associate datalist values with a textarea, and then fix the code so that it's no
longer possible.

[ziran.sun, 2014-04-29 12:33:38]

Code updated as per comments. Please review. Thanks!

[Ilya Sherman, 2014-04-29 20:29:29]

https://codereview.chromium.org/246833006/diff/60001/components/autofill/cont...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/246833006/diff/60001/components/autofill/cont...
components/autofill/content/renderer/autofill_agent.cc:659: data_list_labels));
Aha!  This makes perfect sense -- we were showing stale values for the data
list.

It seems like it might make sense to just include the data list values and
labels as part of the AutofillHostMsg_QueryFormFieldAutofill message.  WDYT?

[ziran.sun, 2014-04-30 10:05:22]

https://codereview.chromium.org/246833006/diff/60001/components/autofill/cont...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/246833006/diff/60001/components/autofill/cont...
components/autofill/content/renderer/autofill_agent.cc:659: data_list_labels));
On 2014/04/29 20:29:30, Ilya Sherman wrote:
> Aha!  This makes perfect sense -- we were showing stale values for the data
> list.
> 
> It seems like it might make sense to just include the data list values and
> labels as part of the AutofillHostMsg_QueryFormFieldAutofill message.  WDYT?

It might make sense to send one message rather than two in this case. However, I
feel that this has restricted setDatalist call within queryFormFieldAutofill().
I'm thinking cases that we only want to update datalist values. The other things
is - inserting data_list_values and data_list_labels to
AutofillHostMsg_QueryFormFieldAutofill means that we also need to interact
IPC_messages to handle 7 arguments (at this moment it handles maximum 5
arguments).

Do let me know if I missed any key points though. Thanks!

[Ilya Sherman, 2014-05-01 00:53:41]

Ok, LGTM then.  Thanks very much for bearing with me and getting to the bottom
of this!  It's always nice to identify the root cause when DCHECKs fail :)

[Ilya Sherman, 2014-05-01 00:53:45]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-05-01 00:54:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/246833006/60001

[commit-bot: I haz the power, 2014-05-01 01:55:56]

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

[commit-bot: I haz the power, 2014-05-01 01:55:57]

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

[ziran.sun, 2014-05-01 09:18:06]

The CQ bit was checked by ziran.sun@samsung.com

[commit-bot: I haz the power, 2014-05-01 09:18:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/246833006/60001

[commit-bot: I haz the power, 2014-05-01 09:36:25]

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

[commit-bot: I haz the power, 2014-05-01 09:36:26]

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

[ziran.sun, 2014-05-01 09:55:31]

On 2014/05/01 00:53:41, Ilya Sherman wrote:
> Ok, LGTM then.  Thanks very much for bearing with me and getting to the bottom
> of this!  It's always nice to identify the root cause when DCHECKs fail :)

It always makes sense to find the root cause for the issue. Thank you very much
for your patience!

[Ilya Sherman, 2014-05-01 19:19:47]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-05-01 19:20:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/246833006/60001

[commit-bot: I haz the power, 2014-05-01 20:19:12]

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

[commit-bot: I haz the power, 2014-05-01 20:19:12]

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

[Ilya Sherman, 2014-05-01 21:12:17]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-05-01 21:13:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/246833006/60001

[commit-bot: I haz the power, 2014-05-01 23:39:52]

Change committed as 267667