[Abandoned] Add Autofill to the <webview> chrome://chrome-signin page

Add Autofill to the <webview> chrome://chrome-signin page

The change in the chrome_autofill_client.cc is needed to enable the autofill popup on
Mac.

BUG=447938
TEST=Start Chrome. Navigate to chrome://chrome-signin. Enter a correct username
with an incorrect password. After the password error appears, you should be able
to autofill that username.

Committed: https://crrev.com/4a1e13b16b5ef2cb6f7c800ad3c08f1f44bef285
Cr-Commit-Position: refs/heads/master@{#314889}

Committed: https://crrev.com/e6d3ca36ec225b0e13e385318c6c4f35fad4ca47
Cr-Commit-Position: refs/heads/master@{#316899}

[noms (inactive), 2015-02-04 20:56:21]

Patchset #3 (id:40001) has been deleted

[noms (inactive), 2015-02-04 21:47:22]

Patchset #1 (id:1) has been deleted

[noms (inactive), 2015-02-04 21:47:27]

Patchset #1 (id:20001) has been deleted

[noms (inactive), 2015-02-04 21:53:43]

noms@chromium.org changed reviewers:
+ estade@chromium.org, fsamuel@chromium.org

[noms (inactive), 2015-02-04 21:53:44]

Hiya!

I've added autofill to webview, and hopefully not entirely incorrectly. Please
take a look!

estade: c/b/ui/autofill
fsamuel: c/b/guest_view

[Evan Stade, 2015-02-04 21:57:23]

https://codereview.chromium.org/866523002/diff/80001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):

https://codereview.chromium.org/866523002/diff/80001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/chrome_autofill_client.cc:193:
web_contents()->GetContentNativeView(),
I'm not familiar with the differences between GetNativeView and
GetContentNativeView, but I assume you tested this on several platforms?

[noms (inactive), 2015-02-04 21:59:44]

On 2015/02/04 21:57:23, Evan Stade wrote:
>
https://codereview.chromium.org/866523002/diff/80001/chrome/browser/ui/autofi...
> File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):
> 
>
https://codereview.chromium.org/866523002/diff/80001/chrome/browser/ui/autofi...
> chrome/browser/ui/autofill/chrome_autofill_client.cc:193:
> web_contents()->GetContentNativeView(),
> I'm not familiar with the differences between GetNativeView and
> GetContentNativeView, but I assume you tested this on several platforms?

I've only tested it on Desktop platforms. The problem is really just on Mac, so
I could if'def the change to only happen
for Mac. Actually, if that's ok with you, that might make me *way* more
comfortable.

[noms (inactive), 2015-02-04 21:59:46]

On 2015/02/04 21:57:23, Evan Stade wrote:
>
https://codereview.chromium.org/866523002/diff/80001/chrome/browser/ui/autofi...
> File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):
> 
>
https://codereview.chromium.org/866523002/diff/80001/chrome/browser/ui/autofi...
> chrome/browser/ui/autofill/chrome_autofill_client.cc:193:
> web_contents()->GetContentNativeView(),
> I'm not familiar with the differences between GetNativeView and
> GetContentNativeView, but I assume you tested this on several platforms?

I've only tested it on Desktop platforms. The problem is really just on Mac, so
I could if'def the change to only happen
for Mac. Actually, if that's ok with you, that might make me *way* more
comfortable.

[Evan Stade, 2015-02-04 22:00:41]

On 2015/02/04 21:59:46, Monica Dinculescu wrote:
> On 2015/02/04 21:57:23, Evan Stade wrote:
> >
>
https://codereview.chromium.org/866523002/diff/80001/chrome/browser/ui/autofi...
> > File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):
> > 
> >
>
https://codereview.chromium.org/866523002/diff/80001/chrome/browser/ui/autofi...
> > chrome/browser/ui/autofill/chrome_autofill_client.cc:193:
> > web_contents()->GetContentNativeView(),
> > I'm not familiar with the differences between GetNativeView and
> > GetContentNativeView, but I assume you tested this on several platforms?
> 
> I've only tested it on Desktop platforms. The problem is really just on Mac,
so
> I could if'def the change to only happen
> for Mac. Actually, if that's ok with you, that might make me *way* more
> comfortable.

I'd rather understand what's going on and know for sure if the ifdef is
necessary

[noms (inactive), 2015-02-04 22:11:16]

On 2015/02/04 22:00:41, Evan Stade wrote:
> On 2015/02/04 21:59:46, Monica Dinculescu wrote:
> > On 2015/02/04 21:57:23, Evan Stade wrote:
> > >
> >
>
https://codereview.chromium.org/866523002/diff/80001/chrome/browser/ui/autofi...
> > > File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/866523002/diff/80001/chrome/browser/ui/autofi...
> > > chrome/browser/ui/autofill/chrome_autofill_client.cc:193:
> > > web_contents()->GetContentNativeView(),
> > > I'm not familiar with the differences between GetNativeView and
> > > GetContentNativeView, but I assume you tested this on several platforms?
> > 
> > I've only tested it on Desktop platforms. The problem is really just on Mac,
> so
> > I could if'def the change to only happen
> > for Mac. Actually, if that's ok with you, that might make me *way* more
> > comfortable.
> 
> I'd rather understand what's going on and know for sure if the ifdef is
> necessary

