Use an OOPIF-compatible call chain when finding the top-level frame

Kolos@ suggested engedy was more familiar with Password Manager+OOPIF.

Engedy@ can you please take a look?

---

Use an OOPIF-compatible call chain when finding the top-level frame

The ShowSuggestions autofill method must walk up the DOM hierarchy to
determine the security origin of the top-level markup. Because the
form may live in a different process than the top-level markup (due to
out-of-process iframes), we must use a method call chain that works
correctly across processes.

BUG=685982

[elawrence, 2017-01-27 17:29:19]

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

[commit-bot: I haz the power, 2017-01-27 17:29:36]

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

[elawrence, 2017-01-27 18:19:02]

elawrence@chromium.org changed reviewers:
+ mathp@chromium.org, nasko@chromium.org

[elawrence, 2017-01-27 18:19:03]

Please have a look?

[commit-bot: I haz the power, 2017-01-27 18:27:01]

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

[commit-bot: I haz the power, 2017-01-27 18:27:02]

Dry run: This issue passed the CQ dry run.

[Mathieu, 2017-01-27 18:56:00]

Description was changed from

==========
Use an OOPIF-compatible call chain when finding the top-level frame

The ShowSuggestions autofill method must walk up the DOM hierarchy to
determine the security origin of the top-level markup. Because the
form may live in a different process than the top-level markup (due to
out-of-process iframes), we must use a method call chain that works
correctly across processes.

BUG=685982
==========

to

==========
Use an OOPIF-compatible call chain when finding the top-level frame

The ShowSuggestions autofill method must walk up the DOM hierarchy to
determine the security origin of the top-level markup. Because the
form may live in a different process than the top-level markup (due to
out-of-process iframes), we must use a method call chain that works
correctly across processes.

BUG=685982
==========

[Mathieu, 2017-01-27 18:56:00]

mathp@chromium.org changed reviewers:
+ dvadym@chromium.org
- mathp@chromium.org

[Mathieu, 2017-01-27 18:56:18]

mathp@chromium.org changed reviewers:
+ mathp@chromium.org

[Mathieu, 2017-01-27 18:56:19]

+dvadym is a more proper owner for password code here.

[nasko, 2017-01-27 21:43:35]

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

https://codereview.chromium.org/2660643002/diff/1/components/autofill/content...
components/autofill/content/renderer/password_autofill_agent.cc:858:
render_frame()->GetWebFrame()->top()->getSecurityOrigin())
Can't we use getSecurityOrigin().isPotentiallyTrustworthy() here? The multiple
conversions going on are too hard to follow and might lead to subtle mistakes.

[elawrence, 2017-01-30 15:23:45]

https://codereview.chromium.org/2660643002/diff/1/components/autofill/content...
> components/autofill/content/renderer/password_autofill_agent.cc:858:
> render_frame()->GetWebFrame()->top()->getSecurityOrigin())
> Can't we use getSecurityOrigin().isPotentiallyTrustworthy() here? The multiple
> conversions going on are too hard to follow and might lead to subtle mistakes.

I can change this if you feel strongly about it. Currently
isPotentiallyTrustworthy() has an ASSERT that the protocol isn't DATA, which
means we'd need to special-case before entrance to that codepath to avoid
crashing on Data URIs. There appear to be some other corner cases (e.g. FILE:)
that we'd need to evaluate on a case-by-case basis.

Our eventual goal is to change the nature of this check so that we don't look at
the URL directly and instead query the state of the top-level Omnibox Security
Chip-- if it's Not Secure or Dangerous, we'll show the in-form warning,
otherwise we won't. That's a bigger plumbing project for a future CL.

[elawrence, 2017-01-30 15:50:27]

elawrence@chromium.org changed reviewers:
+ kolos@chromium.org

[elawrence, 2017-01-30 15:50:28]

dvadym is out until the 6th. kolos@, could you PTAL?

[nasko, 2017-01-30 20:32:54]

On 2017/01/30 15:23:45, elawrence wrote:
>
https://codereview.chromium.org/2660643002/diff/1/components/autofill/content...
> > components/autofill/content/renderer/password_autofill_agent.cc:858:
> > render_frame()->GetWebFrame()->top()->getSecurityOrigin())
> > Can't we use getSecurityOrigin().isPotentiallyTrustworthy() here? The
multiple
> > conversions going on are too hard to follow and might lead to subtle
mistakes.
> 
> I can change this if you feel strongly about it. Currently
> isPotentiallyTrustworthy() has an ASSERT that the protocol isn't DATA, which
> means we'd need to special-case before entrance to that codepath to avoid
> crashing on Data URIs. There appear to be some other corner cases (e.g. FILE:)
> that we'd need to evaluate on a case-by-case basis.
> 
> Our eventual goal is to change the nature of this check so that we don't look
at
> the URL directly and instead query the state of the top-level Omnibox Security
> Chip-- if it's Not Secure or Dangerous, we'll show the in-form warning,
> otherwise we won't. That's a bigger plumbing project for a future CL.

Let's make sure we don't crash, so LGTM on this CL. However, it will be good to
clean this up.
Also, can you file a bug for providing IsSecure() method on url::Origin itself?
Why do we need to send it to a completely different method to judge that, one
that doesn't even accept ur::Origin.

[elawrence, 2017-01-30 21:58:17]

Thanks, Nasko!

> Also, can you file a bug for providing IsSecure() method on url::Origin
itself?

Filed crbug.com/686891

[elawrence, 2017-01-31 16:13:07]

Description was changed from

==========
Use an OOPIF-compatible call chain when finding the top-level frame

The ShowSuggestions autofill method must walk up the DOM hierarchy to
determine the security origin of the top-level markup. Because the
form may live in a different process than the top-level markup (due to
out-of-process iframes), we must use a method call chain that works
correctly across processes.

BUG=685982
==========

to

==========
Kolos@ suggested engedy was more familiar with Password Manager+OOPIF.

Engedy@ can you please take a look?

---

Use an OOPIF-compatible call chain when finding the top-level frame

The ShowSuggestions autofill method must walk up the DOM hierarchy to
determine the security origin of the top-level markup. Because the
form may live in a different process than the top-level markup (due to
out-of-process iframes), we must use a method call chain that works
correctly across processes.

BUG=685982
==========

[elawrence, 2017-01-31 16:13:07]

elawrence@chromium.org changed reviewers:
+ engedy@chromium.org
- kolos@chromium.org, mathp@chromium.org

[engedy, 2017-01-31 18:15:29]

Thanks for filing the bug, and +1 on cleaning this up in the long-term. But LGTM
as a short-term fix.

[elawrence, 2017-01-31 18:21:28]

The CQ bit was checked by elawrence@chromium.org

[commit-bot: I haz the power, 2017-01-31 18:22:05]

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, 2017-01-31 18:31:56]

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

[commit-bot: I haz the power, 2017-01-31 18:31:59]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[elawrence, 2017-01-31 20:46:18]

This change landed with a TBR via a different CL:
https://codereview.chromium.org/2665993004/