The way lazyboy@ explained it (he figured out the fix) is that
GetContentNativeView will initialize the 
PopupController with the embedder's native view (i.e. the webview's native
view). Without the change, 
on Mac, the autofill popup doesn't show up at all. I know next to nothing about
autofill or webview,
so maybe Fady or Istiaque can answer if this is a quirk of webview on mac, or of
native views in general.
:(

[Fady Samuel, 2015-02-04 22:55:29]

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

[Fady Samuel, 2015-02-04 22:55:30]

This is a bit outside of my understanding and I'm in a weird timezone at the
moment (Sydney) so I'll pass this on to Istiaque. Thanks!

[noms (inactive), 2015-02-05 00:13:00]

Patchset #3 (id:100001) has been deleted

[noms (inactive), 2015-02-05 00:13:35]

On 2015/02/04 22:00:41, Evan Stade wrote:
> On 2015/02/04 21:59:46, Monica Dinculescu wrote:
> > On 2015/02/04 21:57:23, Evan Stade wrote:
> > >
> >
>
https://codereview.chromium.org/866523002/diff/80001/chrome/browser/ui/autofi...
> > > File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/866523002/diff/80001/chrome/browser/ui/autofi...
> > > chrome/browser/ui/autofill/chrome_autofill_client.cc:193:
> > > web_contents()->GetContentNativeView(),
> > > I'm not familiar with the differences between GetNativeView and
> > > GetContentNativeView, but I assume you tested this on several platforms?
> > 
> > I've only tested it on Desktop platforms. The problem is really just on Mac,
> so
> > I could if'def the change to only happen
> > for Mac. Actually, if that's ok with you, that might make me *way* more
> > comfortable.
> 
> I'd rather understand what's going on and know for sure if the ifdef is
> necessary

So the problem is that <webview> doesn't have actual an platform
window/NativeViews, so it relies on the embedder's one for 
certain things, in particular, mac popups. I've changed the code so that we only
do something different if the webcontents
is inside the webview.

[lazyboy, 2015-02-05 00:39:50]

The WebViewGuest* changes LGTM.
Narrowing down the container_view() override for <webview> looks fine too.

https://chromiumcodereview.appspot.com/866523002/diff/120001/chrome/browser/u...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):

https://chromiumcodereview.appspot.com/866523002/diff/120001/chrome/browser/u...
chrome/browser/ui/autofill/chrome_autofill_client.cc:189: // If the webcontents
is inside a webview, use the embedder's native view,
s/webcontents/WebContents
s/webview/<webview>

[lazyboy, 2015-02-05 00:43:56]

FYI

https://chromiumcodereview.appspot.com/866523002/diff/120001/chrome/browser/u...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):

https://chromiumcodereview.appspot.com/866523002/diff/120001/chrome/browser/u...
chrome/browser/ui/autofill/chrome_autofill_client.cc:192: if
(extensions::WebViewGuest::FromWebContents(web_contents()))
#if defined(ENABLE_EXTENSIONS) this call and the header include.

[noms (inactive), 2015-02-05 00:51:52]

New patchsets have been uploaded after l-g-t-m from lazyboy@chromium.org

[noms (inactive), 2015-02-05 02:50:33]

https://codereview.chromium.org/866523002/diff/120001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):

https://codereview.chromium.org/866523002/diff/120001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/chrome_autofill_client.cc:189: // If the webcontents
is inside a webview, use the embedder's native view,
On 2015/02/05 00:39:50, lazyboy wrote:
> s/webcontents/WebContents
> s/webview/<webview>

Done.

https://codereview.chromium.org/866523002/diff/120001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/chrome_autofill_client.cc:192: if
(extensions::WebViewGuest::FromWebContents(web_contents()))
On 2015/02/05 00:43:56, lazyboy wrote:
> #if defined(ENABLE_EXTENSIONS) this call and the header include.

Yup, just noticed that. Done.

[Evan Stade, 2015-02-05 03:12:56]

https://codereview.chromium.org/866523002/diff/140001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (left):

https://codereview.chromium.org/866523002/diff/140001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/chrome_autofill_client.cc:193:
web_contents()->GetNativeView(),
when I look at what this view is actually used for, it seems like
GetContentNativeView() should be fine (I can only see it being used for
gfx::Screen::GetScreenFor()). You can just go ahead and change to
GetContentNativeView(), provided you do a 2-second sanity check of normal web
autofill on mac and non-mac.

[noms (inactive), 2015-02-05 18:56:08]

https://codereview.chromium.org/866523002/diff/140001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (left):

https://codereview.chromium.org/866523002/diff/140001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/chrome_autofill_client.cc:193:
web_contents()->GetNativeView(),
On 2015/02/05 03:12:56, Evan Stade wrote:
> when I look at what this view is actually used for, it seems like
> GetContentNativeView() should be fine (I can only see it being used for
> gfx::Screen::GetScreenFor()). You can just go ahead and change to
> GetContentNativeView(), provided you do a 2-second sanity check of normal web
> autofill on mac and non-mac.

Done. I've tested it on all desktop platforms, and it looks good!

[Evan Stade, 2015-02-05 21:44:08]

autofill lgtm

[Fady Samuel, 2015-02-05 21:50:52]

lgtm

[noms (inactive), 2015-02-05 21:51:51]

Woot. Thanks!

[noms (inactive), 2015-02-05 21:51:55]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2015-02-05 21:53:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/866523002/160001

[commit-bot: I haz the power, 2015-02-05 21:57:08]

Committed patchset #5 (id:160001)

[commit-bot: I haz the power, 2015-02-05 21:58:40]

Patchset 5 (id:??) landed as
https://crrev.com/4a1e13b16b5ef2cb6f7c800ad3c08f1f44bef285
Cr-Commit-Position: refs/heads/master@{#314889}

[Vitaly Buka (NO REVIEWS), 2015-02-06 22:28:39]

A revert of this CL (patchset #5 id:160001) has been created in
https://codereview.chromium.org/887003007/ by vitalybuka@chromium.org.

The reason for reverting is: BUG=456215.

[noms (inactive), 2015-02-11 20:27:55]

New patchsets have been uploaded after l-g-t-m from
estade@chromium.org,fsamuel@chromium.org

[noms (inactive), 2015-02-11 21:34:37]

Patchset #7 (id:200001) has been deleted

[noms (inactive), 2015-02-11 21:36:42]

noms@chromium.org changed reviewers:
+ vabr@chromium.org

[noms (inactive), 2015-02-11 21:36:43]

Hi Fady and Vaclav,

I think I've fixed the crash (needed to throw more factories at the webview).
Please take another look. Thanks!

[vabr (Chromium), 2015-02-12 14:40:30]

On 2015/02/11 21:36:43, Monica Dinculescu wrote:
> Hi Fady and Vaclav,
> 
> I think I've fixed the crash (needed to throw more factories at the webview).
> Please take another look. Thanks!

Hi Monica,

I looked at the diff from patch set #5, which says the difference is adding
ManagePasswordsUIController::CreateForWebContents(contents);

I'm concerned about how the password manager is used inside webviews. How does
the UI cope with the fact that the Omnibox is missing? Does it provide the user
with enough context about the page using the password? Could the app somehow
gain access to the stored passwords for pages displayed in the webview?

Adding password manager to the webview sounds like a rather serious change, and
it would be good to have a design for that for review.

Cheers,
Vaclav

P.S. I'm going OOO soon, to grab the team mailing-list just
goto/chrome-password-manager/team.

[Fady Samuel, 2015-02-12 14:48:57]

On 2015/02/12 14:40:30, vabr (Chromium) wrote:
> On 2015/02/11 21:36:43, Monica Dinculescu wrote:
> > Hi Fady and Vaclav,
> > 
> > I think I've fixed the crash (needed to throw more factories at the
webview).
> > Please take another look. Thanks!
> 
> Hi Monica,
> 
> I looked at the diff from patch set #5, which says the difference is adding
> ManagePasswordsUIController::CreateForWebContents(contents);
> 
> I'm concerned about how the password manager is used inside webviews. How does
> the UI cope with the fact that the Omnibox is missing? Does it provide the
user
> with enough context about the page using the password? Could the app somehow
> gain access to the stored passwords for pages displayed in the webview?
> 
> Adding password manager to the webview sounds like a rather serious change,
and
> it would be good to have a design for that for review.
> 
> Cheers,
> Vaclav
> 
> P.S. I'm going OOO soon, to grab the team mailing-list just
> goto/chrome-password-manager/team.

The app is already assumed to be more privileged than the content of the webview
and can inject arbitrary script and intercept HTTP requests and responses. I
think the general issue with auto fill in general is it exposes information from
the browser to potentially a Chrome App. I would consider only installing all
these auto fill helpers in WebUI.

[noms (inactive), 2015-02-12 18:01:36]

no autofill in extensions/apps

[noms (inactive), 2015-02-12 18:03:42]

Patchset #9 (id:260001) has been deleted

[noms (inactive), 2015-02-12 18:05:55]

Oh, TIL "git cl upload -m" as a typo sends an email. Sorry for the spam.

Reverted the PasswordManagerController change, and instead disabled autofill for
non-chrome extensions (so it works in chrome://signin but not on
external extensions). We do this a lot for webview (keyboard accelerators), so
this seems ok. 

On 2015/02/12 18:01:36, Monica Dinculescu wrote:
> no autofill in extensions/apps

[Fady Samuel, 2015-02-12 19:16:27]

https://codereview.chromium.org/866523002/diff/280001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc
(right):

https://codereview.chromium.org/866523002/diff/280001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:83: //
<webview> outside of Chrome Apps does not handle autofill.
This should say that <webview> in Chrome Apps and Extensions does not handle
autofill.

[noms (inactive), 2015-02-12 19:55:28]

https://codereview.chromium.org/866523002/diff/280001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc
(right):

https://codereview.chromium.org/866523002/diff/280001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:83: //
<webview> outside of Chrome Apps does not handle autofill.
On 2015/02/12 19:16:26, Fady Samuel wrote:
> This should say that <webview> in Chrome Apps and Extensions does not handle
> autofill.

Done.

[vabr (Chromium), 2015-02-12 20:03:45]

Thanks, Monica.

Looking at patch set 9, I understand that password manager cannot execute at all
in a webview owned by an app or extension, which is an improvement.

But it sounds very fragile. Currently, you are embedding only half-working
version of password autofill in webview (can fill, crashes on save).
If you need password autofill (p.a.), then this is risky -- whenever p.a. can
fill, there it can also save, and thus crash.
If you do not need p.a., then it would still be good to know, if there is
something in the dependencies between autofill and password autofill, which
could be fixed, so that it is not necessary to wire in an unusable version of
password autofill to use autofill.

Cheers,
Vaclav

[Fady Samuel, 2015-02-12 20:08:07]

On 2015/02/12 20:03:45, vabr (Chromium) wrote:
> Thanks, Monica.
> 
> Looking at patch set 9, I understand that password manager cannot execute at
all
> in a webview owned by an app or extension, which is an improvement.
> 
> But it sounds very fragile. Currently, you are embedding only half-working
> version of password autofill in webview (can fill, crashes on save).
> If you need password autofill (p.a.), then this is risky -- whenever p.a. can
> fill, there it can also save, and thus crash.
> If you do not need p.a., then it would still be good to know, if there is
> something in the dependencies between autofill and password autofill, which
> could be fixed, so that it is not necessary to wire in an unusable version of
> password autofill to use autofill.
> 
> Cheers,
> Vaclav

I don't see why we shouldn't support saving in WebUI?

[jww, 2015-02-12 20:24:21]

On 2015/02/12 20:08:07, Fady Samuel wrote:
> On 2015/02/12 20:03:45, vabr (Chromium) wrote:
> > Thanks, Monica.
> > 
> > Looking at patch set 9, I understand that password manager cannot execute at
> all
> > in a webview owned by an app or extension, which is an improvement.
> > 
> > But it sounds very fragile. Currently, you are embedding only half-working
> > version of password autofill in webview (can fill, crashes on save).
> > If you need password autofill (p.a.), then this is risky -- whenever p.a.
can
> > fill, there it can also save, and thus crash.
> > If you do not need p.a., then it would still be good to know, if there is
> > something in the dependencies between autofill and password autofill, which
> > could be fixed, so that it is not necessary to wire in an unusable version
of
> > password autofill to use autofill.
> > 
> > Cheers,
> > Vaclav
> 
> I don't see why we shouldn't support saving in WebUI?

Hi everyone. This seems like a rather large change, and as vabr@ points out,
with some potentially hairy and brittle consequences. I certainly haven't
thought about this at all, but has this undergone security review or any other
launch-level reviews? I would argue that https://crbug.com/447938, which is
listed as the bug for this CL, is not sufficient and this feature really needs a
full scale launch bug and launch review. Thanks!

[vabr (Chromium), 2015-02-12 20:26:18]

On 2015/02/12 20:08:07, Fady Samuel wrote:
> On 2015/02/12 20:03:45, vabr (Chromium) wrote:
> > Thanks, Monica.
> > 
> > Looking at patch set 9, I understand that password manager cannot execute at
> all
> > in a webview owned by an app or extension, which is an improvement.
> > 
> > But it sounds very fragile. Currently, you are embedding only half-working
> > version of password autofill in webview (can fill, crashes on save).
> > If you need password autofill (p.a.), then this is risky -- whenever p.a.
can
> > fill, there it can also save, and thus crash.
> > If you do not need p.a., then it would still be good to know, if there is
> > something in the dependencies between autofill and password autofill, which
> > could be fixed, so that it is not necessary to wire in an unusable version
of
> > password autofill to use autofill.
> > 
> > Cheers,
> > Vaclav
> 
> I don't see why we shouldn't support saving in WebUI?

Currently, the code won't save them, if I understand correctly, because it will
crash trying to display the bubble.
If that is fixed, and the plan is to save passwords in WebUI, my next question
is which passwords and why.
If this is about, e.g., the sync password, then password manager won't save that
particular anyway (it does not do it on the web either), for security reasons (a
temporary access to user's saved passwords should not grant permanent access
through sync).
I do not know other cases of passwords in the WebUI, but they could be similarly
sensitive and require case-by-case consideration.

[noms (inactive), 2015-02-13 18:07:07]

Hmm, let me try to clear some things up that might answer everyone's concerns:
- in its current state, this CL only enables autofill for non apps and
extensions, so there's
no concern of external apps interacting with credentials or the password manager
- the only thing in Chrome currently using this is the Sign in page 
(chrome://chrome-signin) which does not even attempt to save the password, so
there
is no crash
- this is why I think this isn't a new feature, and it is a fix for the linked
bug, which is
"autofill is borked on the chrome://chrome-signin" page.

I can add extra checks to check the webcontents URL and make sure it's literally
only enabled for the sign-in page, but that might be a bit of overkill.

[noms (inactive), 2015-02-16 18:58:06]

On 2015/02/13 18:07:07, Monica Dinculescu wrote:
> Hmm, let me try to clear some things up that might answer everyone's concerns:
> - in its current state, this CL only enables autofill for non apps and
> extensions, so there's
> no concern of external apps interacting with credentials or the password
manager
> - the only thing in Chrome currently using this is the Sign in page 
> (chrome://chrome-signin) which does not even attempt to save the password, so
> there
> is no crash
> - this is why I think this isn't a new feature, and it is a fix for the linked
> bug, which is
> "autofill is borked on the chrome://chrome-signin" page.
> 
> I can add extra checks to check the webcontents URL and make sure it's
literally
> only enabled for the sign-in page, but that might be a bit of overkill.

Joel/Fady, does the above sound ok? Is this CL ready to land (it's a stable
blocker, 
so if it's not ready to land as-is, then we need to look at alternatives)

[Fady Samuel, 2015-02-16 19:01:58]

On 2015/02/16 18:58:06, Monica Dinculescu wrote:
> On 2015/02/13 18:07:07, Monica Dinculescu wrote:
> > Hmm, let me try to clear some things up that might answer everyone's
concerns:
> > - in its current state, this CL only enables autofill for non apps and
> > extensions, so there's
> > no concern of external apps interacting with credentials or the password
> manager
> > - the only thing in Chrome currently using this is the Sign in page 
> > (chrome://chrome-signin) which does not even attempt to save the password,
so
> > there
> > is no crash
> > - this is why I think this isn't a new feature, and it is a fix for the
linked
> > bug, which is
> > "autofill is borked on the chrome://chrome-signin" page.
> > 
> > I can add extra checks to check the webcontents URL and make sure it's
> literally
> > only enabled for the sign-in page, but that might be a bit of overkill.
> 
> Joel/Fady, does the above sound ok? Is this CL ready to land (it's a stable
> blocker, 
> so if it's not ready to land as-is, then we need to look at alternatives)

I'm happy with the patch as it is. I don't think it's introducing anything that
wasn't already possible.

[jww, 2015-02-17 19:17:26]

On 2015/02/16 19:01:58, Fady Samuel wrote:
> On 2015/02/16 18:58:06, Monica Dinculescu wrote:
> > On 2015/02/13 18:07:07, Monica Dinculescu wrote:
> > > Hmm, let me try to clear some things up that might answer everyone's
> concerns:
> > > - in its current state, this CL only enables autofill for non apps and
> > > extensions, so there's
> > > no concern of external apps interacting with credentials or the password
> > manager
> > > - the only thing in Chrome currently using this is the Sign in page 
> > > (chrome://chrome-signin) which does not even attempt to save the password,
> so
> > > there
> > > is no crash
> > > - this is why I think this isn't a new feature, and it is a fix for the
> linked
> > > bug, which is
> > > "autofill is borked on the chrome://chrome-signin" page.
> > > 
> > > I can add extra checks to check the webcontents URL and make sure it's
> > literally
> > > only enabled for the sign-in page, but that might be a bit of overkill.
> > 
> > Joel/Fady, does the above sound ok? Is this CL ready to land (it's a stable
> > blocker, 
> > so if it's not ready to land as-is, then we need to look at alternatives)
> 
> I'm happy with the patch as it is. I don't think it's introducing anything
that
> wasn't already possible.

I would be much happier if there was a DCHECK in chrome_autofill_client.cc
verifying that this is only occurring in the signin page. I think anything
broader than that will need heavier consideration. Otherwise, lgtm if vabr@
thinks it's cool. Thanks!

[jww, 2015-02-17 19:27:32]

jww@chromium.org changed reviewers:
+ engedy@chromium.org

[jww, 2015-02-17 19:27:43]

On 2015/02/17 19:17:26, jww wrote:
> On 2015/02/16 19:01:58, Fady Samuel wrote:
> > On 2015/02/16 18:58:06, Monica Dinculescu wrote:
> > > On 2015/02/13 18:07:07, Monica Dinculescu wrote:
> > > > Hmm, let me try to clear some things up that might answer everyone's
> > concerns:
> > > > - in its current state, this CL only enables autofill for non apps and
> > > > extensions, so there's
> > > > no concern of external apps interacting with credentials or the password
> > > manager
> > > > - the only thing in Chrome currently using this is the Sign in page 
> > > > (chrome://chrome-signin) which does not even attempt to save the
password,
> > so
> > > > there
> > > > is no crash
> > > > - this is why I think this isn't a new feature, and it is a fix for the
> > linked
> > > > bug, which is
> > > > "autofill is borked on the chrome://chrome-signin" page.
> > > > 
> > > > I can add extra checks to check the webcontents URL and make sure it's
> > > literally
> > > > only enabled for the sign-in page, but that might be a bit of overkill.
> > > 
> > > Joel/Fady, does the above sound ok? Is this CL ready to land (it's a
stable
> > > blocker, 
> > > so if it's not ready to land as-is, then we need to look at alternatives)
> > 
> > I'm happy with the patch as it is. I don't think it's introducing anything
> that
> > wasn't already possible.
> 
> I would be much happier if there was a DCHECK in chrome_autofill_client.cc
> verifying that this is only occurring in the signin page. I think anything
> broader than that will need heavier consideration. Otherwise, lgtm if vabr@
> thinks it's cool. Thanks!

vabr@ is on vacation, so he won't be checking anything :-) engedy, can you take
a look at this one last time? Thanks!

[jww, 2015-02-17 19:51:53]

On 2015/02/17 19:27:43, jww wrote:
> On 2015/02/17 19:17:26, jww wrote:
> > On 2015/02/16 19:01:58, Fady Samuel wrote:
> > > On 2015/02/16 18:58:06, Monica Dinculescu wrote:
> > > > On 2015/02/13 18:07:07, Monica Dinculescu wrote:
> > > > > Hmm, let me try to clear some things up that might answer everyone's
> > > concerns:
> > > > > - in its current state, this CL only enables autofill for non apps and
> > > > > extensions, so there's
> > > > > no concern of external apps interacting with credentials or the
password
> > > > manager
> > > > > - the only thing in Chrome currently using this is the Sign in page 
> > > > > (chrome://chrome-signin) which does not even attempt to save the
> password,
> > > so
> > > > > there
> > > > > is no crash
> > > > > - this is why I think this isn't a new feature, and it is a fix for
the
> > > linked
> > > > > bug, which is
> > > > > "autofill is borked on the chrome://chrome-signin" page.
> > > > > 
> > > > > I can add extra checks to check the webcontents URL and make sure it's
> > > > literally
> > > > > only enabled for the sign-in page, but that might be a bit of
overkill.
> > > > 
> > > > Joel/Fady, does the above sound ok? Is this CL ready to land (it's a
> stable
> > > > blocker, 
> > > > so if it's not ready to land as-is, then we need to look at
alternatives)
> > > 
> > > I'm happy with the patch as it is. I don't think it's introducing anything
> > that
> > > wasn't already possible.
> > 
> > I would be much happier if there was a DCHECK in chrome_autofill_client.cc
> > verifying that this is only occurring in the signin page. I think anything
> > broader than that will need heavier consideration. Otherwise, lgtm if vabr@
> > thinks it's cool. Thanks!
> 
> vabr@ is on vacation, so he won't be checking anything :-) engedy, can you
take
> a look at this one last time? Thanks!

...and I meant that I'd like to see a DCHECK in chrome_web_view_guest_delegate
(not chrome_autofill_client). Sorry for the confusion!

[noms (inactive), 2015-02-17 20:15:11]

New patchsets have been uploaded after l-g-t-m from jww@chromium.org

[noms (inactive), 2015-02-17 20:15:57]

DCHECK has been added!

[engedy, 2015-02-18 13:34:27]

LGTM % comments. 

Joel, one question also for you.

https://codereview.chromium.org/866523002/diff/320001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc
(right):

https://codereview.chromium.org/866523002/diff/320001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:90:
DCHECK(web_view_guest_->GetOwnerSiteURL().GetOrigin().spec() ==
@Joel, given that there is security implications of this check failing, should
we make it a CHECK(), as opposed to a DCHECK()? Or simply return at this point
if the condition is not met?

https://codereview.chromium.org/866523002/diff/320001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:94:
ChromePasswordManagerClient::CreateForWebContentsWithAutofillClient(
Could you please add a TODO that this will need to be revised later? I share
Vaclav's concern that currently the Password Manager does support actually
support this use-case, hence the solution is very fragile.

[engedy, 2015-02-18 13:35:28]

https://codereview.chromium.org/866523002/diff/320001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc
(right):

https://codereview.chromium.org/866523002/diff/320001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:94:
ChromePasswordManagerClient::CreateForWebContentsWithAutofillClient(
On 2015/02/18 13:34:27, engedy wrote:
> Could you please add a TODO that this will need to be revised later? I share
> Vaclav's concern that currently the Password Manager does support actually
> support this use-case, hence the solution is very fragile.

... does not support ...

[Fady Samuel, 2015-02-18 14:08:20]

https://codereview.chromium.org/866523002/diff/320001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc
(right):

https://codereview.chromium.org/866523002/diff/320001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:90:
DCHECK(web_view_guest_->GetOwnerSiteURL().GetOrigin().spec() ==
On 2015/02/18 13:34:27, engedy wrote:
> @Joel, given that there is security implications of this check failing, should
> we make it a CHECK(), as opposed to a DCHECK()? Or simply return at this point
> if the condition is not met?

I have another maybe insignificant concern: this is doing a string comparison,
is it preferred to do a GURL comparison?

So:

DCHECK(web_view_guest_->GetOwnerSiteURL().GetOrigin() ==
GURL(chrome::kChromeUIChromeSigninURL));

?

[Fady Samuel, 2015-02-18 14:09:41]

On 2015/02/18 14:08:20, Fady Samuel wrote:
>
https://codereview.chromium.org/866523002/diff/320001/chrome/browser/guest_vi...
> File chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc
> (right):
> 
>
https://codereview.chromium.org/866523002/diff/320001/chrome/browser/guest_vi...
> chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:90:
> DCHECK(web_view_guest_->GetOwnerSiteURL().GetOrigin().spec() ==
> On 2015/02/18 13:34:27, engedy wrote:
> > @Joel, given that there is security implications of this check failing,
should
> > we make it a CHECK(), as opposed to a DCHECK()? Or simply return at this
point
> > if the condition is not met?
> 
> I have another maybe insignificant concern: this is doing a string comparison,
> is it preferred to do a GURL comparison?
> 
> So:
> 
> DCHECK(web_view_guest_->GetOwnerSiteURL().GetOrigin() ==
> GURL(chrome::kChromeUIChromeSigninURL));
> 
> ?

I just realized that this effectively does the same thing.

[engedy, 2015-02-18 14:17:10]

On 2015/02/18 14:09:41, Fady Samuel wrote:
> On 2015/02/18 14:08:20, Fady Samuel wrote:
> >
>
https://codereview.chromium.org/866523002/diff/320001/chrome/browser/guest_vi...
> > File chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/866523002/diff/320001/chrome/browser/guest_vi...
> > chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:90:
> > DCHECK(web_view_guest_->GetOwnerSiteURL().GetOrigin().spec() ==
> > On 2015/02/18 13:34:27, engedy wrote:
> > > @Joel, given that there is security implications of this check failing,
> should
> > > we make it a CHECK(), as opposed to a DCHECK()? Or simply return at this
> point
> > > if the condition is not met?
> > 
> > I have another maybe insignificant concern: this is doing a string
comparison,
> > is it preferred to do a GURL comparison?
> > 
> > So:
> > 
> > DCHECK(web_view_guest_->GetOwnerSiteURL().GetOrigin() ==
> > GURL(chrome::kChromeUIChromeSigninURL));
> > 
> > ?
> 
> I just realized that this effectively does the same thing.

Also, could you please make sure to do testing with the
#enable-save-password-bubble flag too, so as to make sure there are no crashes
there either?

[noms (inactive), 2015-02-18 18:29:08]

New patchsets have been uploaded after l-g-t-m from engedy@chromium.org

[noms (inactive), 2015-02-18 18:33:13]

Changed the DCHECK into an early return. Also, I've tested with the bubble flag
-- a "save password" bubble is never shown on the signin page, and it doesn't
now either.

https://codereview.chromium.org/866523002/diff/320001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc
(right):

https://codereview.chromium.org/866523002/diff/320001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:90:
DCHECK(web_view_guest_->GetOwnerSiteURL().GetOrigin().spec() ==
On 2015/02/18 13:34:27, engedy wrote:
> @Joel, given that there is security implications of this check failing, should
> we make it a CHECK(), as opposed to a DCHECK()? Or simply return at this point
> if the condition is not met?

I've made it a hard return (this was my original suggestion, so I'm happy with
it)

https://codereview.chromium.org/866523002/diff/320001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:94:
ChromePasswordManagerClient::CreateForWebContentsWithAutofillClient(
On 2015/02/18 13:34:27, engedy wrote:
> Could you please add a TODO that this will need to be revised later? I share
> Vaclav's concern that currently the Password Manager does support actually
> support this use-case, hence the solution is very fragile.

Done.

[jww, 2015-02-18 20:08:44]

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

[jww, 2015-02-18 20:08:45]

https://codereview.chromium.org/866523002/diff/320001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc
(right):

https://codereview.chromium.org/866523002/diff/320001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:90:
DCHECK(web_view_guest_->GetOwnerSiteURL().GetOrigin().spec() ==
On 2015/02/18 18:33:12, Monica Dinculescu wrote:
> On 2015/02/18 13:34:27, engedy wrote:
> > @Joel, given that there is security implications of this check failing,
should
> > we make it a CHECK(), as opposed to a DCHECK()? Or simply return at this
point
> > if the condition is not met?
> 
> I've made it a hard return (this was my original suggestion, so I'm happy with
> it)

I think engedy is right, it should be a CHECK(), not a DCHECK, and I don't think
a hard return is the right behavior. If it hits this point, something has gone
terribly wrong in our code, so we should abort, and since it's a security issue,
it should be a run time failure.

I think the string comparison is fine since it's just a straight equality check
and we do care about the path.

[noms (inactive), 2015-02-18 21:04:03]

I've also updated the CL description to match the contents of the CL

https://codereview.chromium.org/866523002/diff/320001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc
(right):

https://codereview.chromium.org/866523002/diff/320001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:90:
DCHECK(web_view_guest_->GetOwnerSiteURL().GetOrigin().spec() ==
On 2015/02/18 20:08:45, jww wrote:
> On 2015/02/18 18:33:12, Monica Dinculescu wrote:
> > On 2015/02/18 13:34:27, engedy wrote:
> > > @Joel, given that there is security implications of this check failing,
> should
> > > we make it a CHECK(), as opposed to a DCHECK()? Or simply return at this
> point
> > > if the condition is not met?
> > 
> > I've made it a hard return (this was my original suggestion, so I'm happy
with
> > it)
> 
> I think engedy is right, it should be a CHECK(), not a DCHECK, and I don't
think
> a hard return is the right behavior. If it hits this point, something has gone
> terribly wrong in our code, so we should abort, and since it's a security
issue,
> it should be a run time failure.
> 
> I think the string comparison is fine since it's just a straight equality
check
> and we do care about the path.

I don't think I understand how this is a security issue. A CHECK would mean that
only Chrome signin can use webview, which I think is too strict. The original
concern was that we were adding autofill to all of <webview>, which should go
through a feature launch. Adding autofill to *just* the signin page was ok,
which is exactly what this CL is doing.

[jww, 2015-02-18 21:17:28]

On 2015/02/18 21:04:03, Monica Dinculescu wrote:
> I've also updated the CL description to match the contents of the CL
> 
>
https://codereview.chromium.org/866523002/diff/320001/chrome/browser/guest_vi...
> File chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc
> (right):
> 
>
https://codereview.chromium.org/866523002/diff/320001/chrome/browser/guest_vi...
> chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:90:
> DCHECK(web_view_guest_->GetOwnerSiteURL().GetOrigin().spec() ==
> On 2015/02/18 20:08:45, jww wrote:
> > On 2015/02/18 18:33:12, Monica Dinculescu wrote:
> > > On 2015/02/18 13:34:27, engedy wrote:
> > > > @Joel, given that there is security implications of this check failing,
> > should
> > > > we make it a CHECK(), as opposed to a DCHECK()? Or simply return at this
> > point
> > > > if the condition is not met?
> > > 
> > > I've made it a hard return (this was my original suggestion, so I'm happy
> with
> > > it)
> > 
> > I think engedy is right, it should be a CHECK(), not a DCHECK, and I don't
> think
> > a hard return is the right behavior. If it hits this point, something has
gone
> > terribly wrong in our code, so we should abort, and since it's a security
> issue,
> > it should be a run time failure.
> > 
> > I think the string comparison is fine since it's just a straight equality
> check
> > and we do care about the path.
> 
> I don't think I understand how this is a security issue. A CHECK would mean
that
> only Chrome signin can use webview, which I think is too strict. The original
> concern was that we were adding autofill to all of <webview>, which should go
> through a feature launch. Adding autofill to *just* the signin page was ok,
> which is exactly what this CL is doing.

Yup, agreed. Thanks for the clarification. LGTM (again :-)

[noms (inactive), 2015-02-18 21:21:09]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2015-02-18 21:21:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/866523002/340001

[commit-bot: I haz the power, 2015-02-18 21:49:27]

Committed patchset #12 (id:340001)

[commit-bot: I haz the power, 2015-02-18 21:50:14]

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

[noms (inactive), 2015-02-19 19:39:00]

A revert of this CL (patchset #12 id:340001) has been created in
https://codereview.chromium.org/944513002/ by noms@chromium.org.

The reason for reverting is: Introduced crashes in
ManagePasswordsUIController::OnPasswordSubmitted:(

https://crash.corp.google.com/browse?q=product.name%3D%27Chrome%27%20AND%20pr....

[engedy, 2015-02-20 15:39:28]

engedy@chromium.org changed reviewers:
+ mkwst@chromium.org

[engedy, 2015-02-20 15:39:29]

(+mkwst)

[engedy, 2015-02-20 17:29:20]

@Fady, in order to give Mike and me some more context, could you please briefly
elaborate on how exactly the WebView is utilized here? Is the
accounts.google.com page (or, in case of SAML SSO, the third party login page)
opened directly inside the WebView? How long does the WebView live, and when do
we navigate away from the top-top-level page?

[noms (inactive), 2015-02-20 18:41:27]

On 2015/02/20 17:29:20, engedy wrote:
> @Fady, in order to give Mike and me some more context, could you please
briefly
> elaborate on how exactly the WebView is utilized here? Is the
> http://accounts.google.com page (or, in case of SAML SSO, the third party
login page)
> opened directly inside the WebView? How long does the WebView live, and when
do
> we navigate away from the top-top-level page?

My understanding is:
- you navigate to chrome://chrome-signin, which is a webview page that hosts the

accounts.google.com signin page
- the signin process either does a whole bunch of accounts.google.com redirects
(or, in the case of SAML, redirect to a third party page). This is still inside
the 
webview
- when signin ends, the tab is reused and we open the NTP (and the webview dies)

[engedy, 2015-02-20 19:55:49]

In that case, we can no longer rely on scheme-based suppression of the password
manager UI.

We could, as a very hacky temporary fix for M42, pass a flag into
ChromePasswordManagerClient at construction time to let it know that it is
inside a WebView (or let it figure that out for itself somehow). Mike would be
able to comment on how reliably we could prevent all UI calls.

In the long run, however, we should certainly to decouple Autofill from Password
Autofill so that we do not need to force the latter into a WebView. We should
also consider whether or not autofilling the username in M42 is really worth the
substantially higher risk of crashing.

[noms (inactive), 2015-02-23 15:03:05]

On 2015/02/20 19:55:49, engedy wrote:
> In that case, we can no longer rely on scheme-based suppression of the
password
> manager UI.
> 
> We could, as a very hacky temporary fix for M42, pass a flag into
> ChromePasswordManagerClient at construction time to let it know that it is
> inside a WebView (or let it figure that out for itself somehow). Mike would be
> able to comment on how reliably we could prevent all UI calls.
> 
> In the long run, however, we should certainly to decouple Autofill from
Password
> Autofill so that we do not need to force the latter into a WebView. We should
> also consider whether or not autofilling the username in M42 is really worth
the
> substantially higher risk of crashing.

I think that Password Autofill is really the requirement for webview (imagine if
you
have a Chrome generated password. Autofill is basically the only way that lets
you
sign into Chrome. 

The Password Save bubble is the one that would make the most sense to be 
decoupled, and would sort our this kind of scenario.

[noms (inactive), 2015-03-04 19:26:41]

vabr@/engedy@: ping! What should be the next steps for this CL? Can password
autofill and password saving be decoupled easily in the PasswordManager?

[vabr (Chromium), 2015-03-05 16:11:17]

> I think that Password Autofill is really the requirement for webview (imagine
if
> you
> have a Chrome generated password. Autofill is basically the only way that lets
> you
> sign into Chrome.

Using stored passwords as sync credentials is problematic. More precisely: sync
credentials cannot be stored, for security reasons (the sync credential, except
for custom passphrase users, allow to get all the other credentials).
So if the user has their credential stored in Chrome, and uses it to set up
sync, Chrome will very likely have problems with it -- currently, it won't
update that password on subsequent changes, and in the future it might try to
delete it.

The above present a problem, although not with this CL :). I'll add this to
http://crbug.com/458279.

So here you need password autofill, but not saving, correct?
It might be reasonable to introduce some read-only mode of the password manager,
where data already stored would be autofilled, but no saving and advanced
management (done through the UI missing in webview) would be available.

It would be good to have a design for that, and check carefully for the impact
on users. Read-only means, e.g., that passwords cannot be deleted or sites
blacklisted against filling passwords.

[vabr (Chromium), 2015-03-06 09:14:03]

The recent response on http://crbug.com/458279#c6 implies that the use-case from
https://codereview.chromium.org/866523002/#msg74 here is not possible.

So as an alternative to using password autofill in read-only mode, we should
still consider not using it in webview at all.

[noms (inactive), 2015-03-16 15:54:26]

On 2015/03/06 09:14:03, vabr (Chromium) wrote:
> The recent response on http://crbug.com/458279#c6 implies that the use-case
from
> https://codereview.chromium.org/866523002/#msg74 here is not possible.
> 
> So as an alternative to using password autofill in read-only mode, we should
> still consider not using it in webview at all.

Alright. I will close this CL and the bug as won't fix. We can revisit this
decision in the future.

Thanks everyone for sticking with this CL and not killing me in the process! :)